should support settings:///system/<...> urls

Bug #1227690 reported by Thomas Strehl
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
ubuntu-system-settings
Fix Released
High
Alberto Mardegan
ubuntu-system-settings (Ubuntu)
Fix Released
High
Unassigned

Bug Description

url-dispatcher might invoke system-settings with a request to show a certain page/setting. The page is now indicated by a URL, e.g. 'settings:///system/battery' which needs to be properly interpreted by system app.

Related branches

Revision history for this message
Iain Lane (laney) wrote :

system-settings understands how to launch a panel if you call it with the panel name as argument. e.g. system-settings about works

Revision history for this message
Sebastien Bacher (seb128) wrote :

those url schemes are from upstart-app-launch I think ... Ted, can you confirm?

affects: ubuntu-system-settings → upstart-app-launch
Changed in upstart-app-launch:
importance: Undecided → High
Changed in ubuntu-system-settings (Ubuntu):
importance: Undecided → High
status: New → Incomplete
Revision history for this message
Thomas Strehl (strehl-t) wrote :

Well, upstart-app-launch just forward/initiate those, but they are defined in the indicators themselves, e.g. url_dispatch_send("settings://system/battery", NULL, NULL);

Revision history for this message
Ted Gould (ted) wrote :

They were defined by an e-mail between tvoss, mardy and myself. Then routing was implemented in url-dispatcher and they're being used by the indicators. System settings needs to be able to catch them as well.

affects: upstart-app-launch → ubuntu-system-settings
Revision history for this message
Loïc Minier (lool) wrote :

So this needs the system-settings .desktop file to have %u, and for system-settings to handle settings://system/battery.

What's not clear is where the list of settings:// is defined?

Changed in ubuntu-system-settings (Ubuntu):
status: Incomplete → New
Revision history for this message
Sebastien Bacher (seb128) wrote :

Ted said that Alberto said he would look at that ... Alberto, do you still plan to work on that change?

Changed in ubuntu-system-settings:
assignee: nobody → Alberto Mardegan (mardy)
status: New → Triaged
Changed in ubuntu-system-settings (Ubuntu):
status: New → Triaged
summary: - system-settings app doesn't handle page/setting request properly
+ should support settings://system/<...> urls
Revision history for this message
Iain Lane (laney) wrote : Re: [Bug 1227690] Re: system-settings app doesn't handle page/setting request properly

On Tue, Sep 24, 2013 at 04:03:42PM -0000, Loïc Minier wrote:
> So this needs the system-settings .desktop file to have %u, and for
> system-settings to handle settings://system/battery.

Handle in what way? I guess a message will be delivered to u-s-s
somehow?

> What's not clear is where the list of settings:// is defined?

On our side, /usr/share/ubuntu/settings/system/*.settings

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Alberto Mardegan (mardy) wrote : Re: should support settings://system/<...> urls

I'm working on something else (also urgent!) at the moment; I think I could start working on this on Friday. If you think it deserves higher priority, someone else needs to work on it. :-)

Revision history for this message
Sebastien Bacher (seb128) wrote :

> Handle in what way? I guess a message will be delivered to u-s-s somehow?

My understanding is that it's going to go through the normal mimetype handling mechanism, which means clicking on "settings://...." is going to call the default handle for settings: (which is u-s-s) "system-settings settings://system/sound" for example ... we should parse the "settings://system/<name>" argument as a request to open "<name>"

(the "system" part of the name is because they want to reserve the option to have different settings in the futur, the alternative is to use "system-settings://<name>")

> I think I could start working on this on Friday. If you think it deserves higher priority, someone else needs to work on it.

Thanks Alberto, friday should be fine

Revision history for this message
Iain Lane (laney) wrote : Re: [Bug 1227690] Re: should support settings://system/<...> urls

On Wed, Sep 25, 2013 at 10:41:15AM -0000, Sebastien Bacher wrote:
> > Handle in what way? I guess a message will be delivered to u-s-s
> somehow?
>
> My understanding is that it's going to go through the normal mimetype
> handling mechanism, which means clicking on "settings://...." is going
> to call the default handle for settings: (which is u-s-s) "system-
> settings settings://system/sound" for example ... we should parse the
> "settings://system/<name>" argument as a request to open "<name>"

Why does it have to pass the whole string? The settings://system/ part
is only interesting to the URL dispatcher so it can find out what to
launch. It'd make the application code nicer if we only had to take the
information which is interesting to us - the name of the panel. As a
bonus we already handle this.

Not sure how it interacts with the single instance stuff though.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Thomas Strehl (strehl-t) wrote : Re: should support settings://system/<...> urls

Parsing out the panel name from the complete URL passed to settings app is not a big deal, in fact it's just one line of code using QUrl::path()

Revision history for this message
Sebastien Bacher (seb128) wrote :

@Thomas:
> Parsing out the panel name from the complete URL passed to settings app is not a big deal, in fact it's just one line of code using
> QUrl::path()

right, it wouldn't be a big deal also to call "system-settings <name>" which is already supported. The fact that it's little code doesn't make a convenient argument on why that syntax is better than the one we are currently using and which is working...

> Why does it have to pass the whole string? The settings://system/ part
> is only interesting to the URL dispatcher so it can find out what to
> launch. It'd make the application code nicer if we only had to take the
> information which is interesting to us - the name of the panel.

One advantage of supporting the full url scheme is that you could click on "system://..." urls in e.g an email or your web browser and have it working through the standard url handler mechanism (that takes the default app and give it the url as a parameter). Not sure that's a feature we want though, I'm not sure I want to make easy for spammer to bring users to system settings with a click for example...

Revision history for this message
Iain Lane (laney) wrote : Re: [Bug 1227690] Re: should support settings://system/<...> urls

On Wed, Sep 25, 2013 at 11:16:17AM -0000, Thomas Strehl wrote:
> Parsing out the panel name from the complete URL passed to settings app
> is not a big deal, in fact it's just one line of code using QUrl::path()

I'm sure, but what's the point? It's an implementation detail that the
application shouldn't need to know about. Seems like every application
which might be activated in this way will need to do it too.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Iain Lane (laney) wrote :

On Wed, Sep 25, 2013 at 11:23:44AM -0000, Sebastien Bacher wrote:
> […]
> > Why does it have to pass the whole string? The settings://system/ part
> > is only interesting to the URL dispatcher so it can find out what to
> > launch. It'd make the application code nicer if we only had to take the
> > information which is interesting to us - the name of the panel.
>
> One advantage of supporting the full url scheme is that you could click
> on "system://..." urls in e.g an email or your web browser and have it
> working through the standard url handler mechanism (that takes the
> default app and give it the url as a parameter). Not sure that's a
> feature we want though, I'm not sure I want to make easy for spammer to
> bring users to system settings with a click for example...

I see the benefits of having the URL scheme for launching one
application from another, I'm only saying that (it seems to me as if)
the backend should strip out the information that's not interesting to
the application being launched.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Alberto Mardegan (mardy) wrote : Re: should support settings://system/<...> urls

I think that the value is in that using URLs is easier than an array of strings.
However Seb raised a very valid point, about the risks of exposing System Settings via those URLs. Will the url-dispatcher implement some kind of ACL, to make sure that only whitelisted apps can request certain URLs?

Revision history for this message
Alberto Mardegan (mardy) wrote : Re: [Bug 1227690] Re: should support settings://system/<...> urls

On 09/25/2013 02:50 PM, Iain Lane wrote:
> I see the benefits of having the URL scheme for launching one
> application from another, I'm only saying that (it seems to me as if)
> the backend should strip out the information that's not interesting to
> the application being launched.

It could be interesting, if an application is handling more than one
scheme: in that case it really needs to see the complete URL.

Revision history for this message
Iain Lane (laney) wrote :

On Wed, Sep 25, 2013 at 12:08:31PM -0000, Alberto Mardegan wrote:
> On 09/25/2013 02:50 PM, Iain Lane wrote:
> > I see the benefits of having the URL scheme for launching one
> > application from another, I'm only saying that (it seems to me as if)
> > the backend should strip out the information that's not interesting to
> > the application being launched.
>
> It could be interesting, if an application is handling more than one
> scheme: in that case it really needs to see the complete URL.

In that case it'll need to know how to handle the multiple arguments
anyway, so you'd pass both.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message
Ted Gould (ted) wrote :

On Wed, 2013-09-25 at 10:59 +0000, Iain Lane wrote:

> Why does it have to pass the whole string? The settings://system/ part
> is only interesting to the URL dispatcher so it can find out what to
> launch. It'd make the application code nicer if we only had to take the
> information which is interesting to us - the name of the panel. As a
> bonus we already handle this.

Because the activation mechanisms only work with URIs, they don't pass
parameters. Parameters are only added on the Desktop File Exec line.
For most applications they need the full URI as it's a file or website.
System Settings really is the exception here.

In the future we could implement something like this with desktop
actions, but we're not going to support those in the 13.10 timeframe.
They are on my desired list for 14.04 though.

On Wed, 2013-09-25 at 10:59 +0000, Iain Lane wrote:

> Not sure how it interacts with the single instance stuff though.

They will work together. We expect a QML plugin to land tomorrow
that'll provide a signal of a new URL if the application is already
running. Upstart App Launch will also take care of discussing with
Unity to unsuspend any suspended applications as well and making sure
they get focus.

On Wed, 2013-09-25 at 11:53 +0000, Alberto Mardegan wrote:

> I think that the value is in that using URLs is easier than an array
> of strings.
> However Seb raised a very valid point, about the risks of exposing
> System Settings via those URLs. Will the url-dispatcher implement some
> kind of ACL, to make sure that only whitelisted apps can request
> certain URLs?

The URLs are only used to start a new application under confinement. So
that application would be brought to focus and shown to the user.
There's no way to do operations that return values or otherwise allow
for interaction with the application. Ubuntu Security is aware of this
and has signed off on the architecture.

It is important that application ensures that it handles the URL well.
Since it is a regular expression we can provide more protection to
system settings if that'd be useful. For instance only allowing
characters after the final "/".

Revision history for this message
Ted Gould (ted) wrote :

Changing the URL slightly because of this bug.

https://bugs.launchpad.net/url-dispatcher/+bug/1231444

summary: - should support settings://system/<...> urls
+ should support settings:///system/<...> urls
description: updated
Alberto Mardegan (mardy)
Changed in ubuntu-system-settings:
status: Triaged → In Progress
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:ubuntu-system-settings at revision 404, scheduled for release in ubuntu-system-settings, milestone Unknown

Changed in ubuntu-system-settings:
status: In Progress → Fix Committed
Revision history for this message
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package ubuntu-system-settings - 0.1+13.10.20131002-0ubuntu1

---------------
ubuntu-system-settings (0.1+13.10.20131002-0ubuntu1) saucy; urgency=low

  [ Sebastien Bacher ]
  * click: the installed-size info is in kib, not in bytes.

  [ Iain Lane ]
  * Use the indicator's value for the charge level instead of
    calculating it from QtSystems derived information.
  * [battery] Update the UI dynamically when the wireless state changes.
  * Various translation tweaks as noted in LP: #1233214. (LP: #1215712,
    #1233214)
  * [battery] Bind the onClicked handlers instead of checked to avoid
    loops when enabling/disabling wifi.
  * Remove one more missed translation.
  * Bump required version of ubuntu-keyboard-data, as we require the
    schemas for the language panel.
  * Remove the SettingsCheckEntry now that the design issues are
    resolved.

  [ Ted Gould ]
  * Adding a %u to the desktop file Exec line.

  [ William Hua ]
  * Hook up language panel to some maliit settings.

  [ Didier Roche ]
  * Use some async calls for system update where we don't expect any
    returned value.
  * Add an indeterminate progress bar until system image answers.

  [ Charles Kerr ]
  * Show an ActivityIndicator to tell users when discovery mode is on to
    search for new bluetooth devices.
  * add PIN code pairing. Implementation is similar to the existing
    passkey pairing.

  [ Ken VanDine ]
  * revert the previous revert done in rev 391, Bug (#LP: #1231368) was
    caused by gallery-app needing a rebuild for content-hub changes. .
    (LP: #1231368)

  [ Alberto Mardegan ]
  * Parse URLs given in command line. (LP: #1227690)

  [ Ubuntu daily release ]
  * Automatic snapshot from revision 419
 -- Ubuntu daily release <email address hidden> Wed, 02 Oct 2013 04:09:15 +0000

Changed in ubuntu-system-settings (Ubuntu):
status: Triaged → Fix Released
Changed in ubuntu-system-settings:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.