Created attachment 174696 [details] [review]
util: add Util.spawn, Util.killall, remove Shell.Process

A long time ago, gjs could handle NULL-terminated-string-array
properties, but not NULL-terminated-string-array function parameters.
So we created Shell.Process to work around this. But this was fixed a
long time ago, and we can just use GLib.spawn_async() directly now.

Additionally though, GLib.spawn_async() / Shell.Process will throw
errors if the command isn't found, and most places that used
Shell.Process weren't actually dealing with that possibility. Add a
new function, Util.spawn, that spawns a command line or argv, and pops
up an error message if it fails. (For now this uses zenity but
eventually it would use the shell dialog mechanism.)

Also, add Util.killall() since we're now killing processes from
several different places.

Review of attachment 174696 [details] [review]:

Generally, it looks good.
Personally though I would make Util.spawn and Util.spawn_command separate.
Type checking in JS is worse than C and method overloading, IMHO, should be avoided, if possible; also, it would make the function more difficult to learn by example (which is the only way extension writers have).
Or, instead, you could just remove the ability to specify a command line, and just use an array of arguments at all times. It would save an additional parsing of arguments, with potential quoting issues.

Also, the facility for reporting shell errors is there, somewhere on the overview-relayout branch, replacing the InfoBar with message tray notifications.

(In reply to comment #2)
> also, it would make the function more difficult to learn
> by example (which is the only way extension writers have).

the fix for "we have bad documentation" is "make better documentation".

> Or, instead, you could just remove the ability to specify a command line, and
> just use an array of arguments at all times. It would save an additional
> parsing of arguments, with potential quoting issues.

There are places where you want to call it with distinct arguments that may potentially have quoting issues (Util.spawn(['gnome-power-statistics', '--device', device_id]);) and other places where you want to call it with a command line that may make arbitrary use of quotes, etc (the run dialog). We could just fall back to using GLib.shell_parse_argv() again in the latter cases, but this just seemed more convenient to me...

> Also, the facility for reporting shell errors is there, somewhere on the
> overview-relayout branch, replacing the InfoBar with message tray
> notifications.

Hm... I meant the modal dialog stuff, although notifications would be better I guess. So maybe I'll let this linger until that lands.

(In reply to comment #3)
> (In reply to comment #2)
> > also, it would make the function more difficult to learn
> > by example (which is the only way extension writers have).
> 
> the fix for "we have bad documentation" is "make better documentation".

True.
My comment on Util.spawn vs Util.spawn_command still stands.

> > Or, instead, you could just remove the ability to specify a command line, and
> > just use an array of arguments at all times. It would save an additional
> > parsing of arguments, with potential quoting issues.
> 
> There are places where you want to call it with distinct arguments that may
> potentially have quoting issues (Util.spawn(['gnome-power-statistics',
> '--device', device_id]);) and other places where you want to call it with a
> command line that may make arbitrary use of quotes, etc (the run dialog). We
> could just fall back to using GLib.shell_parse_argv() again in the latter
> cases, but this just seemed more convenient to me...

Aren't we going through $SHELL in the run dialog?
(That is, /bin/sh -c "my --command")

> 
> > Also, the facility for reporting shell errors is there, somewhere on the
> > overview-relayout branch, replacing the InfoBar with message tray
> > notifications.
> 
> Hm... I meant the modal dialog stuff, although notifications would be better I
> guess. So maybe I'll let this linger until that lands.

Yeah, I guessed you meant those, but I personally would not like to see modal errors.

(In reply to comment #4)
>
> Aren't we going through $SHELL in the run dialog?
> (That is, /bin/sh -c "my --command")

I like this even better actually than having callers use g_shell_parse_argv.

Created attachment 175544 [details] [review]
messageTray: add SystemNotificationSource, moved from overview.js

Move the overview's "System Information" source here, so it can be
used by non-overview code as well.

Created attachment 175547 [details] [review]
util: rename from utils, avoid RegExp literal

since my original patch (?) we now have js/misc/utils.js. It didn't make sense to have both misc.utils and ui.util, so I figured I should merge them. "util" seemed like the right name, and misc like the right location.

Created attachment 175549 [details] [review]
util: add Util.spawn, Util.killall, remove Shell.Process

now with separate spawn and spawnCommandLine.

I didn't change anything to use "sh -c". That could happen separately.

Review of attachment 175544 [details] [review]:

Definitely looks good, except for a minor point.
If this gets pushed fast enough, I'll use it for the bluetooth indicator (not the agent part, only the error reporting for missing obex:// handler)

::: js/ui/messageTray.js
@@ +1745,3 @@
+            Mainloop.source_remove(this._timeoutId);
+        this._timeoutId = Mainloop.timeout_add_seconds(SYSTEM_NOTIFICATION_HIDE_TIMEOUT,
+                                                       Lang.bind(this, this._onTimeout));

Can't you just bind this.destroy?
It is not as if one would override .destroy after having called .notify, but before the timeout is triggered.

Review of attachment 175549 [details] [review]:

::: js/ui/notificationDaemon.js
@@ +129,3 @@
             log('Failed to acquire org.freedesktop.Notifications; trying again');
+            Util.killall('notification-daemon');
+            Util.killall('notification-osd');

that should be notify-osd

::: js/ui/status/accessibility.js
@@ +110,3 @@
         this.menu.addMenuItem(new PopupMenu.PopupSeparatorMenuItem());
         this.menu.addAction(_("Universal Access Settings"), function() {
+            Util.spawn(['gnome-control-center universal-access']);

['gnome-control-center', 'universal-access']
(or it will look for "gnome-control-center\ universal-access")

How does this relate to https://bugzilla.gnome.org/show_bug.cgi?id=637061 and the issue of wanting correct focus-stealing-prevention timestamps?

(In reply to comment #11)
> How does this relate to https://bugzilla.gnome.org/show_bug.cgi?id=637061 and
> the issue of wanting correct focus-stealing-prevention timestamps?

Right. It doesn't.

IIRC, gnome-panel's run dialog actually matches $ARGV[0] of the command line you give it to .desktop file Exec lines. So we should probably do the same thing, in which case the Util.spawn/spawnCommandLine API would not need to change, but it would transparently switch to using GAppInfo where appropriate? (This would require having a binary-name-to-desktop-file-name cache, which if we don't already have we could probably make use of in the window-tracking stuff anyway...)

Created attachment 176618 [details] [review]
shell-app-system.h: clean up (indentation, order, etc)

Created attachment 176619 [details] [review]
ShellAppSystem: add shell_app_system_find_executable()

Cache the executable names from desktop file Exec lines, and allow
finding a ShellAppInfo based on that

Created attachment 176620 [details] [review]
ShellApp: is_on_workspace() should be TRUE for workspaceless apps

If a starting-up app has not requested a particular workspace, then
shell_app_is_on_workspace() should return TRUE for any workspace.

Otherwise we will never get startup notification for them, since the
app menu only shows apps that are starting on the current workspace.

Created attachment 176621 [details] [review]
messageTray: add SystemNotificationSource, moved from overview.js

Move the overview's "System Information" source here, so it can be
used by non-overview code as well.

Created attachment 176622 [details] [review]
util: rename from utils, avoid RegExp literal

Rename imports.misc.utils to imports.misc.util for more consistency
(eg, with shell-util).

Also, use the string-based RegExp() constructor rather than a RegExp
literal, since the literal is extremely difficult to parse correctly,
and confuses emacs and probably other editors and thus messes up
autoindentation, etc.

Created attachment 176623 [details] [review]
util: add Util.spawn and friends, remove Shell.Process

Remove Shell.Process, which existed to work around gjs bugs that have
long since been fixed.

Add Util.spawn and Util.spawnCommandLine, for spawning a command in
the background, automatically handling errors via
MessageTray.SystemNotificationSource(), and Util.trySpawn and
Util.trySpawnCommandLine that don't do automatic error handling (but
do at least clean up the error message in the exception a bit).

In all cases, if the provided command refers to a known application,
set up a GdkApplicationLaunchContext, so that we get startup
notification and we don't get incorrect focus-stealing-prevention.

Update various other bits of code around the shell to use the new
methods.

Created attachment 176624 [details] [review]
Util.killall: add utility for killing unwanted processes

Since we have to use pkill, kludgily, for the right combination of
portability and featurefulness, put the code in one place rather than
duplicating it everywhere.

not totally sure about the division of labor in the spawn() patch, and there's still a FIXME there pointing out that we should be checking for StartupNotify=true in the .desktop file, but we aren't (mostly because we can't from JS since GDesktopFile isn't boxed). But maybe more of that code should go into ShellAppInfo anyway or something?

Created attachment 177980 [details] [review]
messageTray: add SystemNotificationSource, moved from overview.js

Move the overview's "System Information" source here, so it can be
used by non-overview code as well.

Created attachment 177981 [details] [review]
util: rename from utils, avoid RegExp literal

Rename imports.misc.utils to imports.misc.util for more consistency
(eg, with shell-util).

Also, use the string-based RegExp() constructor rather than a RegExp
literal, since the literal is extremely difficult to parse correctly,
and confuses emacs and probably other editors and thus messes up
autoindentation, etc.

Created attachment 177982 [details] [review]
util: add Util.spawn and friends

The new version of this patch does not try to do startup notification
from Util.spawn(). Instead, it adds a new Util.spawnDesktop() that takes
a desktop file ID and spawns that via ShellAppSystem. For cases where
we don't have a .desktop file, we still use Util.spawn(), and don't get
startup notification, but that's fine, because those are mostly
non-app-like use cases anyway (eg, gnome-screensaver-command).

Created attachment 177983 [details] [review]
Util.killall: add utility for killing unwanted processes

Since we have to use pkill, kludgily, for the right combination of
portability and featurefulness, put the code in one place rather than
duplicating it everywhere.

Created attachment 177984 [details] [review]
shell: remove ShellProcess

ShellProcess only existed to work around gjs bugs that have long since
been fixed, and has now been obsoleted by Util.spawn*. Kill it.

bug 639180 is needed to prevent a slight change of behavior with the user menu's "System Preferences" item.

The two shell-app-system patches are no longer prereqs for the rest of the series, but the first one is a good cleanup anyway, and the second one is correct anyway.

Review of attachment 176618 [details] [review]:

Assume it's OK

Review of attachment 176620 [details] [review]:

Looks good

Review of attachment 177980 [details] [review]:

Looks fine

Review of attachment 177981 [details] [review]:

I'm not aware of anything *other* than js2-mode that gets confused by RegExp literals, so my take is sort of "get a better emacs mode", but js2-mode does have some features (like parsing and detecting errors) that go well beyond what is otherwise available so being friendly to it is probably good. There are, of course, some cases, where the literal is quite different than a RegExp object - because it is (afaik) only compiled once at parse time,  but here the difference is unimportant.

Rest of patch looks fine - I have no opinion on whether "util" or "utils" is better but certainly being consistent is better than being inconsistent.

Review of attachment 177982 [details] [review]:

Looks like a good cleanup to me, and don't see anything but trivial issues.

::: js/misc/util.js
@@ +6,3 @@
+const Shell = imports.gi.Shell;
+
+const Main = imports.ui.main;

Somehow misc importing ui seems like encapsulation breakage, but I guess it's OK if we consider misc to be stuff that doesn't have UI in the interfaces rather than stuff that doesn't touch the UI at all.

@@ +33,3 @@
+// @argv: an argv array
+//
+// Runs @argv in the background, handling basic errors

I really don't see anything "basic" about the errors it handles - it seems to handle all errors - "handling errors basically" ?

@@ +101,3 @@
+        [success, argc, argv] = GLib.shell_parse_argv(command_line);
+    } catch (err) {
+        err.message = err.message.replace(/[^:]*: /, _("Could not parse command:") + "\n");

This seems as needful of a detailed explanation as the above - maybe needs a See trySpawn() as below, though it would only refer to the Error invoking ...: part and not the parentheses part. (Had to try an example to figure it out

 Error invoking GLib.shell_parse_argv: Text ended before matching quote was found for ". (The text was '"')

)

@@ +127,3 @@
+
+    try {
+        app.launch();

Hmm, shouldn't shell_app_info_launch_full() use shell_global_get_current_time() not clutter_get_current_event_time()?

@@ +141,3 @@
+    Main.messageTray.add(source);
+    let notification = new MessageTray.Notification(source, title, err.message);
+    notification.setTransient(true);

is transient right for this? I thought transient was mostly for information that the user could get to by other means.

Review of attachment 177983 [details] [review]:

Look fine, one question

::: js/misc/util.js
@@ +159,3 @@
+        // whatever...
+
+        let argv = ['pkill', '-f', '^([^ ]*/)?' + processName + '$'];

Are we worried about processes with command line arguments that the trailing $ would make us not match?

Review of attachment 177984 [details] [review]:

Good to commit once users are gone

(In reply to comment #31)
> Somehow misc importing ui seems like encapsulation breakage, but I guess it's
> OK if we consider misc to be stuff that doesn't have UI in the interfaces
> rather than stuff that doesn't touch the UI at all.

Yeah. Or we could just get rid of the misc/ui split and put everything directly under js/...

> +// Runs @argv in the background, handling basic errors
> 
> I really don't see anything "basic" about the errors it handles - it seems to
> handle all errors - "handling errors basically" ?

Not sure why I thought "basic errors" was a good description. What I meant is that it handles errors from GLib.spawn*, but not anything that happens after that (eg, program crashes immediately after successfully launching).

> Hmm, shouldn't shell_app_info_launch_full() use shell_global_get_current_time()
> not clutter_get_current_event_time()?

Yes

> is transient right for this? I thought transient was mostly for information
> that the user could get to by other means.

eef194c3: "According to the designers, system notifications should be transient".

(In reply to comment #32)
> +        let argv = ['pkill', '-f', '^([^ ]*/)?' + processName + '$'];
> 
> Are we worried about processes with command line arguments that the trailing $
> would make us not match?

for our current cases it doesn't matter... but it seems like changing '$' to '($| )' would do the trick? (processName must be followed by either a space or the end of the string)

pushed with fixes

Attachment 177980 [details] pushed as 7322a4e - messageTray: add SystemNotificationSource, moved from overview.js
Attachment 177981 [details] pushed as a65a0f0 - util: rename from utils, avoid RegExp literal
Attachment 177982 [details] pushed as 8bdfb8d - util: add Util.spawn and friends
Attachment 177983 [details] pushed as 23353fb - Util.killall: add utility for killing unwanted processes
Attachment 177984 [details] pushed as 9ddf19a - shell: remove ShellProcess

Attachment 176618 [details] pushed as b919dd7 - shell-app-system.h: clean up (indentation, order, etc)
Attachment 176620 [details] pushed as 99a865f - ShellApp: is_on_workspace() should be TRUE for workspaceless apps