New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plugins: enable links navigation in plugin descriptions #2280

Merged
merged 6 commits into from Mar 7, 2017

Conversation

Projects
None yet
3 participants
@ptitjes
Collaborator

ptitjes commented Feb 27, 2017

  • add a show_uri(label, uri) function to dispatch a uri
  • add a move_to(plugin_id) method to PluginWindow
  • remove automatic escaping of plugin descriptions
  • show uri for links inside plugin descriptions
  • use this mechanism for SqueezeboxPlaylistPlugin
  • correctly escape existing plugin descriptions
  • add a convenience clear() method to ClearEntryMixin

The reason for doing that is to ease the configuration of plugins that are dependent one to another. The Squeezebox set of plugins is an example of that. Also, I'd like to release my secondary (aka. preview) player code as two plugins (one that enables/provides the secondary player with configuration for its gst pipeline, and one that adds a "Pre-listen" SongsMenu item).

The show_uri function has been put in qltk.__init__.py so as to enable any part of the user interface to link to plugin preferences. You can do it with the following code:

label = Gtk.Label()
label.set_markup("<a href=\"ql:plugins:My Plugin Id\">My plugin</a>")
label.connect("activate-link", show_uri)

Ultimately, one can later add an if-branch in show_uri to handle "ql:prefs:" uri prefix to link to some preferences UI element.

ptitjes added some commits Feb 27, 2017

plugins: enable links navigation in plugin descriptions
* add a show_uri(label, uri) function to dispatch a uri
* add a move_to(plugin_id) method to PluginWindow
* remove automatic escaping of plugin descriptions
* show uri for links inside plugin descriptions
* use this mechanism for SqueezeboxPlaylistPlugin
* correctly escape existing plugin descriptions
* add a convenience clear() method to ClearEntryMixin
plugins: fix code style for links navigation to plugin preferences
* hack test_punctuation to skip ql:plugins: uri prefixes
@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Feb 27, 2017

Just stumbled upon #1865 which proposes a "quodlibet:" uri prefix...

@lazka

This comment has been minimized.

Member

lazka commented Mar 1, 2017

@declension Any thoughts one this? Since it includes squeeze box + plugin window work in #2218

@declension

This comment has been minimized.

Member

declension commented Mar 1, 2017

On holiday now, so in brief:

quodlibet:// seemed and seems like the best URL protocol to use to me - and more importantly is already in use by the Soundcloud auth flows (and there is some progress towards registering QL to handle these on an OS level - works for me etc)

Ideally this change would integrate with that rather than a separate URI handling (showing, here, but maybe not always) process - and one that would allow us eventually to deep-link to specific parts of the app e.g. the plugin preferences pages.

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Mar 2, 2017

@declension I'm not sure I understood all of your comment, sorry.

As for the scheme, "quodlibet" makes sense. I'm totally OK with it.

As for the //, you made me doubt so I went to check the URI specification again. The // should be present if and only there is an authority (a.k.a. host) before the hierarchical path part (explanation). I believe that a host makes no sense for QL's URIs and then that the absolute plugins/prefs access URIs should have the following shape:

quodlibet:/plugins/MyPluginId
quodlibet:/prefs/MyPrefPageId

with the possibility to focus on a particular widget with an additional optional "#WidgetId" part, when it is possible in the future.

Do we agree on that ?

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Mar 2, 2017

@declension As for the rest of your comment, I don't understand much the relationship between SoundCloud and QuodLibet.

However, I believe your are saying that we will have the "quodlibet:" scheme registered at the OS level. Are you referring to Gnome ? Also, I think at some point we have to do the dispatch of the URI inside QL, right ? So once the "quodlibet:" scheme is recognized by the OS we get the control and have to process the hierarchical part. So we will be able to reuse that part from my show_uri function.

So I propose that I split this function in two:

  • A function that is the internal "quodlibet:" URIs dispatch (__show_quodlibet_uri)
  • A function that is the current show_uri that either calls __show_quodlibet_uri or fallback to Gtk as it does currently

Do we agree on that too ?

@lazka

This comment has been minimized.

Member

lazka commented Mar 2, 2017

However, I believe your are saying that we will have the "quodlibet:" scheme registered at the OS level. Are you referring to Gnome ?

There is. xdg-open quodlibet://foo should open QL.


OK, so with multiple users of the quodlibet scheme we can have something like push those show_uri() calls to the global Application instance which can emit some "::show-uri" signal and interested parties can react to it. And we should namespace them. And we should be cautious, since that lets other programs/webbrowsers control QL..

