Created attachment 313725 [details] [review]
shell-app: don't use new window for app actions

We were assuming that if a GApplication implements a GAction called
"new-window" it can manage new windows.
Probably that's true. However, instead of activating this action
through the dbus method, we just activated the application, which
in case of GApplications I would say most of the times means present
the current window, instead of creating a new window.
Gnome shell was activating the application instead of calling the dbus
method supposedly because needs the start notification.
In conclusion this guessing can be more confusing than anything, and
instead applications can make accessible explicit actions through the
desktop file.

So instead of adding a new dash menu entry with a "New Window" operation
when the application implements a new-window GAction, check its desktop
file actions and call them directly.

Note that in the patch we check for the desktop file actions to determine
if it can manage new windows and then in AppDisplay we check for it not
being there, so in the current state of Gnome Shell this code is useless,
however it keeps the logic.

Review of attachment 313725 [details] [review]:

forgot to check for strv == NULL, I will update patch in a few hours.

Created attachment 313727 [details] [review]
shell-app: don't use new window for app actions

We were assuming that if a GApplication implements a GAction called
"new-window" it can manage new windows.
Probably that's true. However, instead of activating this action
through the dbus method, we just activated the application, which
in case of GApplications I would say most of the times means present
the current window, instead of creating a new window.
Gnome shell was activating the application instead of calling the dbus
method supposedly because needs the start notification.
In conclusion this guessing can be more confusing than anything, and
instead applications can make accessible explicit actions through the
desktop file.

So instead of adding a new dash menu entry with a "New Window" operation
when the application implements a new-window GAction, check its desktop
file actions and call them directly.

Note that in the patch we check for the desktop file actions to determine
if it can manage new windows and then in AppDisplay we check for it not
being there, so in the current state of Gnome Shell this code is useless,
however it keeps the logic.

Review of attachment 313727 [details] [review]:

Why not have it activate the new-action GAction in that case?

