[PATCH] use cached lyrics + fix displaying of XHTML pages

Mon Sep 14 20:56:08 CEST 2009

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 <html> (right after the
DOCTYPE)
but for XHTML pages there's an xmlns and an xml:lang
attribute passed to the <html> tag...
so in XHTML the attribute might look like this:
     <html xmlns="http://www.w3.org/1999/xhtml" lang="en" xml:lang="en">

as amarok only checks if the document contains "<html>"
it would think that the given document is plaintext
because it starts with <html xmlns=.... (and not with
"<html>")

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 <HTML> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: amarok-lyrics_fix.patch
Type: text/x-diff
Size: 2990 bytes
Desc: not available
Url : http://mail.kde.org/pipermail/amarok-devel/attachments/20090914/d5a95054/attachment.bin 