GNOME Bugzilla – Bug 598349
don't rely on mouse cursor for startup notification
Last modified: 2010-06-17 19:21:09 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.
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.
I think we can just go with style 3 for now.
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.
Created attachment 155483 [details] [review] show startup notification in AppPanelMenu It work only for app that use libstartup-notification(gedit, anjuta ...).
Created attachment 155502 [details] svg progress
Created attachment 155503 [details] png progress
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.
Created attachment 155508 [details] [review] show startup notification in AppPanelMenu make animations more smooth. use new icon.
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.
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.
Created attachment 157467 [details] [review] show startup notification in AppPanelMenu Merge with HEAD(require patchs from bug 612833)
Created attachment 158812 [details] [review] show startup notification in AppPanelMenu Merge with HEAD
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.
Created attachment 159219 [details] [review] show startup notification in AppPanelMenu (require libstartup-notification from git)
Created attachment 162636 [details] [review] show startup notification in AppMenuButton (merger with HEAD)
Jon, can you take a look at this and see if it's what we want from a UX perspective?
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.
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.
Created attachment 163041 [details] [review] add st_texture_cache_load_sliced_image I split patch on 3 part. first patch attached to Bug 620899.
Created attachment 163042 [details] [review] show startup notification in AppMenuButton
Created attachment 163283 [details] [review] show startup notification in AppMenuButton merge with HEAD
Review of attachment 163041 [details] [review]: Looks fine, thanks
Review of attachment 163041 [details] [review]: Looks fine, thanks!
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.
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!