Created attachment 191576 [details] [review]
Add a FocusApp method to org.gnome.Shell

This method, which accepts a .desktop filename, is used to highlight
a specific application in the overview, for example because it has
just been created or installed.

Review of attachment 191576 [details] [review]:

Looks good, still some minor issues though.

::: js/ui/appDisplay.js
@@ +168,3 @@
+    _selectPendingApp: function(closure) {
+        for (let i = closure.idStart; i < this._apps.length; i++) {
+            if (this._apps[i]._appInfo.get_id() == closure.appId) {

That's ugly we should make that public or add a getter (yes it is used somewhere else like that to but accessing a private property like that is still ugly).

::: js/ui/shellDBus.js
@@ +61,3 @@
     },
 
+    FocusApp: function(id, sync) {

Should add a doc comment before the method.
And do we really need the sync?

(In reply to comment #2)
> Review of attachment 191576 [details] [review]:
> 
> Looks good, still some minor issues though.
> 
> ::: js/ui/appDisplay.js
> @@ +168,3 @@
> +    _selectPendingApp: function(closure) {
> +        for (let i = closure.idStart; i < this._apps.length; i++) {
> +            if (this._apps[i]._appInfo.get_id() == closure.appId) {
> 
> That's ugly we should make that public or add a getter (yes it is used
> somewhere else like that to but accessing a private property like that is still
> ugly).

this._apps[i] is an object created by AlphabeticalView, which also sets _appInfo, so I think it is fine using it.

> ::: js/ui/shellDBus.js
> @@ +61,3 @@
>      },
> 
> +    FocusApp: function(id, sync) {
> 
> Should add a doc comment before the method.
> And do we really need the sync?

I added it for cases where inotify is not fast enough, as the focused application may not exist yet. It is true that this can cause two refreshes in rapid succession, which is not ideal (but since most of time is actually spent loading the icons, it could go unnoticed).

(In reply to comment #3)
> (In reply to comment #2)
> > Review of attachment 191576 [details] [review] [details]:
> > 
> > Looks good, still some minor issues though.
> > 
> > ::: js/ui/appDisplay.js
> > @@ +168,3 @@
> > +    _selectPendingApp: function(closure) {
> > +        for (let i = closure.idStart; i < this._apps.length; i++) {
> > +            if (this._apps[i]._appInfo.get_id() == closure.appId) {
> > 
> > That's ugly we should make that public or add a getter (yes it is used
> > somewhere else like that to but accessing a private property like that is still
> > ugly).
> 
> this._apps[i] is an object created by AlphabeticalView, which also sets
> _appInfo, so I think it is fine using it.

OK, fair enough.

> > ::: js/ui/shellDBus.js
> > @@ +61,3 @@
> >      },
> > 
> > +    FocusApp: function(id, sync) {
> > 
> > Should add a doc comment before the method.
> > And do we really need the sync?
> 
> I added it for cases where inotify is not fast enough, as the focused
> application may not exist yet. It is true that this can cause two refreshes in
> rapid succession, which is not ideal (but since most of time is actually spent
> loading the icons, it could go unnoticed).

Do you have specific examples of that "inotify being not fast enough" ? But yeah it shouldn't really matter performance wise.

So, played with this in ephy:

- The basics seem to work OK. I can call the method and the shell jumps to the app overview.
- It does not focus the new launcher though, just sits at the beginning of the list.
- I think we should animate the transition instead of making it instantaneous, otherwise the experience is somewhat confusing.

After some more playing:

- It seems the "does not focus the new app" thing happens because the new .desktop file takes a long time to appear in the overview. Does not work even with the sync parameter being TRUE.

- I can get the animation by calling .show() on the overview and doing everything else in a callback for the ::shown signal, which is fired after the show process is completed.

Is this still needed? If not, let's close this.

It's still needed.

Created attachment 206911 [details] [review]
Add a FocusApp method to org.gnome.Shell

This method, which accepts a .desktop filename, is used to highlight
a specific application in the overview, for example because it has
just been created or installed.

Rebased to master now.
It mostly works now (since applications are no longer loaded in batches), except that it seems to have weird interactions with focus, if the app view hasn't been shown yet. That is, the app view is scrolled, but the app is not focused. Could it be related to the focusDummy stuff?

Xan, Bastien, can you try this out again if it's still needed?

The FocusApp method is still needed, yes.

Created attachment 242348 [details] [review]
Add a FocusApp method to org.gnome.Shell

This method, which accepts a .desktop filename, is used to highlight
a specific application in the overview, for example because it has
just been created or installed.

Rebased.

Are we missing 3.10 too?

The patch was originally written for 3.2...

(In reply to comment #14)
> Are we missing 3.10 too?
> 
> The patch was originally written for 3.2...

Oh if no one beats me to it I will review it later today (a bit busy right now).

Review of attachment 242348 [details] [review]:

Looks good just not sure I like the "try again on idle" thing.

::: js/ui/appDisplay.js
@@ +116,3 @@
+        } else {
+            // We probably need to wait until the view is built, try
+            // again in a idle

This is lame ... can't we have a proper "viewDidLoad" or whatever signal?

Created attachment 253808 [details] [review]
Add a FocusApp method to org.gnome.Shell

This method, which accepts a .desktop filename, is used to highlight
a specific application in the overview, for example because it has
just been created or installed.

Review of attachment 253808 [details] [review]:

Looks good.

Attachment 253808 [details] pushed as 415563d - Add a FocusApp method to org.gnome.Shell

See Bug 707505 about the epiphany/Web side of the bug.