Integration #9

Closed
wants to merge 37 commits into
from

Projects

None yet

2 participants

@onto
Contributor
onto commented Mar 17, 2012

Hello, I reorganized all my commits.

Can you inspect and if they match then pull)

@multani

Hum, why not, though I'm not sure this is really useful. Probably the default location is fine most of the the time, and we can't content by default everybody.

Two things though:

  • Please keep the lines under 80 characters
  • Remove the "/" in the _("SONG_DIR/"), we don't want a new translation string for this.
Owner
onto replied Aug 21, 2012
@multani

Good.

  • construct_tab in streams.py is not used anymore
  • construct_tab in playlist.py is not used anymore
  • you import gettext in playlists.py but it doesn't seem to be used/useful?
  • keep Stream.init() under 80 columns
  • you don't use new_tab in playlists.py, the Playlist tab doesn't appear anymore
@multani

This is great!

Keep columns under 80 characters though (for "disconnect" for example)

Owner
onto replied Mar 20, 2012

How can I make a small change in this commit? Only in new commit?

Owner
onto replied Aug 21, 2012

Fix it in 0ac868b

@multani

Hum, OK, but this is not used at the moment...

I'm not willing to merge it if it's not directly useful, especially if it's a new feature for plugin writer, which doesn't seem to be documented somewhere.

Owner
onto replied Mar 20, 2012

I think it will useful in future (i want to add "loved" button from lastfm plugin).

Owner
onto replied Aug 14, 2012

As I understand from the code, the order of plugins in the list and, accordingly, the order of the buttons are in alphabetical order if all plugins in one folder.

Owner
onto replied Aug 21, 2012

Now it renamed to "add_toolbar_button" ae532ed and used in a53de59

@multani

Why? I don't get the point and would prefer to have a default nice Sonata with lyrics and covers (although it means having default plugins which work).

I don't want to merge this, unless you have a good reason.

Also, come on:  self.DEFAULT_PLUGINS = []

Owner
onto replied Mar 20, 2012

Yes, most likely you're right.

@multani
  • it uses two spaces instead of four for the indentation
  • it contains way too much hard-coded values (paths to the configuration file, art work, etc.)
  • it does the parsing of the configuration file by itself, instead of relying on Sonata config object or something
  • it "crashes" if pynotify is not available
  • I'm not sure if I or we have the permission to add this into Sonata's core repository
  • the picture is not resized, if it's big, the pop-up will be big

For those reasons, I don't want to merge this. Additionnaly, it "conflicts" with the default notification system of Sonata (both can be enabled, in different places in the preferences window). We should do something about it before adding the plugin.

Owner
onto replied Mar 20, 2012

Some people(as me) like to using system-defined notification system (like notify-osd or similar in xfce and kde) for notifications.

If you find that similar plugin has a place in the sonata, then I can write a plug-in without the disadvantages of this.

@multani

Sounds good.

  • Please check the columns size of toggle_actions
  • You removed the accelerator to "Show Sonata", Random and Repeat but I don't know where this menu is used. Can you tell me?
  • You should consider adding the Consume entry into the tray menu
Owner
onto replied Aug 21, 2012
@multani

I guess you can really remove the unused gettext import, or I don't get your commit message then.

@multani

That's bunch of new plugins!
I think we should only ship one by default, which does the job, and provide the others as external plugins, but I'm not sure how to do this in Sonata, though.

  • you can greatly simplified your plugins by removing the classes, and doing the work in get_lyrics directly. No need to have globals, instantiation, classes, and all the rest (look at 3825253 for an example)
  • There is a test infrastructure since 315a16b and I really encourage you to test your formatting/translating function, instead of adding a "It should" comment
  • you should consider using string.maketranslate() and string.translate() for converting large series of characters (you can then store the translation table as a global in the module). It's maybe not really important, though (it processes small amount of text, the gain should not be so interesting).
  • I did some work on LyricsWiki, could be nice to have a look and reuse it...
  • You can replace all your classes
@multani

I like that the plugins don't need to launch themselves into thread, that's great!

  • you have some unused import in info.py
  • you must call self.get_lyrics_response() from the main thread using gobject.timeout_add or somthing like this, as it was done previously.
  • consider adding logging call in debug/info level to explain that Sonata is fetching plugins (with which plugin also)
  • as I said in e1dc1e, it would have been cool to adapt the plugin instead of throwing it away completely. Unless the new on does exactly the same, which would be cool since it's much shorter. But I guess if it was "so" complicated had a good reason.
Owner
onto replied Aug 21, 2012
  • 85bcc5d
  • I don't clearly understand callback from old code. May be it can solve with thread.setDaemon()?
  • 554e168
  • rewritten plugin works similar but smaller
@multani

For one thing, that's very cool that the scrobbler plugin is not tightly integrated into Sonata as before, thanks for this!

On the other hand, I really don't want to import another scrobbler implementation, especially which uses the old 1.2 API of Last.fm. I don't want this offmessage/pyscrobbler#5 especially when there are dozens of implementation available outside.

I'm willing to merge a change which:

  • extract the scrobbler features from Sonata core into a plugin
  • provide an interface to the existing scrobbler module in Sonata, in the hope that one day we can remove it and use one of the external Scrobbler plugin.

But please, no more code dump from outside, heavily modified, which we (Sonata dev ) will need to maintain, fix and improve.

Owner
onto replied Mar 20, 2012

You mean I need to rewrite the plugin to use external library with 2.0 API of last.fm?

Owner
onto replied Mar 20, 2012

Ok, I planned it for the future.

Owner
onto replied Aug 15, 2012

Please look at a53de59

@multani

This is really great!

Could improve it though, so :

This could serve as a good base if we decide to improve the plugin system.

@multani

Good, I can merge this (the user still needs "gksu", though).

Owner
onto replied Aug 14, 2012

May be it should rewrite somehow like this:

sus = ["gksu","kdesu","beesu","gnomesu","xdg-su"]

for su in sus:
try:
subprocess.Popen([su, "xdg-open", "/etc/mpd.conf"]))
break
except OSError:
pass

but I don't know how it rewrite to lambda.

@multani

Sounds good.
It breaks the API of the plugins though...

Owner
onto replied Mar 20, 2012

Seems API was not really described anywhere?

@multani

You must change your comment, we (I) not idea what you mean.

It looks like the interface of this plugin hook is not very well defined, I guess that's why it has never been implemented.
I would prefer to merge something which does the job or at least, which is used by at least one plugin to see how we can improve things.

Now, it's a just a dummy implementation for something not used, which you don't seem to really appreciate anyway.

Owner
onto replied Mar 20, 2012

It used, for example, in my scrobbler plugin to show configuration window(for username and password).

Maybe we can call "plugin_configure" on selected plugin in another way.

Owner
onto replied Aug 21, 2012
@multani
Owner
multani commented Mar 20, 2012

I had a look at all your changesets and overall it looks good:

I believe I clearly said what I wanted to merge and what I don't want to in the actual state.

Please pay attention at the length of your lines, I don't want another pep8-ification run in a few months.

You changed a lot of things related to the plugins interface. The small doc you started is a good start but it needs to be improved, and it would be nice (although I don't know if it will be useful) to document in the README or somewhere else the list of changes you made related to this interface (I'm thinking things like, auto-background processing of lyrics plugin s, tab_construct name change, etc.)

Overall, I can't merge this like this, too many small things to fix first, and small changesets I just don't want in Sonata at the moment. You should consider removing them from the branch you want to merge, or change them.

@onto
Owner
onto commented on 554e168 Aug 21, 2012

Like this:
[2012-08-21 20:20:40] sonata.info: Lyrics for 'Paramore - Throwing Punches' fetched by azlyrics plugin.
[2012-08-21 20:29:10] sonata.info: Lyrics for 'System Of A Down - Boom!' fetched by azlyrics plugin.
[2012-08-21 20:40:57] sonata.info: Lyrics for 'Lumen - Другой мир' fetched by alltexts plugin.

@multani
Owner
multani commented Nov 7, 2012

Hi,

I did a pass on your branch, reorganized commit together, grouped things related, squashed things that could be squashed so I can understand it better, and rebased everything on top of master, which took me already a while.

I made several smaller branches from your big branch, and merged some of them. There are a few that I didn't merge yet, and I will open new pull requests for each of those branches, that I invite you to clone in your local repository and make some fixes that I will recommend you.

I'm closing this now, please have a look at the new pull requests. Thanks.

@multani multani closed this Nov 7, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment