GNOME Bugzilla – Bug 605491
Misc style/UI changes
Last modified: 2010-01-07 23:31:46 UTC
See patches.
Created attachment 150408 [details] [review] Change "Documents" to "Items" Per 20091114 mockup.
Created attachment 150409 [details] [review] Misc. style changes Matching 20091114 mockup.
Created attachment 150410 [details] [review] [Makefile-st.am] Generate st/st.h All of the st-$foo.h files say to include st.h, but we didn't have one. Therefore, generate it.
Created attachment 150411 [details] [review] [ShellSlicer] New class to display center of child A #StBin that has 0 minimum size, and will clip its child in the middle.
Created attachment 150412 [details] [review] [panel] Scale up, center and fade application icon in app menu Per 20091114 design.
Review of attachment 150408 [details] [review]: Looks fine, obviously. In the mockup, "Places" is renamed to "Places & Devices" - could make sense to include this in the patch.
(In reply to comment #3) > Created an attachment (id=150410) [details] [review] > [Makefile-st.am] Generate st/st.h > > All of the st-$foo.h files say to include st.h, but we didn't have > one. Therefore, generate it. The generated file should be added to .gitignore
Review of attachment 150412 [details] [review]: The scaling and centering looks fine, although there is a minor issue with the readability of the application name for light app icons (e.g. evolution). The fading on the other hand does not seem right: The gradient should probably only be applied to the texture's alpha channel to avoid conflicts with the panel background - it is not too obvious with the panel's current dark gradient, but the panel background is effectively hardcoded to black. ::: src/Makefile.am @@ +87,3 @@ shell-global.h \ + shell-slicer.h \ + shell-slicer.c \ IMO, this part should rather be merged with the previous patch.
Looks really nice. Noticed two issues: * As Florian noted, text of app name is hard to read. <mccann> I think we did a subtle shadow on the text in the mockup <mccann> jeremyperry: do you remember? <jeremyperry> yep a dark shadow on the text helps is stand out from the icons. Best if the shadow isnt offset * Also the icon changes size while the application is launching
Review of attachment 150410 [details] [review]: Basically looks fine. .gitignore addition like Florian noted, and please make sure that it doesn't break 'make distcheck' before committing. ::: src/Makefile-st.am @@ +152,1 @@ $(st_built_sources) \ When the backslashes are lined up, please keep them this way (I'd also prefer if the backslashes were lined up for additions like the st/st.h rule, but I know that's not so easy if you don't have Emacs and C-c C-\ )
Review of attachment 150411 [details] [review]: In general, I think it looks good. I might have called it StClipBin rather than StSlicer, but this name is OK. Some comments in detail. ::: src/shell-slicer.c @@ +3,3 @@ +/** + * SECTION:shell-slicer + * @short_description: Display only part of another actor Don't we have the SECTION comments in the headers, and not in the C files? @@ +43,3 @@ + } + + st_theme_node_adjust_preferred_width (theme_node, NULL, natural_width_p); You need to adjust the min_width as well, since you draw the theme stuff (border, etc) in the parent and not sliced @@ +72,3 @@ + } + + st_theme_node_adjust_preferred_height (theme_node, NULL, natural_height_p); And here you need to adjust the min_height @@ +115,3 @@ + + x_overflow = MAX(0, (int)(((child_box.x2 - child_box.x1) - width + 0.5) / 2)); + y_overflow = MAX(0, (int)(((child_box.y2 - child_box.y1) - height + 0.5) / 2)); Do you really want to clamp here or do you want to center the child if it is smaller than the widget itself? It's confusing to use a temporary for width/height and not child_width, child_height- makes the expression look less parallel than it is. Actually, rather than just centering the child, you should use st_bin_get_align_factors(). @@ +120,3 @@ + + cogl_translate (-x_overflow, -y_overflow, 0); + cogl_clip_push (x_overflow, y_overflow, width, height); This is going to be easier to undrestand if you reverse the two lines: clip (0, 0, width, height); translate ((int)(0.5 + x_align * (width - child_width)), (int)(0.5 + y_align * (height - child_height)); (Also, missing space before the two '-') @@ +136,3 @@ + + /* get the default pick implementation */ + CLUTTER_ACTOR_CLASS (shell_slicer_parent_class)->pick (self, pick_color); But this is st_bin_pick itself, which does exactly what you have here! Also, picking needs the same handling as painting in basically all aspects.
Review of attachment 150412 [details] [review]: Generally looks good, few more things in addition to Florian's comment. ::: js/ui/panel.js @@ +100,3 @@ + + _repositionLabel: function() { + this._label.x = Math.floor(AppDisplay.APPICON_SIZE / 2); Is this right for the non-app-but-window case where we are doing the old mini-icon thing? @@ +142,3 @@ let icon = this._activeSequence.create_icon(PANEL_ICON_SIZE); + this._iconSlicer.set_child(icon); + this._iconFade.hide(); It's not apparent why you do different things for apps and non-app windows - is it because we only have the mini-icon for the window passed through the various layers and not the icon? A comment would be appreciated. ::: src/shell-drawing.c @@ +55,2 @@ void +shell_draw_app_icon_fade (ClutterCairoTexture *texture) Brief comment describing what this draws would be useful. Eventually would be cool to have better CSS gradient support, which would allow this without custom drawing code.
Review of attachment 150409 [details] [review]: Assume it's OK, hard to really review :-)
Review of attachment 150408 [details] [review]: I think the wording is a bit strange for search, but yes, it matches the current mockups, so good. Florian's comment about including Places & Devices in this patch makes sense to me.
(In reply to comment #9) > Noticed two issues: > * As Florian noted, text of app name is hard to read. > > <mccann> I think we did a subtle shadow on the text in the mockup > <mccann> jeremyperry: do you remember? > <jeremyperry> yep a dark shadow on the text helps is stand out from the icons. > Best if the shadow isnt offset This is going to be a bit hard to do - the drop shadow stuff that Florian has worked on isn't going to work since it only drops shadows the background, and not the contents of the widget. You can try the simple nautilus style approach of creating five labels with the same text, four of them black and offset by one pixel in each direction. (since you are already positioning the label manually it should be easy to position addition labels as well.)
(In reply to comment #11) > Review of attachment 150411 [details] [review]: > > In general, I think it looks good. I might have called it StClipBin rather than > StSlicer, but this name is OK. > > Some comments in detail. > > ::: src/shell-slicer.c > @@ +3,3 @@ > +/** > + * SECTION:shell-slicer > + * @short_description: Display only part of another actor > > Don't we have the SECTION comments in the headers, and not in the C files? [walters@pocket gnome-shell (misc)]$ grep SECTION src/*.h src/shell-recorder.h: * SECTION:ShellRecorder [walters@pocket gnome-shell (misc)]$ grep SECTION src/*.c src/shell-app.c: * SECTION:shell-app src/shell-app-usage.c: * SECTION:shell-app-usage src/shell-doc-system.c: * SECTION:shell-doc-system src/shell-generic-container.c: * SECTION:shell-generic-container src/shell-menu.c: * SECTION:shell-menu src/shell-slicer.c: * SECTION:shell-slicer src/shell-stack.c: * SECTION:shell-stack src/shell-window-tracker.c: * SECTION:shell-window-tracker [walters@pocket gnome-shell (misc)]$ Admittedly there is a high correlation here between authorship and placement of SECTION =) But more seriously is there precedent for the former? I've no objections to switching if so.
Created attachment 150920 [details] [review] [dash] Change section titles to match 20091114 mockup Documents is changed to Items, and add "& Devices" to Places.
Created attachment 150921 [details] [review] Misc. style changes Matching 20091114 mockup.
Created attachment 150922 [details] [review] [Makefile-st.am] Generate st/st.h All of the st-$foo.h files say to include st.h, but we didn't have one. Therefore, generate it.
Created attachment 150923 [details] [review] [ShellSlicer] New class to display center of child A #StBin that has 0 minimum size, and will clip its child in the middle.
Created attachment 150924 [details] [review] [panel] Scale up, center and fade application icon in app menu Per 20091114 design.
Review of attachment 150922 [details] [review]: As pointed out by Florian on IRC, doesn't address the \ alignment problems, looks fine to commit once that is fixed
Review of attachment 150923 [details] [review]: Looks good except for one stylistic comment. ::: src/shell-slicer.c @@ +105,3 @@ + return; + + _st_bin_get_align_factors ((StBin*)self, &x_align, &y_align); I'd prefer to consistently use cast macros ST_BIN(), CLUTTER_ACTOR(), SHELL_SLICER(), etc. - I think ad-hoc "it's faster if we use a raw C cast" is a mistake - if casts show up as problems, then we can compile the whole thing with -DG_DISABLE_CHECK_CASTS (or whatever that is called), as we do for GTK+ for stable release. (Obviously, there's lots of this ad-hoc C casting in St via it's Mx inheritance, I'd like to avoid introducing more.)
Review of attachment 150924 [details] [review]: Looks pretty good, and a reasonable way of doing of doing the shadowing. Doing the fade this way (creating a modified texture rather than overlaying a black gradient) seems OK, though not obviously better than the previous implementation. Few things noticed in detail. And I'd like a better commit message. ::: js/ui/panel.js @@ +83,3 @@ + let [minHeight, natHeight] = this._label.get_preferred_height(forWidth); + alloc.min_size = minHeight + 2; + alloc.natural_size = natHeight + 2; In general, I think shadows normally want to take up padding space rather than cause a larger allocation. Since there is no constraint of a ClutterActor to draw within its allocation, I'd probably just lap the shadow out of the allocation and if the enclosing code wants to add padding, let the enclosing code add padding. @@ +132,3 @@ + }, + + _onTextChanged: function() { You don't use this @@ +189,3 @@ + let labelHeight = labelAlloc.y2 - labelAlloc.y1; + // Add one to account for the one pixel shadow + this._label.actor.y = Math.floor((actorHeight - labelHeight) / 2) + 1; But as you have it, the shadow starts at the x,y of the actor, so you'd center the label shadow and all. (And if you took my suggestion, then you'd still just center the label and the shadow would go around that symmetrically.) @@ +228,3 @@ + this._appIconMode = true; + let icon = this._focusedApp.create_icon_texture(AppDisplay.APPICON_SIZE); + let faded = Shell.fade_app_icon(icon); /* TODO consider caching */ Caching is probably unnnecessary if this only changes when the active app changes @@ +235,1 @@ } else if (this._activeSequence != null) { As commented before, this requires a comment explaining why you don't do things the nice way in this case ::: src/shell-drawing.c @@ +82,3 @@ + pixels = g_malloc0 (rowstride * height); + + cogl_texture_get_data (texture, COGL_PIXEL_FORMAT_RGBA_8888, I don't really like the unpremultiply then premultiply that is going to go on here since data is stored premulitplied internally - unpremultication involves an expensive divide - I'd suggest using COGL_PIXEL_FORMAT_RGBA_8888_PRE here and belowand multiplying all four channels, not just the A channel. @@ +92,3 @@ + { + guchar *pixel = &pixels[j * rowstride + i * 4]; + pixel[3] = pixel[3] * (1.0 - ((float) i - fade_start) / (width - fade_start)); Would suggest rounding (0.5 + ) for stability.
Attachment 150920 [details] pushed as 5202dd5 - [dash] Change section titles to match 20091114 mockup Attachment 150921 [details] pushed as 7043215 - Misc. style changes Attachment 150922 [details] pushed as da797df - [Makefile-st.am] Generate st/st.h Attachment 150923 [details] pushed as 262f92e - [ShellSlicer] New class to display center of child Attachment 150924 [details] pushed as c17e124 - [panel] Scale up, center and fade application icon in app menu