From amarok-devel Mon Sep 14 18:56:08 2009 From: Martin Date: Mon, 14 Sep 2009 18:56:08 +0000 To: amarok-devel Subject: [PATCH] use cached lyrics + fix displaying of XHTML pages Message-Id: <888a464d0909141156s1f73b7e3xe4493b7e3981a06f () mail ! gmail ! com> X-MARC-Message: https://marc.info/?l=amarok-devel&m=125295511932726 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--0015175906b6f5a72504738e3787" --0015175906b6f5a72504738e3787 Content-Type: text/plain; charset=UTF-8 Hi, ok.. for the last week I've been without internet (AGAIN thanks to my great ISP) while being without internet I noticed that my (cached) lyrics were not displayed I've attached a patch that fixes the problem mentioned above plus another (minor) issue first: why were the cached lyrics not displayed: in LyricsEngine.cpp there's the following code: const bool cached = !lyrics.isEmpty() && !The::engineController()->isStream() && ( currentTrack->name() == m_title ) && ( currentTrack->artist()->name() == m_artist ); now you may ask what's wrong with this... then just take a look at the next two lines of code: m_title = currentTrack->name(); m_artist = currentTrack->artist()->name(); got it? :) first the const bool cached code checks if the current track's name matches m_title but this will NEVER be true, because m_title is assigned AFTER checking (same applies to m_artist) so I simple removed that check... as it should not be necessary (since currentTrack should ALWAYS point to the current song (Meta::TrackPtr) - if NOT there would be a bug somewhere the other problem I've found was that some XHTML sites were treated as plaintext in the lyrics applet. the solution to this was also very easy but first some background information: usually a HTML site starts with (right after the DOCTYPE) but for XHTML pages there's an xmlns and an xml:lang attribute passed to the tag... so in XHTML the attribute might look like this: as amarok only checks if the document contains "" it would think that the given document is plaintext because it starts with ") also regarding this XHTML fix I've added some missing Qt::CaseInsensitive flags to the .contains() calls (which might be useful if someone thinks starting a page with looks better) I just see one usuability change with my patch: as now the cached lyrics will be used, the user will NOT be informed if he has already lyrics stored in his database for the current track and reloading the lyrics (fetching them from a server) failed. that's because when fetching the lyrics failed amarok will not show the "could not fetch" lyrics message as it already has some lyrics. maybe the error message should not be displayed in the QTextBrowser of the lyrics applet itself, but some "cool info bar" above the QTextBrowser with "cool info bar" I mean something like the "Get data from openDesktop.org to learn more"... in the About dialog (in the authors tab). I've made a (bad) mockup of this - see [0] an alternative (in my opinion) would be showing the error messages like amarok 1.x did it (bottom left, in an dialog box - maybe KNotification could be used for this) BUT: as I'm not a graphics gui I suck at UI designing and thus thinking about usuability so those ideas are what *I* would do ;) Regards, Martin [0] http://www.abload.de/img/amarok-lyrics_fixu3u9.png --0015175906b6f5a72504738e3787 Content-Type: text/x-diff; charset=US-ASCII; name="amarok-lyrics_fix.patch" Content-Disposition: attachment; filename="amarok-lyrics_fix.patch" Content-Transfer-Encoding: base64 X-Attachment-Id: f_fzlkrwqm0 ZGlmZiAtLWdpdCBhL3NyYy9jb250ZXh0L0x5cmljc01hbmFnZXIuY3BwIGIvc3JjL2NvbnRleHQv THlyaWNzTWFuYWdlci5jcHAKaW5kZXggZDgzMjBiMi4uZmUzZDVlYSAxMDA2NDQKLS0tIGEvc3Jj L2NvbnRleHQvTHlyaWNzTWFuYWdlci5jcHAKKysrIGIvc3JjL2NvbnRleHQvTHlyaWNzTWFuYWdl ci5jcHAKQEAgLTE2Nyw3ICsxNjcsNyBAQCBMeXJpY3NNYW5hZ2VyOjpseXJpY3NSZXN1bHQoIGNv bnN0IFFTdHJpbmcmIGx5cmljc1hNTCwgYm9vbCBjYWNoZWQgKSAvL1NMT1QKICAgICAgICAgICAg IGx5cmljcyA9IFRoZTo6ZW5naW5lQ29udHJvbGxlcigpLT5jdXJyZW50VHJhY2soKS0+Y2FjaGVk THlyaWNzKCk7CiAgICAgICAgICAgICAvL2RlYnVnKCkgPDwgInVzaW5nIGNhY2hlZCBseXJpY3M6 ICIgPDwgbHlyaWNzOwogCi0gICAgICAgICAgICBpZiggbHlyaWNzLmNvbnRhaW5zKCAiPGh0bWw+ IiAsIFF0OjpDYXNlSW5zZW5zaXRpdmUpICkKKyAgICAgICAgICAgIGlmKCBseXJpY3MuY29udGFp bnMoICI8aHRtbCIgLCBRdDo6Q2FzZUluc2Vuc2l0aXZlKSApCiAgICAgICAgICAgICB7CiAgICAg ICAgICAgICAgICAgLy93ZSBoYXZlIHN0b3JlZCBodG1sIGx5cmljcywgc28gdXNlIHRoYXQgZGly ZWN0bHkKICAgICAgICAgICAgICAgICBzZW5kTmV3THlyaWNzSHRtbCggbHlyaWNzICk7CkBAIC0y MTgsNyArMjE4LDcgQEAgTHlyaWNzTWFuYWdlcjo6bHlyaWNzRXJyb3IoIGNvbnN0IFFTdHJpbmcg JmVycm9yICkKICAgICAgICAgZGVidWcoKSA8PCAic2hvd2luZyBjYWNoZWQgbHlyaWNzISI7CiAg ICAgICAgIC8vVE9ETzogYWRkIHNvbWUgc29ydCBvZiBmZWVkYmFjayB0aGF0IHdlIGNvdWxkIG5v dCBmZXRjaCBuZXcgb25lcwogICAgICAgICAvL3NvIHdlIGFyZSBzaG93aW5nIGEgY2FjaGVkIHJl c3VsdAotICAgICAgICBpZiggY3VycmVudFRyYWNrLT5jYWNoZWRMeXJpY3MoKS5jb250YWlucygg IjxodG1sPiIgLCBRdDo6Q2FzZUluc2Vuc2l0aXZlICkgKQorICAgICAgICBpZiggY3VycmVudFRy YWNrLT5jYWNoZWRMeXJpY3MoKS5jb250YWlucyggIjxodG1sIiAsIFF0OjpDYXNlSW5zZW5zaXRp dmUgKSApCiAgICAgICAgIHsKICAgICAgICAgICAgIC8vd2UgaGF2ZSBzdG9yZWQgaHRtbCBseXJp Y3MsIHNvIHVzZSB0aGF0IGRpcmVjdGx5CiAgICAgICAgICAgICBzZW5kTmV3THlyaWNzSHRtbCgg Y3VycmVudFRyYWNrLT5jYWNoZWRMeXJpY3MoKSApOwpkaWZmIC0tZ2l0IGEvc3JjL2NvbnRleHQv ZW5naW5lcy9seXJpY3MvTHlyaWNzRW5naW5lLmNwcCBiL3NyYy9jb250ZXh0L2VuZ2luZXMvbHly aWNzL0x5cmljc0VuZ2luZS5jcHAKaW5kZXggZDk5Y2VjYS4uNDQ1MzBmMiAxMDA2NDQKLS0tIGEv c3JjL2NvbnRleHQvZW5naW5lcy9seXJpY3MvTHlyaWNzRW5naW5lLmNwcAorKysgYi9zcmMvY29u dGV4dC9lbmdpbmVzL2x5cmljcy9MeXJpY3NFbmdpbmUuY3BwCkBAIC0xMjAsNyArMTIwLDcgQEAg dm9pZCBMeXJpY3NFbmdpbmU6OnVwZGF0ZSgpCiAgICAgUVN0cmluZyBseXJpY3MgPSBjdXJyZW50 VHJhY2stPmNhY2hlZEx5cmljcygpOwogICAgIAogICAgIC8vIGRvbid0IHJlbHkgb24gY2FjaGlu ZyBmb3Igc3RyZWFtcwotICAgIGNvbnN0IGJvb2wgY2FjaGVkID0gIWx5cmljcy5pc0VtcHR5KCkg JiYgIVRoZTo6ZW5naW5lQ29udHJvbGxlcigpLT5pc1N0cmVhbSgpICYmICggY3VycmVudFRyYWNr LT5uYW1lKCkgPT0gbV90aXRsZSApICYmICggY3VycmVudFRyYWNrLT5hcnRpc3QoKS0+bmFtZSgp ID09IG1fYXJ0aXN0ICk7CisgICAgY29uc3QgYm9vbCBjYWNoZWQgPSAhbHlyaWNzLmlzRW1wdHko KSAmJiAhVGhlOjplbmdpbmVDb250cm9sbGVyKCktPmlzU3RyZWFtKCk7CiAgICAgCiAgICAgbV90 aXRsZSA9IGN1cnJlbnRUcmFjay0+bmFtZSgpOwogICAgIG1fYXJ0aXN0ID0gY3VycmVudFRyYWNr LT5hcnRpc3QoKS0+bmFtZSgpOwpAQCAtMTUyLDcgKzE1Miw3IEBAIHZvaWQgTHlyaWNzRW5naW5l Ojp1cGRhdGUoKQogCiAgICAgaWYoIGNhY2hlZCApCiAgICAgewotICAgICAgICBpZiggbHlyaWNz LmNvbnRhaW5zKCAiPGh0bWw+IiApICkKKyAgICAgICAgaWYoIGx5cmljcy5jb250YWlucyggIjxo dG1sIiAsIFF0OjpDYXNlSW5zZW5zaXRpdmUgKSApCiAgICAgICAgICAgICBuZXdMeXJpY3NIdG1s KCBseXJpY3MgKTsKICAgICAgICAgZWxzZQogICAgICAgICB7CkBAIC0xNjUsNyArMTY1LDcgQEAg dm9pZCBMeXJpY3NFbmdpbmU6OnVwZGF0ZSgpCiAgICAgewogICAgICAgICByZW1vdmVBbGxEYXRh KCAibHlyaWNzIiApOwogICAgICAgICBzZXREYXRhKCAibHlyaWNzIiwgIm5vc2NyaXB0cnVubmlu ZyIsICJub3NjcmlwdHJ1bm5pbmciICk7Ci0gICAgICAgIG1fY3VycmVudEx5cmljcyA9ICJMeXJp Y3MgIFVuYXZhaWxhYmxlIjsKKyAgICAgICAgbV9jdXJyZW50THlyaWNzID0gIkx5cmljcyBVbmF2 YWlsYWJsZSI7CiAgICAgfQogICAgIGVsc2UKICAgICB7CmRpZmYgLS1naXQgYS9zcmMvZGlhbG9n cy9UYWdEaWFsb2cuY3BwIGIvc3JjL2RpYWxvZ3MvVGFnRGlhbG9nLmNwcAppbmRleCA4Njc3ODVi Li4yOTBkZWQxIDEwMDY0NAotLS0gYS9zcmMvZGlhbG9ncy9UYWdEaWFsb2cuY3BwCisrKyBiL3Ny Yy9kaWFsb2dzL1RhZ0RpYWxvZy5jcHAKQEAgLTkzNCw3ICs5MzQsNyBAQCB2b2lkIFRhZ0RpYWxv Zzo6cmVhZFRhZ3MoKQogCiAgICAgLy9seXJpY3MKICAgICAvLyBpZiB0aGVyZSBpcyBubyA8aHRt bD4gdGFnLCBzZXQgaXQgYXMgdGV4dCBpbnN0ZWFkCi0gICAgaWYoIG1fbHlyaWNzLmNvbnRhaW5z KCAiPGh0bWw+IiApICkKKyAgICBpZiggbV9seXJpY3MuY29udGFpbnMoICI8aHRtbCIgLCBRdDo6 Q2FzZUluc2Vuc2l0aXZlICkgKQogICAgICAgICB1aS0+a1RleHRFZGl0X2x5cmljcy0+c2V0SHRt bCggbV9seXJpY3MgKTsKICAgICBlbHNlCiAgICAgICAgICB1aS0+a1RleHRFZGl0X2x5cmljcy0+ c2V0UGxhaW5UZXh0KCBtX2x5cmljcyApOwoK --0015175906b6f5a72504738e3787 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Amarok-devel mailing list Amarok-devel@kde.org https://mail.kde.org/mailman/listinfo/amarok-devel --0015175906b6f5a72504738e3787--