After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 654086 - Add a method to make the Shell focus on a desktop file
Add a method to make the Shell focus on a desktop file
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.0.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks: 707505
 
 
Reported: 2011-07-06 13:57 UTC by Xan Lopez
Modified: 2013-12-16 15:20 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add a FocusApp method to org.gnome.Shell (5.32 KB, patch)
2011-07-09 13:33 UTC, Giovanni Campagna
reviewed Details | Review
Add a FocusApp method to org.gnome.Shell (4.46 KB, patch)
2012-02-06 16:18 UTC, Giovanni Campagna
none Details | Review
Add a FocusApp method to org.gnome.Shell (6.39 KB, patch)
2013-04-24 17:33 UTC, Giovanni Campagna
reviewed Details | Review
Add a FocusApp method to org.gnome.Shell (6.83 KB, patch)
2013-09-02 08:10 UTC, Giovanni Campagna
committed Details | Review

Description Xan Lopez 2011-07-06 13:57:28 UTC
Epiphany 3.2 will allow to create WebApps from any web page. As part of the process a new .desktop file (launching ephy in "application mode") will be created. It would be useful to be able to the shell (from another process) to focus on the newly created launcher in the overview mode so that it's obvious for the user how the new application can be launched. For an example, see how iPad does it: http://www.youtube.com/watch?v=8_vASkxYUMU
Comment 1 Giovanni Campagna 2011-07-09 13:33:35 UTC
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.
Comment 2 drago01 2011-07-13 17:37:09 UTC
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?
Comment 3 Giovanni Campagna 2011-07-13 17:43:10 UTC
(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).
Comment 4 drago01 2011-07-13 18:01:34 UTC
(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.
Comment 5 Xan Lopez 2011-07-22 22:26:53 UTC
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.
Comment 6 Xan Lopez 2011-07-23 01:04:00 UTC
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.
Comment 7 Giovanni Campagna 2011-12-08 21:47:57 UTC
Is this still needed? If not, let's close this.
Comment 8 Bastien Nocera 2011-12-08 21:57:57 UTC
It's still needed.
Comment 9 Giovanni Campagna 2012-02-06 16:18:42 UTC
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.
Comment 10 Giovanni Campagna 2012-02-06 16:21:08 UTC
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?
Comment 11 Jasper St. Pierre (not reading bugmail) 2012-05-18 21:17:28 UTC
Xan, Bastien, can you try this out again if it's still needed?
Comment 12 Bastien Nocera 2013-04-24 14:14:07 UTC
The FocusApp method is still needed, yes.
Comment 13 Giovanni Campagna 2013-04-24 17:33:47 UTC
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.
Comment 14 Giovanni Campagna 2013-08-27 09:17:12 UTC
Are we missing 3.10 too?

The patch was originally written for 3.2...
Comment 15 drago01 2013-08-27 09:21:21 UTC
(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).
Comment 16 drago01 2013-08-27 15:08:55 UTC
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?
Comment 17 Giovanni Campagna 2013-09-02 08:10:03 UTC
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.
Comment 18 drago01 2013-09-02 08:15:18 UTC
Review of attachment 253808 [details] [review]:

Looks good.
Comment 19 Giovanni Campagna 2013-09-02 09:03:36 UTC
Attachment 253808 [details] pushed as 415563d - Add a FocusApp method to org.gnome.Shell
Comment 20 Bastien Nocera 2013-09-04 20:16:29 UTC
See Bug 707505 about the epiphany/Web side of the bug.