@ptitjes You're right that the host part isn't needed, but we currently use "quodlibet://callbacks/soundcloud?code=" for soundcloud (https://github.com/quodlibet/quodlibet.github.io/blob/master/callbacks/soundcloud.html#L118). Is there any downside in just doing quodlibet://<namespace>/<path>? If no, please adjust to something like "quodlibet://pluginprefs/foo".

Unifying this can happen later..

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Mar 2, 2017

There is. xdg-open quodlibet://foo should open QL.

Really nice !!


emit some "::show-uri" signal and interested parties can react to it

I like that idea. That means we could handle those in plugins too.


You're right that the host part isn't needed

Well the specification says:

If a URI does not contain an authority component, then the path cannot begin with two slash characters ("//").

The reason explained is a question of parsing rules for absolute vs. relative URIs. That said we could devise our own parsing rules of relative and absolute URIs, but I think it would be error-prone because new-comers and external people would naturally write our URIs the wrong way.

That said I can see from looking at the cli.py code and searching the web, that there seem to be no utility to parse URIs in Python. So we could indeed use "quodlibet:///" for absolute URIs and "quodlibet://" for relative URIs. (Even if I really find that sooo ugly... :))

So that gives quodlibet:///prefs/plugin/PluginId for plugins preferences ?

Unifying this can happen later..

It won't be easy to change that later if there are lots of URIs everywhere...

@lazka

This comment has been minimized.

Member

lazka commented Mar 2, 2017

that there seem to be no utility to parse URIs in Python

from urlparse import urlparse

So that gives quodlibet:///prefs/plugin/PluginId for plugins preferences ?

ok

It won't be easy to change that later if there are lots of URIs everywhere...

If you think its easier in this PR, go ahead. Either way, please ask if you want a review.

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Mar 2, 2017

Unifying this can happen later..

@lazka Did you mean unifying how the URI is handled by cli.py and the current code of this PR ?

If yes, then I'd prefer to plug the dispatch mechanism (for which I created issue #2292) inside the show_uri() function later. (And also move the if "prefs/plugin/..." in a signal handler.)

If you are OK with that, I'll add a commit here to modify the URIs syntax and then ask you to review it.

@lazka

This comment has been minimized.

Member

lazka commented Mar 3, 2017

@lazka Did you mean unifying how the URI is handled by cli.py and the current code of this PR ?

If yes, then I'd prefer to plug the dispatch mechanism (for which I created issue #2292) inside the show_uri() function later. (And also move the if "prefs/plugin/..." in a signal handler.)

yes

If you are OK with that, I'll add a commit here to modify the URIs syntax and then ask you to review it.

ok

ptitjes added some commits Mar 3, 2017

plugins: move to 'quodlibet:///prefs/plugins/' URI format
* use existing URL parse routines
* use 'quodlibet' as scheme
* add a link generation utility function
* split show_uri to prepare handling QL URI differently
@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Mar 3, 2017

@lazka Wasn't sure as how to treat the links inside translations. I think it is ready to review however.

return __show_quodlibet_uri(parsed)
else:
from quodlibet import app
return Gtk.show_uri_on_window(app.window, uri, 0)

This comment has been minimized.

@lazka

lazka Mar 6, 2017

Member

show_uri_on_window is 3.22+

if hasattr(Gtk, "show_uri_on_window"): ... else: Gtk.show_uri()

the window should be the one containing the label, use qltk.get_top_parent(label)

This comment has been minimized.

@lazka

lazka Mar 6, 2017

Member

Alternative you can just make the return value "True if the uri was handled" and return False here. I guess "activate-link" falls back to show_uri() anyway if you return False in the handler (??)

This comment has been minimized.

@ptitjes

ptitjes Mar 6, 2017

Collaborator

I have to test it again. IIRC, if I return False, it just writes an error in the console.
(Hopefully, Gtk.show_uri() is 3.14 and we just moved to that version ! :))

@@ -223,9 +223,15 @@ def enable_clear_button(self):
self.set_icon_from_gicon(Gtk.EntryIconPosition.SECONDARY, gicon)
self.connect("icon-release", self.__clear)
def clear(self):
self.__do_clear()
def __clear(self, button, *args):
# TODO: don't change the order.. we connect to clear and remove all

This comment has been minimized.

@lazka

lazka Mar 6, 2017

Member

That comment should move to __do_clear as well

specific one.
Currently handled quodlibet uris:
- ql:plugins:<plugin id>

This comment has been minimized.

@lazka

lazka Mar 6, 2017

Member

needs an update

r'[a-z]'
r'(?<!people)'
r'(?<!ql)'
r'(?<!plugins)'

This comment has been minimized.

@lazka

lazka Mar 6, 2017

Member

Is this change still needed?

This comment has been minimized.

@ptitjes

ptitjes Mar 6, 2017

Collaborator

Well, I'm surprised it passed the CI. because it is still ql...

This comment has been minimized.

@ptitjes

ptitjes Mar 6, 2017

Collaborator

Oh no, you're right! Now there is a / after the colon.

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Mar 6, 2017

@lazka Thank you. I'll fix all that.

@ptitjes

This comment has been minimized.

Collaborator

ptitjes commented Mar 6, 2017

@lazka Fixed.

@lazka lazka merged commit 16b5ccb into quodlibet:master Mar 7, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lazka

This comment has been minimized.

Member

lazka commented Mar 7, 2017

Thanks

@ptitjes ptitjes deleted the ptitjes:pluginlinks branch Mar 7, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment