GNOME Bugzilla – Bug 598324
refinements for the window selector
Last modified: 2009-11-21 00:28:07 UTC
Created attachment 145374 [details] mockup Here are a couple of tweaks for the window selector / workspace view. * Want to have a close window button * Caption should be underneath window * Icon should overhang corner of window * Change the active workspace indicator to match the selection box used in alt+tab Attached is a mockup of the first three.
Jeremy reminded me I forgot to mention: * Drop shadow under icon to provide some separation from the window * Note new overhanging position of icons
Created attachment 145489 [details] [review] Patch implementing most of the outlined refinements The attached patch (1) adds a close button to the window preview (2) moves the title caption underneath the previews (3) repositions the window icons to overlap the window corner (4) adds drop shadows to icons (2) and (3) are pretty straight forward. (1) duplicates most functions handling the title captions (updateTitle <=> updateCloseButton, showTitle <=> showCloseButton etc.), it might be prettier to have combined functions of these handling both objects (updateHoverItems?) IMHO the most problematic part is (4) - I have not been able to figure out how to do some nice looking shadows in cairo, so I ended up using ClutterShader with a simple fragment program. Unfortunately, GLSL is not supported by many hardware drivers (read: it doesn't work with my netbook's intel chip) - an alternative would be using GL_ARB_fragment_program (which _is_ supported by more hardware), but there is no support in clutter. So possible solutions would be (a) figure out how to do it with cairo (b) figure out how to use GL_ARB_fragment_program with clutter (c) do nothing and hope for the DRI project to add support for GLSL to more drivers, falling back to no shadows when support is missing Opinions?
Hey, cool. Thanks for taking a shot at this. Overall looking pretty good. Definitely in the direction we want to go. Here's an updated mockup: http://www.gnome.org/~mccann/screenshots/clips/20091022213022/g4299.png Things I noticed while trying out the patch: * The shadows are much too hard and the offsets don't really look right. * The style of the caption box should be more like the latest mockup * Sometimes the close button goes underneath the icon from the window above * Sometimes the caption box goes off the bottom of the workspace * Sometimes the caption box goes over or under other windows * Using the Close button on the Empathy buddy list had no effect
(In reply to comment #2) > IMHO the most problematic part is (4) - I have not been able to figure out how > to do some nice looking shadows in cairo, so I ended up using ClutterShader > with a simple fragment program. Unfortunately, GLSL is not supported by many > hardware drivers (read: it doesn't work with my netbook's intel chip) - an > alternative would be using GL_ARB_fragment_program (which _is_ supported by > more hardware), but there is no support in clutter. So possible solutions would > be > (a) figure out how to do it with cairo > (b) figure out how to use GL_ARB_fragment_program with clutter > (c) do nothing and hope for the DRI project to add support for GLSL to more > drivers, falling back to no shadows when support is missing There are lots of options for getting hard shadows as you have here; in addition to shaders: - You could draw them with cairo (cairo_mask() will use the alpha channel of a color image to draw a shape) - You could use the cogl material API to draw a CoglTexture with the alpha channel faded and the color components clamped to black. Getting soft shadows is harder - it isn't trivial to do even with shaders. (The obvious approach of doing gaussian convolution in the shader is very expensive in terms of texture references, which are a limited quantity especially on lower end hardware.) Two approaches that might work here: A) Use a the Cogl material approach and then draw a hard shadow 5 times - once in the center then 4 times offset by 1 pixel in each direction at reduced alpha. This will be OK efficient but the shadow won't be very soft. And making the shadow sufficiently soft by drawing more copies is going to kill efficiency. B) Do a two-pass Guassian convolution in software and create a second texture with that. (Can even be an A8 texture.) The textures that we are shadowing here are small so I think the overhead of doing it in software will be minimal. There is a somewhat significant extra overhead in terms of additional texture handles, but if we are only creating these shadows for running apps its probably OK. I think B) is probably the better approach. So how would this fit into the code? What I think I'd do is add an extra parameter: ShellCacheOptions options to the load functions in ShellTextureCache and have SHELL_CACHE_LOAD_SHADOW one of those options. Put the actual code for generating shadows from a GdkPixbuf in a separate source file, say, shell-shadow.c. (Colin may have some better ideas about code organization.) This should certainly be split out from the rest of the changes here into a different patch.
A few other things I noticed while trying this. * Once we get the Information Bar / Undo area we should use that to display that the window has been closed and try to offer the option to undo it. * If the window doesn't really support closing - such as when it simply minimizes we shouldn't offer the same option to close. * If the application pops up a confirmation on close we should zoom into the window to see it * We should try to make it easier to get to the close button with the mouse. Right now if I accidentally move outside the window area the close button goes away. We may need some gtkmenu submenu like magic here.
Created attachment 147879 [details] [review] Updated patch
Sorry for the silence, I somehow managed to get myself off CC and was unaware of your comments until a few days ago. I updated the patch to get the scaling/positioning better, so caption boxes and close buttons should no longer interfere with other windows. As I updated the caption boxes to better match the latest mockups, I took the liberty to port them to St.Label and make the style changes in CSS. (In reply to comment #4) > I think B) is probably the better approach. OK, so Gauss it is ... > So how would this fit into the > code? What I think I'd do is add an extra parameter: > > ShellCacheOptions options > > to the load functions in ShellTextureCache and have SHELL_CACHE_LOAD_SHADOW one > of those options. Put the actual code for generating shadows from a GdkPixbuf > in a separate source file, say, shell-shadow.c. So, to get a grip of the bigger picture - having shadows controlled by a flag means that shadow offset and blur are to be hardcoded? What about StTextureCache? Should there be a custom CSS property for shadows or do we expect themes to provide images with shadows if desired? > This should certainly be split out from the rest of the changes here into a > different patch. OK, I'll try to start working on this this week. (In reply to comment #5) > A few other things I noticed while trying this. > > * Once we get the Information Bar / Undo area we should use that to display > that the window has been closed and try to offer the option to undo it. That should be possible by hiding the window/clone instead of closing it right away - but: > * If the window doesn't really support closing - such as when it simply > minimizes we shouldn't offer the same option to close. > * If the application pops up a confirmation on close we should zoom into the > window to see it How can we detect those - is there any way to determine how a window will behave on close *before* hitting the close button? And how can we detect confirmation dialogs, especially when we just hide the corresponding actor? > * We should try to make it easier to get to the close button with the mouse. > Right now if I accidentally move outside the window area the close button goes > away. We may need some gtkmenu submenu like magic here. I'll look into the magic gtkmenu is doing and update the patch.
Created attachment 147906 [details] mockup, with frame Thank you Florian for your patch. Visually the caption box seems strange, but I know this is just a step in the process. Personally the caption box looks better spatially inside the scaled window. I also took the liberty of making a mockup of what window hovering might look like if the style were consistent with the app-switcher. Just some food for thought.
Created attachment 147990 [details] [review] Updated patch Fixed mutter warnings caused by previous patch version.
Review of attachment 147990 [details] [review]: ::: data/theme/gnome-shell.css @@ +91,3 @@ +.window-caption { + +/* Overlay */ This is OK, but what I've been doing typically when porting things to CSS is also at least converting the toplevel element (in this case the Workspaces) to a St container (St.Bin probably), and set things like the color/font from there. In this case we'd probably only want to set the color to white there. Also I've mostly been just setting font-size but inheriting the default of Sans. ::: js/ui/workspaces.js @@ +35,3 @@ + +const WINDOW_ICON_OVERLAP_Y = 0.667; +const WINDOW_ICON_OVERLAP_X = 0.667; This one seems fine as a constant in the JS code, but just for reference I've lately been on a CSS conversion whirlwind, if you do want to convert this to CSS, look at how I've been doing custom style properties here: http://bugzilla-attachments.gnome.org/attachment.cgi?id=147935 Search for "-shell-"; basically connect to style-changed, look up the property, cache the result as a member variable, use in layout code. @@ +406,3 @@ + let titleY = this.actor.y + cloneScreenHeight * title.scale_y + + let titleX = this.actor.x + + title.width = Math.min(title.fullWidth, cloneScreenWidth); Since we're reverse scaling we're going to get fuzzy text regardless, but we might as well add a Math.floor here anyways. There is also the issue that the window clones (I'm fairly sure) are themselves positioned non-integrally; we should probably add a // TODO or FIXME type thing here.
Small changes to the design in the latest mockup: http://www.gnome.org/~mccann/shell/mockups/20091114/shell-mockup-overview.png * Always show the title caption * Don't show the app icon
(In reply to comment #11) > * Don't show the app icon Seriously? I find the icon much more useful for identifying the window than the thumbnail is. The thumbnails all look the same; off white with squiggles
(In reply to comment #12) > (In reply to comment #11) > > * Don't show the app icon > > Seriously? I find the icon much more useful for identifying the window than the > thumbnail is. The thumbnails all look the same; off white with squiggles Are you using multiple workspaces though? (Also, are you actually running with the mutter scaledown patch? It hasn't landed yet AFAIK)
no, not running with the scaledown patch but I don't think that will change much. on my desktop machine i use only a single workspace, and find the thumbnails to be useless. on my laptop I use two workspaces and find the entire window/workspace selector to be useless.
Created attachment 148031 [details] [review] New patch for updated mockup (In reply to comment #11) > Small changes to the design in the latest mockup: > http://www.gnome.org/~mccann/shell/mockups/20091114/shell-mockup-overview.png > > * Always show the title caption > * Don't show the app icon New mockup, new patch ... (In reply to comment #10) > This is OK, but what I've been doing typically when porting things to CSS is > also at least converting the toplevel element (in this case the Workspaces) to > a St container (St.Bin probably), and set things like the color/font from > there. OK, done. > ::: js/ui/workspaces.js > @@ +35,3 @@ > + > +const WINDOW_ICON_OVERLAP_Y = 0.667; > +const WINDOW_ICON_OVERLAP_X = 0.667; > > This one seems fine as a constant in the JS code, but just for reference I've > lately been on a CSS conversion whirlwind, if you do want to convert this to > CSS Dito. > @@ +406,3 @@ > + let titleY = this.actor.y + cloneScreenHeight * title.scale_y + > + let titleX = this.actor.x + > + title.width = Math.min(title.fullWidth, cloneScreenWidth); > > Since we're reverse scaling we're going to get fuzzy text regardless, but we > might as well add a Math.floor here anyways. I got rid of the reverse scaling for both title captions and close buttons - especially the text looks much nicer now.
Florian, Looks really nice. Good work. A few comments: * The border color on the caption boxes is a little bright. Unfortunately iirc, it is not easy to do achieve the effect we're using in the mockup because the border in css is over the top of the box background. In the mockup the border is composited directly onto the wallpaper. For now perhaps we should just use the same border color used in the alt-tab application switcher. * Seems like we're still missing the subtle shadow effect on the close button. Not the end of the world by any means. * The text in the captions looks much better From my point of view this looks good to go if we fix the border color.
Created attachment 148062 [details] [review] Patch for updated mockup (In reply to comment #16) > Looks really nice. Good work. A few comments: Thanks a lot:) > * The border color on the caption boxes is a little bright. Unfortunately > iirc, it is not easy to do achieve the effect we're using in the mockup because > the border in css is over the top of the box background. In the mockup the > border is composited directly onto the wallpaper. For now perhaps we should > just use the same border color used in the alt-tab application switcher. OK, done. > * Seems like we're still missing the subtle shadow effect on the close button. Indeed. As Owen pointed out in comment #4, this should go in a separate patch. Of course we could add a shadow to the theme image, but that's cheating, ain't it? > * The text in the captions looks much better > > From my point of view this looks good to go if we fix the border color. Yahoo! Nice.
The patch looks great, I just ran into one issue, which is the close button instantly disappearing if you happen to go outside the event box. I've tweaked the patch to add an idle timeout (750 milliseconds) and it feels better. I went ahead and pushed your original patch and my patch on top. Should we keep this bug open for further refinements?
(In reply to comment #18) > The patch looks great, I just ran into one issue, which is the close button > instantly disappearing if you happen to go outside the event box. I've tweaked > the patch to add an idle timeout (750 milliseconds) and it feels better. Great. That sounds like the issue I mentioned in comment #5. Good stuff. > I went ahead and pushed your original patch and my patch on top. Should we > keep this bug open for further refinements? I think it was decided to do the shadow part in another bug. Do we have that bug yet? I'll close this one I guess.
There is still some minor issue with the closeButtons: If a closeButton gets shown on entering a WindowOverlay, a previously shown closeButton that has not reached the idle timeout yet only gets hidden when it is on the same workspace as the newly shown closeButton. If it is on a different workspace than the new closeButton, you can see two closeButtons at the same time, until the previous closeButton reaches its idle timeout.