(In reply to Jasper St. Pierre from comment #4)
> Review of attachment 313727 [details] [review] [review]:
> 
> Why not have it activate the new-action GAction in that case?

https://git.gnome.org/browse/gnome-shell/tree/src/shell-app.c#n554

I put my thought about that in the commit message. Not sure that comment is longer valid tbh...

ActivateAction is definitely allowed to have startup-notification stuff go through, I think. I know there's a platform_data in there precisely for that reason.

You might have to use the _TIME hack though.

(In reply to Jasper St. Pierre from comment #6)
> ActivateAction is definitely allowed to have startup-notification stuff go
> through, I think. I know there's a platform_data in there precisely for that
> reason.
> 
> You might have to use the _TIME hack though.

Yes, but you and Ryan NAK'd the patch to support that, back when that code was added.

As for the logic itself, I think it should be reversed.
can_open_new_window() means "If I launch the app again, a new window will appear".
If we assume that a desktop action called new-window means that fdo.Application.Activate() calls present(), then can_open_new_window() should return false, and the "New Window" menu item will be provided by the desktop actions code. Because in theory can_open_new_window() should be true if and only if open_new_window() will open a new window, and if new-window is there, that's most likely not to be the case.
Alternatively, new-window should be hooked up to open_new_window(), which would fix the "drag icon to workspace to start app" - but that would require a lot more startup notification than just _TIME to handle workspaces properly.
(And ideally we should rely less on x11-only startup notification and more on new mutter API for the apps that we start ourselves, but nobody is writing that)

Oh, another thing to mention:

The logic in can_open_new_window() says that running GApplications without a new-window can't open a new window - this is to handle single window apps like gnome-documents or gnome-photos.
But with your patch, all multi window GApplications that don't have a new-window desktop action (because they DTRT in Activate()) will also fall in that case, unless they override it with desktop file key or add a new-window desktop action and associated command line option. This seems suboptimal to me.

(In reply to Giovanni Campagna from comment #7)
> As for the logic itself, I think it should be reversed.
> can_open_new_window() means "If I launch the app again, a new window will
> appear".

This is not what the name implies though, so if we go with this, we should rename it (activate_opens_new_window() or something). However IMHO "this app can open a new window by this particular mean" doesn't sound like something we should expose users to.


> Alternatively, new-window should be hooked up to open_new_window()

I think this is the best option for context menu and modifier-click. A 'new-window' action is a clear sign that the app can open new windows, and users can reasonably expect those options to be available in that case - it's unlikely for an app to have the action and not expose it anywhere in the UI.

However DND is indeed an issue (though I'm pretty sure I've seen it break with multi-window apps' Activate() as well). If we go that route anyway, we should probably exclude apps whose 'new-window' action is not parameterless - I haven't seen any apps doing that, but an unfortunately named "Open in new window" context menu action isn't unthinkable.

*** Bug 759881 has been marked as a duplicate of this bug. ***

Created attachment 336099 [details] [review]
app: Consider "new-window" action for opening new windows

While can_open_new_window() uses some elaborate heuristics to predict
whether an application can open multiple windows, open_new_window()
will always simply relaunch the application. This is often the best
we can do, but when an application provides a "new-window" action in
its .desktop file or on the bus, it is much more likely to work as
expected than blindly activating the app and hoping for a particular
behavior.

Review of attachment 336099 [details] [review]:

::: src/shell-app.c
@@ +567,3 @@
+   * that we can activate on the bus - the muxer will add startup notification
+   * information to the platform data, so this should work just as well as
+   * desktop actions.

Except it won't.

It will add timestamp, but it will not trigger full startup notification, and therefore it will lose the workspace information.

(In reply to Giovanni Campagna from comment #12)
> It will add timestamp, but it will not trigger full startup notification,
> and therefore it will lose the workspace information.

Mmmh, right. Not sure I care too much to be honest though - opening a new window on a particular workspace already breaks more often than not, and it seems like the lesser evil when compared to not actually opening a new window at all. In particular as dragging a launcher to a workspace is a relatively obscure feature when compared to a context menu (ctrl- and middle-click are admittedly fairly obscure as well though).

It would also be missing the feedback (mouse cursor, app menu spinner) part of startup notification though.

(In reply to Giovanni Campagna from comment #14)
> It would also be missing the feedback (mouse cursor, app menu spinner) part
> of startup notification though.

The same as if the user creates a new window inside the app no? So that wouldn't be a major problem I think.

I think either we just don't assume there is a new window if the desktop action is not explicitly set breaking most of current applications, or we do this...

(In reply to Carlos Soriano from comment #15)
> (In reply to Giovanni Campagna from comment #14)
> > It would also be missing the feedback (mouse cursor, app menu spinner) part
> > of startup notification though.
> 
> The same as if the user creates a new window inside the app no? So that
> wouldn't be a major problem I think.
> 
> I think either we just don't assume there is a new window if the desktop
> action is not explicitly set breaking most of current applications, or we do
> this...

Well why are multi-window applications not opening a new window when Activate()'d?

The only one I know broken is evolution, but others (most importantly browsers and terminals) should work fine.

No wait. I checked and gedit is broken too, because it opens a new tab instead of a new window.

But both gedit and evolution have a New Window desktop action, so the brokenness does not apply to them.

Nautilus. We have a --new-window command line for that. Usually we always want to focus the window that has a directory opened rather than always opening a new window. We only open a new window if the directory is not opened in any window, and, if explicitly set with --new-window or with the new-window action in the desktop file.

I honestly don't know why we should open a new window in activate(), if we already can set desktop actions that explicitly do that, no?

(In reply to Carlos Soriano from comment #17)
> I honestly don't know why we should open a new window in activate(), if we
> already can set desktop actions that explicitly do that, no?

Yeah, my point is more about apps that have a new-window action when running, but not a new-window action in the desktop files, because those would follow the path with no startup notification.

(In reply to Giovanni Campagna from comment #14)
> It would also be missing the feedback (mouse cursor, app menu spinner) part
> of startup notification though.

There is no app menu spinner when the app is already running, no matter what method is used to open a new window.


(In reply to Giovanni Campagna from comment #18)
> Yeah, my point is more about apps that have a new-window action when
> running, but not a new-window action in the desktop files, because those
> would follow the path with no startup notification.

Right now the 'new-window' action is always ignored though by anything that calls open_new_window() instead of explicitly using the desktop action. We should at the very least fix that.

I still think that consistency (app menu -> "New window": opens a new window; context menu -> "New window": maybe opens a new window, who knows?) is more important than possible glitches. We can recommend that apps with a 'new-window' action expose it in their .desktop file in the short-term (and should probably do that independent from the outcome of this discussion). In the long-term, we should really try to make actions work nicer with startup notifications, otherwise we'll always end up with cases that don't work as expected and that cannot be worked around by making assumptions in gnome-shell (like opening a window from an action in the app menu or a notification button)

Is there something that prevent to commit the patch ?

See previous comments (such as comment 12 and onwards).

*** Bug 788406 has been marked as a duplicate of this bug. ***

(In reply to Florian Müllner from comment #13)
> (In reply to Giovanni Campagna from comment #12)
> > It will add timestamp, but it will not trigger full startup notification,
> > and therefore it will lose the workspace information.
> 
> Mmmh, right.

Actually, calling Activate on a remote instance adds the exact same platform data as calling ActivateAction. That is, the workspace information is already ignored for running (GtkApplication) apps unless I'm overlooking something - if that's the case, then the patch doesn't actually regress anything, it just doesn't fix that particular issue.

For the record, the patch from comment #11 has been republished, reviewed, and merged in https://gitlab.gnome.org/GNOME/gnome-shell/merge_requests/44

I'm not sure if this should remain open because of the startup-notification concerns

Right, this bug should have been closed then - thanks for the reminder.