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 598349 - don't rely on mouse cursor for startup notification
don't rely on mouse cursor for startup notification
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Colin Walters
gnome-shell-maint
Depends on: 612833 620899
Blocks:
 
 
Reported: 2009-10-13 23:03 UTC by William Jon McCann
Modified: 2010-06-17 19:21 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
mockups (drawn by Jeremy) (25.48 KB, image/jpeg)
2009-10-13 23:03 UTC, William Jon McCann
  Details
show startup notification in AppPanelMenu (23.11 KB, patch)
2010-03-07 17:28 UTC, Maxim Ermilov
none Details | Review
svg progress (59.99 KB, image/svg+xml)
2010-03-07 22:21 UTC, William Jon McCann
  Details
png progress (4.00 KB, image/png)
2010-03-07 22:22 UTC, William Jon McCann
  Details
show startup notification in AppPanelMenu (25.14 KB, patch)
2010-03-07 23:11 UTC, Maxim Ermilov
none Details | Review
show startup notification in AppPanelMenu (24.44 KB, patch)
2010-03-30 10:56 UTC, Maxim Ermilov
none Details | Review
show startup notification in AppPanelMenu (21.30 KB, patch)
2010-04-15 13:24 UTC, Maxim Ermilov
reviewed Details | Review
show startup notification in AppPanelMenu (21.66 KB, patch)
2010-04-21 00:00 UTC, Maxim Ermilov
none Details | Review
show startup notification in AppMenuButton (21.64 KB, patch)
2010-06-03 11:19 UTC, Maxim Ermilov
none Details | Review
screenshot (48.48 KB, image/png)
2010-06-06 17:52 UTC, William Jon McCann
  Details
add st_texture_cache_load_sliced_image (5.91 KB, patch)
2010-06-08 10:42 UTC, Maxim Ermilov
committed Details | Review
show startup notification in AppMenuButton (15.50 KB, patch)
2010-06-08 10:42 UTC, Maxim Ermilov
none Details | Review
show startup notification in AppMenuButton (15.50 KB, patch)
2010-06-10 12:13 UTC, Maxim Ermilov
none Details | Review
Add animated display of startup notification (17.20 KB, patch)
2010-06-17 02:33 UTC, Colin Walters
committed Details | Review

Description William Jon McCann 2009-10-13 23:03:51 UTC
Created attachment 145386 [details]
mockups (drawn by Jeremy)

Our startup notification is less than ideal now for a few reasons.  One of
these reasons is that we rely on the mouse cursor spinning to indicate
activity.

 * doesn't work if no pointers have cursors (eg. touch screens)
 * doesn't work depending on what window the mouse is over
 * is disconnected from the primary place we indicate startup - the application
menu
 * is too subtle / small to really tell the story

Attached are a few ideas Jeremy and I discussed.
Comment 1 William Jon McCann 2009-10-13 23:09:58 UTC
1. shows a barber pole style animation
2. shows a tracer that moves from left to right and stops at the end (possibly sparking when complete)
3. shows a progression, the animated spinner would sit at the end and fade out when the app is done launching.

The app menu itself (if opened) should show something like "Starting..." in insensitive text and still offer the Quit option.

We are leaning toward style 3.
Comment 2 William Jon McCann 2009-11-06 20:28:51 UTC
I think we can just go with style 3 for now.
Comment 3 William Jon McCann 2010-02-09 19:20:25 UTC
Other ideas:
 * We may want to show the app starting up in the app well too unless it is the "current" one.  This will handle the case where multiple apps are starting up.  I suppose we need an indicator that is slightly different from the running one.  Maybe fading slowly between running and "off"?
 * We may want to stay in the overview until an app starts and presents a window.  This might make the transition a little smoother and will make it easier to select a different app if that one fails.
Comment 4 Maxim Ermilov 2010-03-07 17:28:57 UTC
Created attachment 155483 [details] [review]
show startup notification in AppPanelMenu

It work only for app that use libstartup-notification(gedit, anjuta ...).
Comment 5 William Jon McCann 2010-03-07 22:21:53 UTC
Created attachment 155502 [details]
svg progress
Comment 6 William Jon McCann 2010-03-07 22:22:23 UTC
Created attachment 155503 [details]
png progress
Comment 7 William Jon McCann 2010-03-07 22:24:57 UTC
Took a quick shot at making better artwork.

The animations seems to stutter a bit for me.  Ideally it would be as smooth as the hot corner animation.
Comment 8 Maxim Ermilov 2010-03-07 23:11:01 UTC
Created attachment 155508 [details] [review]
show startup notification in AppPanelMenu

make animations more smooth.
use new icon.
Comment 9 William Jon McCann 2010-03-07 23:25:35 UTC
Ugh, I don't think I properly centered the animation images I made... also we should probably get a better artist to make one.  But otherwise is looking pretty good.
Comment 10 Colin Walters 2010-03-18 00:17:15 UTC
Review of attachment 155508 [details] [review]:

Hm, big patch.  I'd prefer to land this after bug 612833 because they touch the same code here, and once that bug lands we'll have MUCH more reliable app tracking, so you won't need to do the hack with looking at the startup ID on the window.
Comment 11 Maxim Ermilov 2010-03-30 10:56:58 UTC
Created attachment 157467 [details] [review]
show startup notification in AppPanelMenu

Merge with HEAD(require patchs from bug 612833)
Comment 12 Maxim Ermilov 2010-04-15 13:24:40 UTC
Created attachment 158812 [details] [review]
show startup notification in AppPanelMenu

Merge with HEAD
Comment 13 Colin Walters 2010-04-20 13:46:43 UTC
Review of attachment 158812 [details] [review]:

::: js/ui/panel.js
@@ +74,3 @@
+                }
+            }
+        }

Would prefer to see this as Shell.TextureCache.load_sliced_image(name, gridWidth, gridHeight);  This current code does synchronous I/O (and image decoding) remember which we want to avoid.

@@ +118,3 @@
         this._label.raise_top();
+        this._i = 0;
+        this._updateId = 0;

How about "this._animationStep"?

@@ +119,3 @@
+        this._w = AppDisplay.APPICON_SIZE / 2;
+        this._i = 0;
+        this._updateId = 0;

"this._clipWidth"?

@@ +124,3 @@
+        this._direction = SPINNER_SPEED;
+        this._i = 0;
+        this._updateId = 0;

I don't see why the AnimatedIcon needs to be a child of TextShadower.  Wouldn't the code be a lot simpler if the AnimatedIcon was in the AppPanelMenu itself?

@@ +128,3 @@
+        this._direction = SPINNER_SPEED;
+        this._i = 0;
+        this._updateId = 0;

Same for the shadow.

@@ +140,3 @@
+    },
+        this._stopAnimation(false);
+    _onDestroy: function() {

Maybe name it "this._animating"?

@@ +155,3 @@
+                this._labels[i].set_clip(this._label.width - this._w, 0, this._w, this.actor.height);
+        }
+    },

Then you wouldn't need all this code, you could just call .set_clip() on the whole actor right?

@@ +176,3 @@
+
+            children[i].remove_clip();
+        for (let i = 2; i < children.length; i++)

So the way I think this would work is that you'd set a clip on the TextShadower, and then inside AppPanelIcon, just set the x/y of the animating icon.  If you do that the St.BoxLayout should position it correctly.

@@ +294,3 @@
+            childBox.x1 = this._w + box.x1 + this._shadow.width;
+        if (direction == St.TextDirection.LTR) {
+

As above, I think this would be better in AppPanelMenu, not in TextShadower.

@@ +425,3 @@
+        if (sequences.length > 0) {
+        let sequences = tracker.get_startup_sequences();
+        let lastSequence = null;

I see you brought back the support for showing raw startup-notification sequences, but I'd rather not do that.  We can't do an application menu for them, so things will be inconsistent.

We need to instead be sure that app tracking works more reliably, and as the startup-notification and GTK+ patches filter out a lot of things will just start working.

There are more heuristics that aren't implemented yet but that we can do like follow the child process created from launching the .desktop file and match up with _NET_WM_PID.

If we were going to show the raw sequences, I'd like to have it be a separate patch anyways.

::: src/st/st-texture-cache.c
@@ +1209,3 @@
+ * @pixbuf: raw pixel data
+ * @cache: a #ShellTextureCache
+ * st_texture_cache_load_from_pixbuf:

You don't need to do this now, but the way I think things should really work is that we have a St.Image class that knows how to animate (and watches its own visibility state to know when to stop animating, etc).  And the St.Image class is backended by the texture cache.

I will work on this at some point myself.
Comment 14 Maxim Ermilov 2010-04-21 00:00:28 UTC
Created attachment 159219 [details] [review]
show startup notification in AppPanelMenu

(require libstartup-notification from git)
Comment 15 Maxim Ermilov 2010-06-03 11:19:22 UTC
Created attachment 162636 [details] [review]
show startup notification in AppMenuButton 

(merger with HEAD)
Comment 16 Colin Walters 2010-06-03 16:14:07 UTC
Jon, can you take a look at this and see if it's what we want from a UX perspective?
Comment 17 William Jon McCann 2010-06-06 17:52:03 UTC
Created attachment 162877 [details]
screenshot

One thing I noticed while testing this is that after starting an app the previous app window still has focus.  And if I, for example, move that last window around then you get a state like in this screenshot.  The app menu incorrectly identifies the focused app and the animation doesn't end.

Other than that I think this looks nice.  I have two concerns about our design for this.  The first is that my progress animation spinner is pretty ugly.  I'll probably need some help from an artist on that.  The second is that it is a little weird to direct the users attention to the top of the screen when the window will eventually appear somewhere in the middle.  But given that we'll need to change the app menu name anyway this is somewhat hard to avoid.

I think we can also use a spinner located at the right of the app name to indicate when the app is busy (a la ipad).

Anyway, once the bug in the screenshot is fixed I think this is something we should try.  I'll work on getting a new image.
Comment 18 Colin Walters 2010-06-07 21:55:48 UTC
Review of attachment 162636 [details] [review]:

Partial review.  Can you split out the texture cache patch?

::: src/shell-app.c
@@ +692,1 @@
     shell_app_state_transition (app, SHELL_APP_STATE_RUNNING);

Hmm...why shouldn't we transition from STARTING -> RUNNING here?

::: src/shell-window-tracker.c
@@ +674,3 @@
           meta_display_focus_the_no_focus_window (display, screen, tv_sec);
+          _shell_app_set_starting (app, starting);
+      if (!starting)

See https://bugzilla.gnome.org/show_bug.cgi?id=620899

::: src/st/st-texture-cache.c
@@ +1002,3 @@
+  gint   grid_width, grid_height;
+  gchar *path;
+typedef struct {

Can't you use:

texture = create_default_texture ();

@@ +1027,3 @@
+  list = g_simple_async_result_get_op_res_gpointer (simple);
+
+  while (list)

Any reason not to use a for loop here?  It's a lot more common for list iteration.

for (list = g_simple_async_result_get_op_res_gpointer (simple); list; list = list->next)

@@ +1062,3 @@
+  g_assert (data);
+
+  if (!(pix = gdk_pixbuf_new_from_file(data->path, NULL)))

Missing space;  gdk_pixbuf_new_from_file (data->path, NULL)

@@ +1073,3 @@
+          GdkPixbuf *pixbuf = gdk_pixbuf_new_subpixbuf (pix, x, y, data->grid_width, data->grid_height);
+          if (!pixbuf)
+            continue;

This should really be g_simple_async_result_set_error(); break

@@ +1085,3 @@
+                                    const gchar       *path,
+                                    gint               grid_width,
+                                    gint               grid_height)

This needs some documentation (at a minimum, it needs (transfer none) since the ClutterGroup is floating.
How about:

/**
 * st_texture_cache_load_sliced_image:
 * @cache: A #StTextureCache
 * @path: Path to a filename
 * @grid_width: Width in pixels
 * @grid_height: Height in pixels
 *
 * This function reads a single image file which contains multiple images internally.  The image file will be divided using @grid_width and @grid_height; note that the dimensions of the image loaded from @path should be a multiple of the specified grid dimensions.
 *
 * Returns: (transfer none): A new ClutterGroup
 */

::: src/st/st-texture-cache.h
@@ +46,3 @@
+                                    const gchar       *path,
+st_texture_cache_load_sliced_image (StTextureCache    *cache,
+ClutterGroup *

Should be split into a separate patch.
Comment 19 Maxim Ermilov 2010-06-08 10:42:13 UTC
Created attachment 163041 [details] [review]
add st_texture_cache_load_sliced_image

I split patch on 3 part. first patch attached to Bug 620899.
Comment 20 Maxim Ermilov 2010-06-08 10:42:46 UTC
Created attachment 163042 [details] [review]
show startup notification in AppMenuButton
Comment 21 Maxim Ermilov 2010-06-10 12:13:18 UTC
Created attachment 163283 [details] [review]
show startup notification in AppMenuButton

merge with HEAD
Comment 22 Colin Walters 2010-06-14 15:15:34 UTC
Review of attachment 163041 [details] [review]:

Looks fine, thanks
Comment 23 Colin Walters 2010-06-14 15:15:59 UTC
Review of attachment 163041 [details] [review]:

Looks fine, thanks!
Comment 24 Colin Walters 2010-06-14 15:15:59 UTC
Review of attachment 163041 [details] [review]:

Looks fine, thanks!
Comment 25 Colin Walters 2010-06-17 02:33:42 UTC
Created attachment 163886 [details] [review]
Add animated display of startup notification

The shell design says that upon launching an application,
no X window should have focus, and we should display an
animated launching indicator.

Implement this by in panel.js, keep track of the last started
application.  If there isn't currently an X focus, show an animation
for the last starting application.
Comment 26 Colin Walters 2010-06-17 02:34:37 UTC
I think it's a little cleaner to keep track of the started app separately in panel.js instead of using the patch from bug 620899.  Let me know what you think!