Merge lp://qastaging/~andersk/unity-lens-applications/lp778202 into lp://qastaging/unity-lens-applications

Proposed by Anders Kaseorg
Status: Rejected
Rejected by: Didier Roche-Tolomelli
Proposed branch: lp://qastaging/~andersk/unity-lens-applications/lp778202
Merge into: lp://qastaging/unity-lens-applications
Diff against target: 17 lines (+4/-3)
1 file modified
src/daemon.vala (+4/-3)
To merge this branch: bzr merge lp://qastaging/~andersk/unity-lens-applications/lp778202
Reviewer Review Type Date Requested Status
Didier Roche-Tolomelli Abstain
Jason Smith Pending
Review via email: mp+60135@code.qastaging.launchpad.net
To post a comment you must log in.
Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

Thanks Anders for your patch and your try to fix this.
However, we don't run to spaw a shell to launch everything from alt + F2 as we are then loosing some nice capability like GIO integration and perfect and reliably matching of the application in the launcher.

What I would suggest (slightly more complicated feature) is a "Run in terminal" checkbox which can help to get there and interpret that.

Thanks a lot for your contribution. Do not hesitate to hop on #ayatana on the freenode channel to get some help about it. (not that and next week however, as most of the unity team is travelling at UDS right now).

review: Needs Fixing
Revision history for this message
Anders Kaseorg (andersk) wrote :

Didier: I think you misunderstand the patch. sh is not a terminal. I’m only using sh -c to interpret the command string correctly, so that you can use, a quoted argument containing spaces (see bug #778202). This is the same way that system() and popen() interpret the command string.

This should have no effect on GIO integration or application matching. The patch only modifies the array passed to Process.spawn_async; no other state is affected.

Revision history for this message
Anders Kaseorg (andersk) wrote :

Oops, by removing exec_or_dir I broke Alt+F2 with a directory name. Fixed now.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

@Anders: Totally agree, but you spaw a shell which will confuse bamf (we really need to use gio to launch the application directly to get a perfect match between the application launched and the launcher).

Launching it in dash (default shell in ubuntu) will just add on level of indirection (sh will be the parent application for every applications you launch from alt + F2 and the dash).

I think the correct solution is rather having a look at Process.spawn_async. There should be a FLAG to interpret those characters. What do you think about it?

Revision history for this message
Anders Kaseorg (andersk) wrote :

I haven’t seen any such problems when testing my patch. For example, I can do Alt+F2 gcalctool, and I get a gcalctool that’s a child of sh -c gcalctool that’s a child of /sbin/init. The appropriate icon shows up in the launcher. I can middle-click the icon to launch another gcalctool. The new one is a direct child of /sbin/init. The two gcalctools are grouped in the same launcher icon, and clicking the icon raises both of them, etc. What else should I be testing?

Also, if BAMF gets confused by init → sh → gcalctool, why wouldn’t it be confused by init → gnome-terminal → bash → gcalctool, or by any of the myriad other applications that launch other applications via sh? If so, is there a bug report for this?

None of the spawn_async flags are relevant. There is a spawn_command_line_async that takes a string and maybe interprets shell quotes correctly, but it doesn’t accept a working_directory, so that would regress bug #736471. There is GLib.Shell.parse_argv, which does only the parsing part of spawn_command_line_async, but still it only supports a small subset of shell functionality (for example, you couldn’t Alt+F2 ‘sleep 5; do something else’).

Revision history for this message
Anders Kaseorg (andersk) wrote :

> (we
> really need to use gio to launch the application directly to get a perfect
> match between the application launched and the launcher).

I think this isn’t a concern because the patch only affects applications launched directly using Process.spawn_async. It does not affect any applications launched using GIO.

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

I'm request Jason's view on bamf, but I think you just trigger by that the fallback of the bamf integration, which is really awesome (we didn't see for a while in both maverick and natty that for a long period of time, we were in the fallback mode as Jason has done a perfect job on that), but with trickier application like java, python and such, this becomes untrue.

I really prefer the "GLib.Shell.parse_arg" path if you can implement that than the hack of sh -c. Seems more robust and clean code-wise.

For others cases like ‘sleep 5; do something else’ I think that "run in a terminal" checkbox is a better way to fix that problem as it's clearly explicit. I'm not even sure that gnome-panel alt + F2 supported that in fact.

I'm adding Jason to this review so that he can help on the bamf side.

Thanks for your further investigation :)

Revision history for this message
Didier Roche-Tolomelli (didrocks) wrote :

I think this is too old to enter anyway ;) Cleaning up MP list

Revision history for this message
Didier Roche-Tolomelli (didrocks) :
review: Abstain

Unmerged revisions

208. By Anders Kaseorg

Interpret command lines using the shell instead of splitting them on spaces

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches