Feature Request: Lyrics Dropdown Selection in Lyrics Pane

[GSF1200S]

While Exaile has a very good method of displaying lyrics (I have yet to see better), I have a suggestion for improving the server selection of a lyrics engine. As it stands now, Exaile users have two options: lyricsfly and lyricswiki. Between the two, I can find lyrics for just about every one of my songs. However, with both extensions (lyricsfly and lyricswiki) checked in the plugin manager, lyrics wiki will ALWAYS show if it outputs lyrics. As a result of lyricswiki having issues with copyrights for many songs, this results in the first two lines of lyrics being displayed and then a message that they are not licensed to display the rest of the lyrics. A user must then go to Edit > Preferences > Plugins and uncheck lyricswiki so that lyricsfly will show.

I propose the following: at the top of the lyrics pane, a simple dropdown box containing all available lyrics servers. If you guys add more lyrics engines in the future, it will make it even easier for users to switch engines on the fly (as opposed to enabling/disabling in the extension list).

[Rocco Aliberti (eri.trabiccolo)]

I tried a solution, adding / changing a few lines of code to the class xl / lyrics.
I created a small class for the combo that allows you to easily manage the features.
See photo -> http://img249.imageshack.us/img249/5999/exaile.jpg
If you want i attach all the files (xl/lyrics patch, new plugin lyricsviewer and the class for the combo).

[Rocco Aliberti (eri.trabiccolo)]

In attachment a patch with new lyricsviewer and the combo.

[reacocard (reacocard)]

Well, i tried that patch against latest trunk, however it wouldn't load, failing with this error:

Traceback (most recent call last):
  File "/home/reacocard/projects/exaile/trunk/xl/plugins.py", line 121, in enable_plugin
    plugin = self.load_plugin(pluginname)
  File "/home/reacocard/projects/exaile/trunk/xl/plugins.py", line 83, in load_plugin
    plugin = imp.load_source(pluginname, os.path.join(path,'__init__.py'))
  File "/home/reacocard/projects/exaile/trunk/plugins/lyricsviewer/__init__.py", line 147
     properties=[gtk.STATE_NORMAL,gtk.STATE_ACTIVE,gtk.STATE_PRELIGHT, \
                                                                         ^
 SyntaxError: unexpected character after line continuation character
18:06:55,501:WARNING : Unable to enable plugin lyricsviewer (xl.plugins)

Putting that aside, a few comments just from looking at the .patch:
 - Don't use tabs to indent your python code. Exaile standard is to use 4 spaces for indent, and as python is a whitespace-sensitive language keeping this consistent is VERY important. Also the fact that all the existing space-indents got converted to tabs makes it very hard to tell what actually changed when looking at the diff.
 - combo_lyrics_methods.py would probably be better named as lyrics_methods_combobox.py or the like, though frankly its so short you might as well just leave its contents in __init__.py.
 - In your combobox, the default choice is labeled "All", I think "Any" would be more appropriate, since we only show one source at a time, not all of them.
 - Not sure what's up with the whole "allow" mess in xl/lyrics.py; why not just change it so that lyrics for all sources are returned instead of just one source, and cache those results in the plugin? That would be simpler, more efficient and doesn't risk interfering with the case where two parts of the code might query lyrics simultaneously.

And one feature suggestion:
 - Extend the LyricSearchMethod class to have a display_name attribute, which each method can then set to a value that will be displayed in your combobox. That will allow us to show names in a more-readable format, not to mention allowing them to be translatable.

[Rocco Aliberti (eri.trabiccolo)]

- About the tabs, you're right, i I have problems with the editor, but i will adjust.

- For the two points about the combo, you're right!

- About allowing all methods: I tried to be less invasive as possible. xl/lyrics.py behaves like this:
calls the find_lyrics method of one "method search" at a time, ending when it finds the first result. I didn't change this thing because I thought it was already a well-considered choice and to let it compatible with the plugin contextinfo. But i can leave xl/lyrics.py as it was before, adding only a new method that returns the lyrics for all sources, and manage everything in the plugin.

About the suggestion:
- Is it really necessary? Why not directly change the attribute "name" in a readable string? Consider that each search method returns a "source" string, which would be the display name.

[Rocco Aliberti (eri.trabiccolo)]

I arranged it this way:
- Added a new method to xl / lyrics.fly that returns the lyrics from all allowed sources, thus maintaining compatibility with other plugins that will use the original method. The '"Allowing" is used only by this method.
- In the plugin I get all the lyrics and display the first (respecting the preferences [when they are implemented]), scrolling the combo you can see the other sources.

Remains to be clarified if we wanna change the "name" attribute of LyricSearchMethod, make it equal to "source", or create an additional attribute "display_name. Let me know

In the meanwhile i clean a bit the code and then put the patch here.

[reacocard (reacocard)]

> The '"Allowing" is used only by this method.
What does this mean? If you're getting all sources, you shouldn't need any of that allow stuff at all anymore.

> Is it really necessary? Why not directly change the attribute
> "name" in a readable string?
Because display names should be translatable and thus are not constant. Internal names need to be constant otherwise it tends to create subtle bugs.

> Consider that each search method returns a "source" string, which
> would be the display name.
True, but at some point I'd like to add a chooser in exaile's preferences to allow changing the order lyrics backends are searched in, much like the current setup for covers. It'd be nice to be able to use the same, translated names in both places, and a simple display_name attribute is the easiest way to do this.

[Rocco Aliberti (eri.trabiccolo)]

Means that i would keep "that allow stuff" 'cause to fetching all the sources is expensive in terms of resources and time. But if you want i take it off.

Ok for the other two points

[Rocco Aliberti (eri.trabiccolo)]

Modified.
Patch is made against r3069.

[Rocco Aliberti (eri.trabiccolo)]

Improved.
Patch is made against r3076

[Rocco Aliberti (eri.trabiccolo)]

fix: improved the handling of the motion event on the url.
Patch made against r3078

[Rocco Aliberti (eri.trabiccolo)]

Sorry, uploaded the wrong version.

[Rocco Aliberti (eri.trabiccolo)]

Excuse me, yesterday I made a big mess.
Final version (unless you encounter bugs).

[reacocard (reacocard)]

Patch committed trunk/3085 with a few minor stylistic changes and one bugfix. Thanks for all your work on this! :D

[Rocco Aliberti (eri.trabiccolo)]

Aren, sorry, out of curiosity, what was the bug?
I only see that in lyricsviewer.py at the event stop_playback you clean the page, except fo the trackname textview (!!).
In the search methods now you return not the source string but the name attribute, and this appears in the bottom bar, maybe also in this case would be better to return display_name attribute (?).
Last thing, in the commit missing the dir lyricsviwer/images (with the gif).

And don't thank me, it was a pleasure.

[Rocco Aliberti (eri.trabiccolo)]

The problem about the dir is fixed.
There's another "little problem" about clearing on stop_cb. When you start a new song and there's a song in playing, almost simultaneously, come two events: stop and playback. Maybe we have to synchronize update_lyrics(), cause might happen, for thread management that does not concern exaile, something like this:

/usr/share/exaile/plugins/lyricsviewer/__init__.py:260: GtkWarning: Invalid text buffer iterator: either the iterator is uninitialized, or the characters/pixbufs/widgets in the buffer have been modified since the iterator was created.

It happened to me with the result to replicate the text twice in the bottom TextView.

[reacocard (reacocard)]

The bug was that it wasn't clearing the panel when playback stopped, hence my attempting to fix it by adding a clear to stop_cb. In trunk/3097 I've moved the clear to a new method, end_cb, that is called on the playback_player_end event - ie. it's only triggered when playback fully ceases, not on track changes. I also adjusted it to ensure that the trackname it also cleared properly.

[Rocco Aliberti (eri.trabiccolo)]

Ok, so that was the bug. It was not a really bug for, i wanted it so. But nothing it's fine in this way. Two things:
1. I noticed that would be better to flush the track_text_buffer always like we do for the others at the beginning of update_lyrics(), cause happens that when you play a song with no name or no artist, it's not reset (exception) and so it keeps the name of the previous processed track.
2. I also noticed that when you disable the plugin it continues to work (I know because I have in my LyricWiki some debug's print lines), don't know why, so it's better to remove the callbacks.
I attach the patch.

[reacocard (reacocard)]

New patch added trunk/3098.

[Rocco Aliberti (eri.trabiccolo)]

Now I think the reaction to player_track_end is useless, is not it?
Removed in the attachment.

[Rocco Aliberti (eri.trabiccolo)]

I don't know how is useful, because many things must still be set via code.
Anyway...
Patch made with "bzr diff -p0"

[Rocco Aliberti (eri.trabiccolo)]

"New" version, i think would be better to start a new tread opening the url, to prevent temporary freeze of the main gui.

[Rocco Aliberti (eri.trabiccolo)]

Other minor fixes.
Fixed a problem with the setup_top_box() method.
Made the update_source_text() method synchronized in order to prevent the aforementioned:

GtkWarning: Invalid text buffer iterator: either the iterator is uninitialized, or the characters/pixbufs/widgets in the buffer have been modified since the iterator was created.

which sometimes happened to me skipping fast many tracks.

[Rocco Aliberti (eri.trabiccolo)]

Final version:
- removed the callback playback_track_end
- added the .ui
- fixed a temporary gui freeze opening the url
- fixed a problem of synchronism accessing the lyrics_source_text_buffer
- fixed a problem of consistency on the list lyrics_found

[Rocco Aliberti (eri.trabiccolo)]

sorry I just noticed i didn'n add the .ui file in the patch.

[Rocco Aliberti (eri.trabiccolo)]

other changes:
- The attribute lyrics_found is set only if really necessary