That's weird. The search result should directly take you to the terminal in question, irrespective of the workspace it is on.

(In reply to comment #0)
> Not sure if this is a shell's bug of terminal's

Definitively a shell bug - the terminal search provider correctly uses the timestamp we pass to ActivateResult, so if focus stealing prevention kicks in anyway, our timestamp is wrong.

(In reply to comment #2)
> if focus stealing prevention kicks in anyway, our timestamp is wrong.

Actually no - that's how our focus stealing prevention is supposed to work. It is the same issue as clicking a link triggering a notification when it opens in a browser window on a different workspace.
I tend to think that our focus stealing prevention kicks in too often, so I'm all for tweaking it, but it's probably best to keep it in 3.13.x so it gets its fair amount of testing. As annoying as the "wrong" notifications are, unexpected focus changes are worse.
(I'll finish a rough patch when I get to the office)

Created attachment 274108 [details] [review]
window: Allow activation on non-active workspaces with proper timestamps

Our focus stealing prevention is still mostly inherited from metacity;
in particular, a (non-transient) window that is not on the current
workspace will not be given focus. This behavior made sense in the
GNOME 2 days, where workspaces were separated much more strictly.
However this is no longer the case in GNOME 3 - activating a launcher
will switch workspaces if necessary, and so will the app switcher.
There is no good reason to not do the same for other user actions
like clicking a URL or activating a search result, so allow activation
of windows on non-active workspaces if a proper timestamp is supplied,
assuming that this is a strong enough indication that we are dealing
with a legitimate user action.

Review of attachment 274108 [details] [review]:

Makes sense to me ... as for the implementation it can be simplified by removing the dead code around it.

::: src/core/window.c
@@ +3719,3 @@
    */
   can_ignore_outdated_timestamps =
     (timestamp != 0 || (FALSE && source_indication != META_CLIENT_TYPE_PAGER));

Uh this looks kind of pointless it is effectively 'timestamp !=0' ...

@@ +3720,3 @@
   can_ignore_outdated_timestamps =
     (timestamp != 0 || (FALSE && source_indication != META_CLIENT_TYPE_PAGER));
+  allow_workspace_switch = timestamp != 0;

So just remove this and fix the above conditional instead.

Created attachment 274119 [details] [review]
window: Treat CurrentTime as legal timestamp in activation

Effectively we have been accepting CurrentTime timestamps for years,
but still complained about "stupid pagers" when encountering them;
just accept that we will never limit treating 0 timestamps as current
time to pagers.


(In reply to comment #5)
>    can_ignore_outdated_timestamps =
>      (timestamp != 0 || (FALSE && source_indication !=
> META_CLIENT_TYPE_PAGER));
> 
> Uh this looks kind of pointless it is effectively 'timestamp !=0' ...

The corresponding lengthy comment[0] suggests some history there ("We'll fight that battle later"). If we decide that we won't ever remove the FALSE to get back the original behavior, then I'm fine with it, but not buried in an otherwise unrelated patch.

[0] https://git.gnome.org/browse/mutter/tree/src/core/window.c#n3712

Review of attachment 274119 [details] [review]:

OK.

(In reply to comment #6)
> Created an attachment (id=274119) [details] [review]
> window: Treat CurrentTime as legal timestamp in activation
> 
> Effectively we have been accepting CurrentTime timestamps for years,
> but still complained about "stupid pagers" when encountering them;
> just accept that we will never limit treating 0 timestamps as current
> time to pagers.
> 
> 
> (In reply to comment #5)
> >    can_ignore_outdated_timestamps =
> >      (timestamp != 0 || (FALSE && source_indication !=
> > META_CLIENT_TYPE_PAGER));
> > 
> > Uh this looks kind of pointless it is effectively 'timestamp !=0' ...
> 
> The corresponding lengthy comment[0] suggests some history there ("We'll fight
> that battle later"). If we decide that we won't ever remove the FALSE to get
> back the original behavior, then I'm fine with it, but not buried in an
> otherwise unrelated patch.

Yeah well no one complained so far so its fine with just removing it.

Created attachment 274121 [details] [review]
window: Allow activation on non-active workspaces with proper timestamps

(In reply to comment #5)
> +  allow_workspace_switch = timestamp != 0;
> 
> So just remove this and fix the above conditional instead.

I still prefer separate variable names - "can_ignore_outdated_timestamps" == "we may still activate the window even when last_user_time is newer", which doesn't really suggest any relation to whether or not workspace switches should be allowed.
But maybe the variable doesn't make sense anymore anyway and we can just use "timestamp != 0" directly in the check ...
(Also: if we now consider 0 a legal timestamp, maybe we shouldn't limit workspace switches at all to non-zero timestamps?)

(In reply to comment #9)
> Created an attachment (id=274121) [details] [review]
> window: Allow activation on non-active workspaces with proper timestamps
> 
> (In reply to comment #5)
> > +  allow_workspace_switch = timestamp != 0;
> > 
> > So just remove this and fix the above conditional instead.
> 
> I still prefer separate variable names - "can_ignore_outdated_timestamps" ==
> "we may still activate the window even when last_user_time is newer", which
> doesn't really suggest any relation to whether or not workspace switches should
> be allowed.
> But maybe the variable doesn't make sense anymore anyway and we can just use
> "timestamp != 0" directly in the check ..

Yeah the can_ignore_outdated_timestamps does not seem to make any sense now.
.
> (Also: if we now consider 0 a legal timestamp, maybe we shouldn't limit
> workspace switches at all to non-zero timestamps?)

Not sure about this but you have a point here.

Created attachment 274131 [details] [review]
window: Treat CurrentTime as legal timestamp in activation

(In reply to comment #10)
> > But maybe the variable doesn't make sense anymore anyway and we can just use
> > "timestamp != 0" directly in the check ..
> 
> Yeah the can_ignore_outdated_timestamps does not seem to make any sense now.

Killed.

Created attachment 274132 [details] [review]
window: Allow activation on non-active workspaces with proper timestamps

Rebased on top of attachment 274131 [details] [review].

(In reply to comment #10)
> (In reply to comment #9)
> > (Also: if we now consider 0 a legal timestamp, maybe we shouldn't limit
> > workspace switches at all to non-zero timestamps?)
> 
> Not sure about this but you have a point here.

Well, if I were sure, I wouldn't have written the patch with the non-zero timestamp limitation in the first place :-)

Still, I've had some time thinking about that on my way home: if an application does something truly stupid ("Ah, this four-hour download finished, let's pop up in the user's face!"), is steal-my-current-focus-and-switch-workspace really that much worse than steal-my-current-focus? The latter is already possible (and has for years) when the window happens to be on the current workspace; yet I'm not aware of any complaints about windows popping out of nowhere (and if apps really want to game us, a timestamp of G_MAXUINT32 instead of 0 should do the trick ...).
Incidentally, removing the restriction would fix an annoying issue with notification actions that activate a window on a different workspace:
notification: "Something interesting in FooApp" <click> notification: "FooApp is ready" <click> => switch to FooApp

Created attachment 274133 [details] [review]
window: Don't restrict workspace switches to non-zero timestamps

In case we do want to drop that limitation, that's what I'd squash with attachment 274132 [details] [review].

* Switching desktop on activation with a given and valid user timestamp seems like an OK risk to take.

* Removing the warning seems OK to me - it's not like it's making anybody fix their code to warn into Mutter's debug output.

* Switching desktops on CurrentTime is something that I don't like at all. Switching desktops on the user is a highly disorienting thing. If it was not a user action we *MUST NOT* switch desktops. If the action doesn't happen exactly when the user does it we *MUST NOT* switch desktops. It's definitely worse than focus stealing. If someone can't be bothered to give us a proper timestamp so we can tell that it was a user action and when it happened, we *SHOULD DEFINITELY NOT* switch desktops.

Review of attachment 274133 [details] [review]:

OK, we don't want this see Owen's comment.

Review of attachment 274131 [details] [review]:

OK.

Review of attachment 274132 [details] [review]:

OK.

(In reply to comment #15)
> If someone can't be bothered to give us a proper timestamp

I was more worried about user actions based on GAction/GApplication, where there is no associated timestamp (notification actions, dash jumplists). However looking at bug 632213, it looks like apps should still be able to get a timestamp in those cases (and if not, it's something that could still be fixed in GIO/GTK+ I guess) ...

Attachment 274131 [details] pushed as 5defe57 - window: Treat CurrentTime as legal timestamp in activation
Attachment 274132 [details] pushed as 87bec99 - window: Allow activation on non-active workspaces with proper timestamps

With this change, focus can be assigned to windows on non-visible workspace, essentialy stealing keystrokes and confusing the user. I've described two such scenarios in bug 729657