GNOME Bugzilla – Bug 613804
panel application menu
Last modified: 2010-05-11 18:13:32 UTC
Initial patches; the main panel patch needs some more work but filing so it doesn't get lost.
Created attachment 156977 [details] [review] [ShellApp] Add quit method Closes the app, will be used for the panel app menu.
Created attachment 156978 [details] [review] [panel] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
Created attachment 156979 [details] [review] [panel] Convert calendar to menu item
Created attachment 157060 [details] [review] [panel] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
Created attachment 157061 [details] [review] [panel] Convert calendar to menu item
Created attachment 157237 [details] [review] Make panel more menu-like, initial application menu * Abstract out PanelMenuBar * Don't allow transitioning the overview while the panel is open
Created attachment 157238 [details] [review] Convert calendar to menu item
Review of attachment 156977 [details] [review]: Doesn't look finished. ::: src/shell-app.c @@ +493,3 @@ + * @app: A #ShellApp + * @cancellable: (allow-none): A #GCancellable + * @callback: Invoked when application quit request completes Hmm, not sure how I feel about using the GIO async infrastructure, but sure. @@ +501,3 @@ + * + * This function uses several different techniques for + * requesting the quit. It does? @@ +521,3 @@ + continue; + + meta_window_delete (win, shell_global_get_current_time (shell_global_get ())); Have you checked what happens with this on a couple of apps (say the GIMP and in openoffice) if you have multiple unsaved windows open? @@ +524,3 @@ + } + + /* TODO - watch for whether the app actually goes away, set an error on timeout, etc. */ This needs to be fleshed out before committing, or if there isn't anything we want to do, then shouldn't have a TODO here. (Though then the use of the async infrastructure is a bit odd.)
Review of attachment 157237 [details] [review]: Haven't reviewed the CSS, presumably OK (have to run now.) Mostly looks good, some comments. ::: js/ui/panel.js @@ +153,3 @@ + _onEnter: function() { + this._isHover = true; + this.actor.set_style_pseudo_class('hover'); This should be using the new StWidget hover support. @@ +182,3 @@ + _getPreferredWidth: function(actor, forHeight, alloc) { + let [minInternalSize, natInternalSize] = this._box.get_preferred_width(forHeight); + let sourceAlloc = this._sourceButton.allocation; Hmm, while this looks innocuous, this needs a comment like: // The allocation property unlike get_allocation_box() doesn't trigger // synchronous relayout, which would be bad here. We assume that the // previous allocation of the button is fine. @@ +199,3 @@ + let availHeight = box.y2 - box.y1; + childBox.x1 = 0; + childBox.y1 = 0; 0, 0 isn't right here - that will overlap the border - you need to pay attention to the x,y of the box passed in. @@ +206,3 @@ + + addAction: function(title, callback) { + let display = new PanelMenuItem(title); Odd variable name @@ +233,3 @@ + + _init: function(menuAlignment) { + if (!menuAlignment) GJS enums are integers, if I remmeber correctly. @@ +242,3 @@ + this.actor.connect('button-press-event', Lang.bind(this, this._onButtonPress)); + this.actor.connect('enter-event', Lang.bind(this, this._onEnter)); + this.actor.connect('leave-event', Lang.bind(this, this._onLeave)); Needs to be using the new hover functionality in StWidget. You can set add your own pseudo-class for the active state then write the CSS rules so that overrides hover. @@ +264,3 @@ + this._repositionMenu(); + Tweener.addTween(this.menu.actor, + { y: panelActor.y + panelActor.height, Are menus that slide out what we want? I'm not sure what's good for the clock (which is meant to be looked at, not so much clicked) is good for menus. Fine for now. @@ +268,3 @@ + transition: "easeOutQuad", + onComplete: Lang.bind(this, function() { + this._state = this.State.OPEN; I think you need to handle close() happening while you are in the TRANSITIONING state - not just ignore it. Alsom ince there is an onCloseComplete, it's a bit ugly to have 'onOpenComplete' inline. @@ +273,3 @@ + + this.actor.set_style_pseudo_class('pressed'); + this.emit('open-state', true); Weird signal name. it's either open-state-changed, or better just open/close @@ +296,3 @@ + let [stageX, stageY] = this.actor.get_transformed_position(); + let [buttonWidth, buttonHeight] = this.actor.get_transformed_size(); + stageX = Math.floor(stageX); Doesn't make sense to floor this but assume other quantities below are integral. @@ +353,3 @@ + reactive: true }); + // Note this variable is read by Panel + this._isMenuOpen = false; Don't _prefix it then, it's fine as a public property. Subclasses are also ways of consuming an API. (I.e., C++ protected is stupid.) @@ +357,3 @@ + // these are more "private" + this._eventCaptureId = 0; + this._activeMenu = null; Should be activeMenuButton @@ +365,3 @@ + button.actor.connect('enter-event', Lang.bind(this, this._onMenuEnter, button)); + button.actor.connect('leave-event', Lang.bind(this, this._onMenuLeave, button)); + button.actor.connect('button-press-event', Lang.bind(this, this._onMenuPress, button)); Think these connections are poking into the internals of another actor, better to make the menu button handle more itself. @@ +379,3 @@ + _onMenuEnter: function(actor, event, button) { + if (!this._isMenuOpen || button == this._activeMenu) + return false; I feel pretty much backwards from your comment on a recent bug, and think that this would read better if this was: if (this._isMenuOpen && button != this._activeMenu) { } because it we are actively doing something in the unusual case when mouse pointer enters a different menu button, not just optimizing out something. It also aovids the duplicate return value. @@ +389,3 @@ + _onMenuLeave: function(actor, event, button) { + return false; + }, No point in having this if it's empty @@ +391,3 @@ + }, + + _onMenuPress: function(actor, event, button) { Think this should be driven off the open state of the menus, not direct event conenctions. @@ +400,3 @@ + }, + + _containsActor: function(container, actor) { We really should get this added to ClutterActor() or ClutterContainer(), we keep rewriting it all over the place. @@ +423,3 @@ + this._activeMenu.close(); + this._disconnectCapture(); + return true; Hmm, don't understand why this doesn't break clicking on a menu item to select. @@ +429,3 @@ + this._activeMenu.close(); + this._disconnectCapture(); + return true; This is not right, you are going to get prelight (and mousewheel scrolling, etc) outside of the menus, but not clicks working. @@ +434,3 @@ + }, + + _disconnectCapture: function() { A method called disconnectCapture shouldn't do random other stuff without being renamed. @@ +568,3 @@ + app.request_quit_finish(); + } catch (e) { + // FIXME - add some sort of force quit dialog Won't you already get the Metacity force quite dialog (possibly multiple times?) @@ +995,3 @@ _onHotCornerEntered : function() { + if (this._isMenuOpen) + return false; Think this better off inside where we actually trigger the overview instead of guarding the enter/leave handling. Otherwise closing the menu + mouse jiggle could trigger the overview.
Created attachment 159773 [details] [review] Add quit method Closes the app, will be used for the panel app menu.
Created attachment 159774 [details] [review] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
Created attachment 159775 [details] [review] Convert calendar to menu item
Review of attachment 159773 [details] [review]: ::: src/shell-app.c @@ +656,3 @@ + * to that window. + */ + modal = get_most_recent_modal_window (app); I'm pretty sure that this won't work as expected without modifications in ShellWindowTracker / ShellApp - right now, app->windows contains application windows which ShellWindowTracker considers "interesting"; this does exclude every window which has the skip-taskbar hint set, so ShellApp hardly knows about any of its modal windows. @@ +672,3 @@ + } + + /* TODO fill in by using DBus GtkApplication Quit method, try XSMP */ Not sure how soon GtkApplication is supposed to land - in the meantime a naive approach of closing each window might make sense to avoid the confusion of non-working menu items (which might make sense anyway for apps which support neither GtkApplication nor XSMP ...) ::: src/shell-app.h @@ +4,3 @@ #include <clutter/clutter.h> +#include <gio/gio.h> Not used.
For reference the mockup and info is here: http://live.gnome.org/GnomeShell/Design/Whiteboards/AppMenu Would be nice to make it look like that. Also, appears that the pressed state for the activities button and the icon in the app menu overhang one pixel below the top bar. Seems to be a regression from master. The clock isn't clickable from the top screen edge.
(In reply to comment #14) > Also, appears that the pressed state for the activities button and the icon in > the app menu overhang one pixel below the top bar. Seems to be a regression > from master. I can confirm that in master the "pressed state" goes beyond the top panel, this happends for both the Activities button and the clock. So not a regression.
*** Bug 601012 has been marked as a duplicate of this bug. ***
(In reply to comment #8) > > Have you checked what happens with this on a couple of apps (say the GIMP and > in openoffice) if you have multiple unsaved windows open? In gimp you get a confirm dialog per window. Dunno, the only other option I see is to not offer quit in the case where there are multiple windows, no XSMP, and no implementation of org.fd.o.Application.
(In reply to comment #9) > This should be using the new StWidget hover support. Fixed. > > @@ +182,3 @@ > + _getPreferredWidth: function(actor, forHeight, alloc) { > + let [minInternalSize, natInternalSize] = > this._box.get_preferred_width(forHeight); > + let sourceAlloc = this._sourceButton.allocation; > > Hmm, while this looks innocuous, this needs a comment like: > > // The allocation property unlike get_allocation_box() doesn't trigger > // synchronous relayout, which would be bad here. We assume that the > // previous allocation of the button is fine. Yeah...though I suppose we don't quite handle application focus changes while the menu is up. > Odd variable name Fixed. > > @@ +233,3 @@ > + > + _init: function(menuAlignment) { > + if (!menuAlignment) > > GJS enums are integers, if I remmeber correctly. This was intended to make the parameter optional, but that was silly since there was only one other caller; I just made it mandatory. > Needs to be using the new hover functionality in StWidget. Done. > Are menus that slide out what we want? Dunno. I removed it. > @@ +296,3 @@ > + let [stageX, stageY] = this.actor.get_transformed_position(); > + let [buttonWidth, buttonHeight] = this.actor.get_transformed_size(); > + stageX = Math.floor(stageX); > > Doesn't make sense to floor this but assume other quantities below are > integral. Fair enough. > @@ +353,3 @@ > + reactive: true }); > + // Note this variable is read by Panel > + this._isMenuOpen = false; > > Don't _prefix it then, it's fine as a public property. Subclasses are also ways > of consuming an API. (I.e., C++ protected is stupid.) Ok changed. > > @@ +357,3 @@ > + // these are more "private" > + this._eventCaptureId = 0; > + this._activeMenu = null; > > Should be activeMenuButton Done. > > @@ +365,3 @@ > + button.actor.connect('enter-event', Lang.bind(this, this._onMenuEnter, > button)); > + button.actor.connect('leave-event', Lang.bind(this, this._onMenuLeave, > button)); > + button.actor.connect('button-press-event', Lang.bind(this, > this._onMenuPress, button)); > > Think these connections are poking into the internals of another actor, better > to make the menu button handle more itself. I tried it the other way and it was even more hairy - having all of the event processing logic inside one class was easier. > I feel pretty much backwards from your comment on a recent bug, and think that > this would read better if this was: > > if (this._isMenuOpen && button != this._activeMenu) { > } > > because it we are actively doing something in the unusual case when mouse > pointer enters a different menu button, not just optimizing out something. Mmmmmm...can I take a pass on this one? > Think this should be driven off the open state of the menus, not direct event > conenctions. I'll have to look at this later. > @@ +400,3 @@ > + }, > + > + _containsActor: function(container, actor) { > > We really should get this added to ClutterActor() or ClutterContainer(), we > keep rewriting it all over the place. Agreed. > @@ +429,3 @@ > + this._activeMenu.close(); > + this._disconnectCapture(); > + return true; > > This is not right, you are going to get prelight (and mousewheel scrolling, > etc) outside of the menus, but not clicks working. True, fixed. > > @@ +434,3 @@ > + }, > + > + _disconnectCapture: function() { > > A method called disconnectCapture shouldn't do random other stuff without being > renamed. Fixed. > @@ +568,3 @@ > + app.request_quit_finish(); > + } catch (e) { > + // FIXME - add some sort of force quit dialog > > Won't you already get the Metacity force quite dialog (possibly multiple > times?) Yes, but we need to take over that dialog anyways. > @@ +995,3 @@ > _onHotCornerEntered : function() { > + if (this._isMenuOpen) > + return false; > > Think this better off inside where we actually trigger the overview instead of > guarding the enter/leave handling. Otherwise closing the menu + mouse jiggle > could trigger the overview. Ok, I'll add this to the todo list.
(In reply to comment #18) > > > @@ +995,3 @@ > > _onHotCornerEntered : function() { > > + if (this._isMenuOpen) > > + return false; > > > > Think this better off inside where we actually trigger the overview instead of > > guarding the enter/leave handling. Otherwise closing the menu + mouse jiggle > > could trigger the overview. > > Ok, I'll add this to the todo list. This one actually depends a bit on how we want this to work - should clicking on Activities when the menu is up close the menu and also trigger the overview? I don't think though that accidental trigger is a real concern; that'd be a quite large jiggle to go from the menu all the way to the hot corner.
Created attachment 160343 [details] [review] Add quit method Closes the app, will be used for the panel app menu.
Created attachment 160344 [details] [review] New widget: BoxPointer Implement a rounded box with an arrow pointer.
Created attachment 160345 [details] [review] [chrome] Add snapToColumns parameter This allows adding a chrome actor whose width will be at least a given number of stage columns.
Created attachment 160346 [details] [review] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
Created attachment 160347 [details] [review] Convert calendar to menu item
Review of attachment 160343 [details] [review]: The patch looks like you planned on using the GIO Async API, changed your mind, and ended up with some unused left-overs :) ::: src/shell-app.h @@ +51,3 @@ +gboolean shell_app_request_quit_finish (ShellApp *app, GAsyncResult *result, GError **error); + +gboolean shell_app_request_quit (ShellApp *app); The first function is not asynchronous and the second one is not implemented?
(In reply to comment #19) > (In reply to comment #18) > > > > > @@ +995,3 @@ > > > _onHotCornerEntered : function() { > > > + if (this._isMenuOpen) > > > + return false; > > > > > > Think this better off inside where we actually trigger the overview instead of > > > guarding the enter/leave handling. Otherwise closing the menu + mouse jiggle > > > could trigger the overview. > > > > Ok, I'll add this to the todo list. > > This one actually depends a bit on how we want this to work - should clicking > on Activities when the menu is up close the menu and also trigger the overview? Hmm, since the activities button doesn't trigger when you just mouse over it, I think it should be behave like clicking outside the menu - menu goes away but the click is eaten. > I don't think though that accidental trigger is a real concern; that'd be a > quite large jiggle to go from the menu all the way to the hot corner. The concern here is if the menu was triggered from the keyboard (presumably there will be a way to do that eventually.)
(In reply to comment #18) > > @@ +365,3 @@ > > + button.actor.connect('enter-event', Lang.bind(this, this._onMenuEnter, > > button)); > > + button.actor.connect('leave-event', Lang.bind(this, this._onMenuLeave, > > button)); > > + button.actor.connect('button-press-event', Lang.bind(this, > > this._onMenuPress, button)); > > > > Think these connections are poking into the internals of another actor, better > > to make the menu button handle more itself. > > I tried it the other way and it was even more hairy - having all of the event > processing logic inside one class was easier. I still don't like it much, but if you gave it a try the other way and it didn't work out, we can leave it this way. > > I feel pretty much backwards from your comment on a recent bug, and think that > > this would read better if this was: > > > > if (this._isMenuOpen && button != this._activeMenu) { > > } > > > > because it we are actively doing something in the unusual case when mouse > > pointer enters a different menu button, not just optimizing out something. > > Mmmmmm...can I take a pass on this one? Yeah, point of reviews isn't to enforce the reviewers taste on the reviewee, it's to come to a consensus on taste when possible :-) (and to catch bugs, of course)
Review of attachment 160343 [details] [review]: No comments ohter than what Florian said.
(In reply to comment #25) > Review of attachment 160343 [details] [review]: > > The patch looks like you planned on using the GIO Async API, changed your mind, > and ended up with some unused left-overs :) The unused leftover was just in the .h file; I've deleted it now, thanks!
Review of attachment 160344 [details] [review]: Looks basically right. Various comments. ::: data/theme/gnome-shell.css @@ +94,3 @@ } +.boxpointer { All boxpointers have the same color? Seems this needs to be specific to, say, the box around the menu. ::: js/ui/boxpointer.js @@ +11,3 @@ + * side. At present only St.Side.TOP is implemented. Call getBox() + * to access a container in which content can be placed. The arrow + * position may be controlled via setArrowOrigin(). Should explain what the source actor is and the constraint on size. @@ +18,3 @@ + +BoxPointer.prototype = { + _init: function(arrowSide, sourceActor) { if (arrowSide != ...TOP) throw new Error('Only St.Side.TOP currently supported'); @@ +34,3 @@ + let orientationString = + (arrowSide == St.Side.LEFT || arrowSide == St.Side.RIGHT) ? + "horizontal" : "vertical"; Unused @@ +50,3 @@ + [found, rise] = themeNode.get_length('-arrow-rise', false); + if ((!isWidth && (this._arrowSide == St.Side.TOP || this._arrowSide == St.Side.BOTTOM)) + || (isWidth && (this._arrowSide == St.Side.LEFT || this._arrowSide == St.Side.RIGHT))) { themeNode.get_length() is moderately expensive - I'd avoid move the rise computation inside here and not get the base if you aren't going to use it. @@ +95,3 @@ + childBox.x2 = availWidth - borderWidth; + childBox.y2 = availHeight - borderWidth; + break; Misindendation @@ +97,3 @@ + break; + default: + log("not implemented"); Don't think you need to fill up the log with log spew - just catch it once when set. @@ +126,3 @@ + } + let cr = area.get_context(); + cr.setOperator(Cairo.Operator.OVER); Not necessary @@ +149,3 @@ + cr.translate(borderWidth, borderWidth); + cr.scale((boxWidth - 2 * borderWidth) / boxWidth, (boxHeight - 2 * borderWidth) / boxHeight); + cr.appendPath(path); Hmm, this has some nice properties, and is clever, but in the end, I think it's not good cairo practice. (Draws all the pixels in the interior twice ,for one thing) set the lineWidth to borderWidth, add appropriate '+/- borderWidth / 2' above, and then do: set_color(backgroundColor); fill_preserve() set_color(borderColor); stroke(); (this isn't theoretically *perfect* if the border is a 1 pixel border, but it is quite close to the exact result) @@ +154,3 @@ + cr.restore(); + if (this._arrowSide == St.Side.TOP) { + cr.translate(borderRadius, -rise); Think I'd use the same fill_preserve()/stroke() path here. Note that if you fill_preserve() then stroke() an unclosed path, the fill() will automatically close the path, but the stroke will only surround the unclosed portion. (I think that's useful for what you are drawing.) @@ +173,3 @@ + getBin: function() { + return this._bin; + }, Our style is 'this.bin' - don't need getters for simple properties. @@ +175,3 @@ + }, + + // @origin: Coordinate specifying middle of the arrow Coordinate system for this coordinate?
Review of attachment 160347 [details] [review]: Unclear talking to Jon if we want this, my take is no: - Calendar is an instance of "quick look" operations, not "do an action" operations. (Related to Gizmos question) - Useful to look at the clock and leave it open when typing in another application - Clock doesn't act like a menu because clicking on a date does nothing rather than dismiss. Will need revisiting later depending on where we go with "gizmos", weather, etc.
Review of attachment 160346 [details] [review]: Some more comments. Trying it out: - Probably with the entire menu getting a black background I showed you (conceivably a bug in the BoxPointer patch instead) - You need to handle Esc to cancel - Deactivation on selection is buggy (see comment below) ::: js/ui/panel.js @@ +141,3 @@ + this.actor.add_style_pseudo_class('hover'); + else + this.actor.remove_style_pseudo_class('hover'); This is handled by St.Widget already as far as I can determine @@ +203,3 @@ + track_hover: true }); + this.actor.connect('button-press-event', Lang.bind(this, this._onButtonPress)); + this.actor.connect('notify::hover', Lang.bind(this, this._onHoverChanged)); This is handled by St.Widget @@ +204,3 @@ + this.actor.connect('button-press-event', Lang.bind(this, this._onButtonPress)); + this.actor.connect('notify::hover', Lang.bind(this, this._onHoverChanged)); + this.actor.connect('notify::allocation', Lang.bind(this, this._repositionMenu)); This is going to trigger warnings, I'm pretty sure. It's just not right to do this in Clutter terms. We don't have a solution :-( ,for something that comes up frequently. Here it probably doesn't matter if we really handle panel menu buttons moving when the menu is up. @@ +221,3 @@ + let panelActor = Main.panel.actor; + + this.menu.actor.y = Math.floor(panelActor.y + panelActor.height); Why not have this in this._repositionMenu? @@ +226,3 @@ + this._repositionMenu(); + + this.actor.set_style_pseudo_class('pressed'); Better to use add_style_class, then the CSS can do: menu-button:hover:pressed @@ +227,3 @@ + + this.actor.set_style_pseudo_class('pressed'); + this.emit('open-state', true); open-state isn't a good signal name. open-state-changed, or just have two signals opened/closed @@ +244,3 @@ + let primary = global.get_primary_monitor(); + let [stageX, stageY] = this.actor.get_transformed_position(); + let [buttonWidth, buttonHeight] = this.actor.get_transformed_size(); clearer as let [buttonX, buttonY] = this.actor.get_transformed_position(); let buttonX = stageX; Rather having the weird disconnect where stageX corresponds to buttonWidth as the position/size of the button, then it is mutated into the X position of the menu. @@ +245,3 @@ + let [stageX, stageY] = this.actor.get_transformed_position(); + let [buttonWidth, buttonHeight] = this.actor.get_transformed_size(); + let [minWidth, prefWidth] = this.menu.actor.get_preferred_width(-1); naturalWidth not prefWidth @@ +254,3 @@ + break; + } + stageX = Math.max(primary.x, stageX); Clamping it onto the monitor only from the left and not from the right doesn't make sense @@ +354,3 @@ + }, + + _eventIsOnAnyMenu: function(event) { OnAnyMenu*Button* @@ +364,3 @@ + }, + + _onEventCapture: function(actor, event) { Logic isn't quite right here - you stay in the menu state after a menu item is selected. Think that has to be driven by the menu - probably it should emit a different signal when closing because you called close() when entering a different menu and closing because an item was selected. @@ +410,3 @@ +AppMenu.prototype = { + __proto__: PanelMenuButton.prototype, IMO, AppMenu deriving from PanelMenuButton is confusing. If it's a a MenuButton, it should be AppMenuButton. If it is AppMenu, then it should derive from PanelMenu. @@ -141,4 +423,4 @@ - this.actor.set_child(this._container); - this._container.connect('get-preferred-width', Lang.bind(this, this._getPreferredWidth)); - this._container.connect('get-preferred-height', Lang.bind(this, this._getPreferredHeight)); - this._container.connect('allocate', Lang.bind(this, this._allocate)); + bin.set_child(this._container); + this._container.connect('get-preferred-width', Lang.bind(this, this._getAppMenuPreferredWidth)); + this._container.connect('get-preferred-height', Lang.bind(this, this._getAppMenuPreferredHeight)); + this._container.connect('allocate', Lang.bind(this, this._appMenuAllocate)); The rename of these menus to 'appMenu" is confusing, since this *isn't* the sizing of the menu but the sizing of the contents of the button. If you need to prevent a clash of method names with a parent class or something, would suggest *Contents*
(In reply to comment #32) > Review of attachment 160346 [details] [review]: > > Some more comments. Trying it out: > > - Probably with the entire menu getting a black background I showed you > (conceivably a bug in the BoxPointer patch instead) Not sure on this one...if you can reproduce it reliably let me know. > - You need to handle Esc to cancel Fixed. > - Deactivation on selection is buggy (see comment below) > > ::: js/ui/panel.js > @@ +141,3 @@ > + this.actor.add_style_pseudo_class('hover'); > + else > + this.actor.remove_style_pseudo_class('hover'); > > This is handled by St.Widget already as far as I can determine Fixed both. > @@ +204,3 @@ > + this.actor.connect('button-press-event', Lang.bind(this, > this._onButtonPress)); > + this.actor.connect('notify::hover', Lang.bind(this, > this._onHoverChanged)); > + this.actor.connect('notify::allocation', Lang.bind(this, > this._repositionMenu)); > > This is going to trigger warnings, I'm pretty sure. It's just not right to do > this in Clutter terms. We don't have a solution :-( ,for something that comes > up frequently. Here it probably doesn't matter if we really handle panel menu > buttons moving when the menu is up. Yeah...I'll add a FIXME I guess. > > @@ +221,3 @@ > + let panelActor = Main.panel.actor; > + > + this.menu.actor.y = Math.floor(panelActor.y + panelActor.height); > > Why not have this in this._repositionMenu? Fixed. > > @@ +226,3 @@ > + this._repositionMenu(); > + > + this.actor.set_style_pseudo_class('pressed'); > > Better to use add_style_class, then the CSS can do: > > menu-button:hover:pressed Fixed. > > @@ +227,3 @@ > + > + this.actor.set_style_pseudo_class('pressed'); > + this.emit('open-state', true); > > open-state isn't a good signal name. open-state-changed, or just have two > signals opened/closed Done. > Rather having the weird disconnect where stageX corresponds to buttonWidth as > the position/size of the button, then it is mutated into the X position of the > menu. Fixed. > @@ +245,3 @@ > + let [stageX, stageY] = this.actor.get_transformed_position(); > + let [buttonWidth, buttonHeight] = this.actor.get_transformed_size(); > + let [minWidth, prefWidth] = this.menu.actor.get_preferred_width(-1); > > naturalWidth not prefWidth > > @@ +254,3 @@ > + break; > + } > + stageX = Math.max(primary.x, stageX); > > Clamping it onto the monitor only from the left and not from the right doesn't > make sense Ok, I moved clamping to the primary into the chrome.js patch. > @@ +354,3 @@ > + }, > + > + _eventIsOnAnyMenu: function(event) { > > OnAnyMenu*Button* Ok. > @@ +364,3 @@ > + }, > + > + _onEventCapture: function(actor, event) { > > Logic isn't quite right here - you stay in the menu state after a menu item is > selected. Think that has to be driven by the menu - probably it should emit a > different signal when closing because you called close() when entering a > different menu and closing because an item was selected. I can't reproduce this - adding log messages the menu correctly goes to closed state after an item activation. > @@ +410,3 @@ > > +AppMenu.prototype = { > + __proto__: PanelMenuButton.prototype, > > IMO, AppMenu deriving from PanelMenuButton is confusing. If it's a a > MenuButton, it should be AppMenuButton. If it is AppMenu, then it should derive > from PanelMenu. Changed. > @@ -141,4 +423,4 @@ > - this.actor.set_child(this._container); > - this._container.connect('get-preferred-width', Lang.bind(this, > this._getPreferredWidth)); > - this._container.connect('get-preferred-height', Lang.bind(this, > this._getPreferredHeight)); > - this._container.connect('allocate', Lang.bind(this, this._allocate)); > + bin.set_child(this._container); > + this._container.connect('get-preferred-width', Lang.bind(this, > this._getAppMenuPreferredWidth)); > + this._container.connect('get-preferred-height', Lang.bind(this, > this._getAppMenuPreferredHeight)); > + this._container.connect('allocate', Lang.bind(this, > this._appMenuAllocate)); > > The rename of these menus to 'appMenu" is confusing, since this *isn't* the > sizing of the menu but the sizing of the contents of the button. If you need to > prevent a clash of method names with a parent class or something, would suggest > *Contents* Changed.
Created attachment 160540 [details] [review] [ShellApp] Add quit method Closes the app, will be used for the panel app menu. Note this is just a very primitive implementation.
Created attachment 160541 [details] [review] New widget: BoxPointer Implement a rounded box with an arrow pointer.
Created attachment 160542 [details] [review] [chrome] Add snapToColumns,constrainToPrimaryMonitor parameters snapToColumns allows adding a chrome actor whose width will be at least a given number of stage columns. constrainToPrimaryMonitor allows us to pop up actors and ensure that they're wholly on the monitor.
Created attachment 160543 [details] [review] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
(In reply to comment #34) > > Logic isn't quite right here - you stay in the menu state after a menu item is > > selected. Think that has to be driven by the menu - probably it should emit a > > different signal when closing because you called close() when entering a > > different menu and closing because an item was selected. > > I can't reproduce this - adding log messages the menu correctly goes to > closed state after an item activation. I mean the menubar stays in the "menu active" state. After you click on the Quit menu item, if you mouse over the menu item again the menu will pop up without clicking. If you click somewhere else on the screen, the next click is eaten.
Created attachment 160597 [details] [review] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
What changed in the last set of patches?
(In reply to comment #41) > What changed in the last set of patches? I followed up for all changes in the comments. If I didn't reply to a comment or mention it - nothing changed, I'm reattaching to keep the order in sync.
(In reply to comment #42) > (In reply to comment #41) > > What changed in the last set of patches? > > I followed up for all changes in the comments. If I didn't reply to a comment > or mention it - nothing changed, I'm reattaching to keep the order in sync. I don't want to be a pain here, but that doesn't help me much. I'm being asked to decipher 4 patches, dozens of comments from you, Florian and myself, and try to figure out what you changed, what you didn't, what you still think needs review, etc. Review cycles are our single most constrained resource, and I think it makes sense for the submitter to try to make it easy on the reviewer and write up what patches changed, what didn't, what you still think needs review. If a patch was accepted-commit-now or needs-work "OK to commit with the following changes", then the submitter reattaches it, they should mark the newly attached patch accepted-commit-now themselves. I suppose in the ideal world maybe the reviewer would take nothing on face value, and check everything themselves, but that doesn't really correspond to the reality of the resources we have available or what we are trying to achieve with our review policy.
Created attachment 160720 [details] [review] [chrome] Add snapToColumns,constrainToPrimaryMonitor parameters snapToColumns allows adding a chrome actor whose width will be at least a given number of stage columns. constrainToPrimaryMonitor allows us to pop up actors and ensure that they're wholly on the monitor.
Created attachment 160721 [details] [review] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
Created attachment 160722 [details] [review] [panel] Port user status menu to panel menu To avoid circular prototype dependencies between panel.js and statusMenu.js, we need to merge statusMenu.js into panel.js. This is less invasive now that the DBus code to interact with gnome-session has been split into js/misc/gnomeSession.js.
The update to chrome.js makes the monitor constraining actually work =) The update to panel.js: * Adds PanelImageMenuItem, PanelSeparatorMenuItem in preparation for the status menu patch * Reworks the arrow positioning so that it's adjusted after chrome.js allocates, so we draw in the correct place if we were moved by chrome allocation
Review of attachment 160720 [details] [review]: (owen asked me to review this one) Why did you decide to put this in Chrome rather than just having the panel menu adjust its own size and position? "snapToColumns" in particular really seems like it's entirely an implementation detail of the panel, not something generically useful for Chrome. constrainToPrimary monitor could maybe be useful for other things, but at the same time, Chrome currently just allocates its children wherever they want to be, so there's no reason they can't just implement that themselves. And they might not want to do it the same way constrainToPrimary does. Eg: ::: js/ui/chrome.js @@ +71,3 @@ + proposedRect.width = proposedBox.x2 - proposedBox.x1; + proposedRect.height = proposedBox.y2 - proposedBox.y1; + // If this isn't on the primary at all, just allocate it as proposed why? why not move it entirely to the primary? @@ +90,3 @@ + } + if (constrainedHeight < natHeight) { + box.y1 -= Math.min(availTop, natHeight - constrainedHeight); if the panel menu were actually going to be taller than the screen, wouldn't we want it to scroll internally, rather than shifting up to cover the panel?
(In reply to comment #48) > Review of attachment 160720 [details] [review]: > > (owen asked me to review this one) > > Why did you decide to put this in Chrome rather than just having the panel menu > adjust its own size and position? "snapToColumns" in particular really seems > like it's entirely an implementation detail of the panel, not something > generically useful for Chrome. Admittedly the column snapping was something I just sort of made up...we clearly need better heuristics for sizing things. I think elsewhere percentage widths were suggested...not sure. Now, we could just say that the menu should itself look at Main.overview.getDisplayGridColumnWidth() ? > constrainToPrimary monitor could maybe be useful for other things, but at the > same time, Chrome currently just allocates its children wherever they want to > be, so there's no reason they can't just implement that themselves. Well with the patch x/y become suggestions. > And they > might not want to do it the same way constrainToPrimary does. Eg: True, I guess. If we were redoing say tooltips we'd probably want monitor constraint but also allow them to go over the panel. Alright, I'll just move this back into panel.js then.
Created attachment 160740 [details] [review] Make panel more menu-like, initial application menu sizing into panel.js itself.
Created attachment 160741 [details] [review] New widget: BoxPointer Implement a rounded box with an arrow pointer.
Latest panel patch moves sizing back into panel.js, we don't modify chrome.js anymore. The boxpointer patch just fixes one drawing bug with the border.
Review of attachment 160540 [details] [review]: Looks good to commit. Does likely need further work before GNOME 3.0 - if you try it with the GIMP having all your saved windows vanish, and your unsaved windows all prompt separately is not a good user experience.
Review of attachment 160741 [details] [review]: ::: js/ui/boxpointer.js @@ +10,3 @@ + * @side: A St.Side type; currently only St.Side.TOP is implemented + * @sourceActor: A Clutter.Actor which is used for sizing; the pointer + * will not be smaller than this. "Not be smaller" doesn't really make sense. Do you mean something like: "The side of the BoxPointer where the pointer is is constrained to be as big as the corresponding dimension of @sourceActor. E.g. for St.Side.TOP, the BoxPointer is horizontally constrained to be bigger than @sourceActor" @@ +51,3 @@ + if ((!isWidth && (this._arrowSide == St.Side.TOP || this._arrowSide == St.Side.BOTTOM)) + || (isWidth && (this._arrowSide == St.Side.LEFT || this._arrowSide == St.Side.RIGHT))) { + [found, rise] = themeNode.get_length('-arrow-rise', false); The let should be moved in here for clarity as well. @@ +131,3 @@ + cr.translate(0, rise); + } + cr.moveTo(borderRadius, 0); Note that your border is half-obscured on the sides and thicker on the corners. You need to inset the path by borderWidth / 2 @@ +153,3 @@ + let halfBase = Math.floor(base/2); + cr.moveTo(this._arrowOrigin - halfBase, rise + borderWidth); + cr.lineTo(this._arrowOrigin, 0); Need borderWidth/2 instead of 0 here too - the tip of the arrow is chopped off.
Review of attachment 160741 [details] [review]: ::: js/ui/boxpointer.js @@ +167,3 @@ + // @origin: Coordinate specifying middle of the arrow, along + // the Y axis for St.Side.LEFT, St.Side.RIGHT and X axis + // for St.Side.TOP and St.Side.BOTTOM. Oh, one more comment: This doesn't really answer my question - what is this relative too? Where is the origin? Is it something like: Position of the arrow as an offset from the left (top and bottom pointers) or top (left and right pointers) of the BoxPointer widget. ?
Review of attachment 160740 [details] [review]: Just a few more comments. ::: js/ui/panel.js @@ +150,3 @@ + _init: function (text) { + this.actor = new St.Bin({}); + this.actor.set_child(new St.Bin({ style_class: 'panel-separator-menu-item' })); Either you shouldn't add this class or you should add the CSS - someone is going to be scratching their head about why their SeparatorMenuItem is doing nothing. @@ +165,3 @@ + track_hover: true }); + this.actor.add(St.TextureCache.get_default().load_icon_name(iconName, 16), { y_align: St.Align.MIDDLE }); + this.actor.add(new St.Label({ text: text }), { expand: true }); This isn't going to work if you mix image and non-image menu items, but we can cross that bridge when we come to it. @@ +183,3 @@ + this._boxPointer = new BoxPointer.BoxPointer(St.Side.TOP, this._sourceButton); + this.actor = this._boxPointer.actor; + this.actor.style_class = 'panel-menu-boxpointer'; Would have been fine to do this as '.panel-menu > .box-pointer' - there aren't ever going to be so many box-pointers that the inefficiency of matching on .box-pointer matters. But this is OK too. @@ +187,3 @@ + vertical: true }); + let bin = this._boxPointer.bin; + bin.x_align = St.Align.START; Ugh. @@ +203,3 @@ + this._box.add(menuItem.actor); + menuItem.connect('activated', Lang.bind(this, function (menuItem, event) { + this.emit('activated'); This results in us closing the menu *after* the actual action of the menu item. I can't think of anything specific that will break - since you are inside the compositor the reason that this was bad for GTK+ - stopping with the pointer/grabbed - doesn't matter. But maybe still worth adding a second signal to PanelMenuItem to get things ordered right. @@ +273,3 @@ + }, + + _repositionMenu: function() { Please add strategic blank lines and a few high level comments about everything that is going on in here. @@ +292,3 @@ + let overflowRight = Math.max(0, (stageX + natWidth) - (primary.x + primary.width)); + if (availLeft > 0 && overflowRight > 0) { + stageX -= Math.min(availLeft, overflowRight); I think trying to handle menu wider than the screen (as I understand this doing) is a bit silly, but if you do want to do that, I think it's simpler to do it with ordering: stageX = Math.min(stageX, primary.x + primary.width - natWidth); stageX = Math.max(stageX, primary.x); @@ +298,3 @@ + this.menu.actor.width = Math.min(natWidth, (primary.x + primary.width) - stageX); + this.menu.actor.y = Math.floor(panelActor.y + panelActor.height); + this.menu.actor.height = Math.min(natHeight, (primary.y + primary.height) - this.menu.actor.y); If you don't add scrolling, then I don't see any point in clamping the size like this, just let it overflow and save the code.
Review of attachment 160722 [details] [review]: ::: data/theme/gnome-shell.css @@ +150,3 @@ +} + +.panel-separator-menu-item { These changes should be in the other patch ::: js/ui/panel.js @@ +597,3 @@ +// Copyright (C) 2008,2009 Red Hat, Inc. + +function StatusMenuButton() { Let's keep this in statusMenu.js - even without this panel.js is over 1000 lines long. I'll do the detailed review that way since the diff will be more meaningful.
(In reply to comment #57) > ::: js/ui/panel.js > @@ +597,3 @@ > +// Copyright (C) 2008,2009 Red Hat, Inc. > + > +function StatusMenuButton() { > > Let's keep this in statusMenu.js - even without this panel.js is over 1000 > lines long. I'll do the detailed review that way since the diff will be more > meaningful. I don't know a way to do that - if panel.js imports statusmenu.js, and statusmenu.js imports panel.js, we can't refer to prototypes from panel.js (like the menu prototypes) inside statusmenu.js.
(In reply to comment #58) > (In reply to comment #57) > > > ::: js/ui/panel.js > > @@ +597,3 @@ > > +// Copyright (C) 2008,2009 Red Hat, Inc. > > + > > +function StatusMenuButton() { > > > > Let's keep this in statusMenu.js - even without this panel.js is over 1000 > > lines long. I'll do the detailed review that way since the diff will be more > > meaningful. > > I don't know a way to do that - if panel.js imports statusmenu.js, and > statusmenu.js imports panel.js, we can't refer to prototypes from panel.js > (like the menu prototypes) inside statusmenu.js. Various ways, simplest, when you *create* the button, do something like: // Can't use global import because statusMenu.js derives from classes // in this file let StatusMenu = imports.gi.statusMenu; menuButton = new StatusMenu.StatusMenuButton();
or move PanelMenuButton outside of panel.js
(In reply to comment #56) > Review of attachment 160740 [details] [review]: > > Just a few more comments. > > ::: js/ui/panel.js > @@ +150,3 @@ > + _init: function (text) { > + this.actor = new St.Bin({}); > + this.actor.set_child(new St.Bin({ style_class: > 'panel-separator-menu-item' })); > > Either you shouldn't add this class or you should add the CSS - someone is > going to be scratching their head about why their SeparatorMenuItem is doing > nothing. Fixed. > @@ +165,3 @@ > + track_hover: true }); > + this.actor.add(St.TextureCache.get_default().load_icon_name(iconName, > 16), { y_align: St.Align.MIDDLE }); > + this.actor.add(new St.Label({ text: text }), { expand: true }); > > This isn't going to work if you mix image and non-image menu items, but we can > cross that bridge when we come to it. I didn't change this. > Would have been fine to do this as '.panel-menu > .box-pointer' - there aren't > ever going to be so many box-pointers that the inefficiency of matching on > .box-pointer matters. But this is OK too. I didn't change this. > > @@ +187,3 @@ > + vertical: true }); > + let bin = this._boxPointer.bin; > + bin.x_align = St.Align.START; > > Ugh. Ok, I changed this to pass in a property map. > > @@ +203,3 @@ > + this._box.add(menuItem.actor); > + menuItem.connect('activated', Lang.bind(this, function (menuItem, > event) { > + this.emit('activated'); > > This results in us closing the menu *after* the actual action of the menu item. > I can't think of anything specific that will break - since you are inside the > compositor the reason that this was bad for GTK+ - stopping with the > pointer/grabbed - doesn't matter. But maybe still worth adding a second signal > to PanelMenuItem to get things ordered right. I just reversed the order of the signal connections to activate. > > @@ +273,3 @@ > + }, > + > + _repositionMenu: function() { > > Please add strategic blank lines and a few high level comments about everything > that is going on in here. Ok. > @@ +292,3 @@ > + let overflowRight = Math.max(0, (stageX + natWidth) - (primary.x + > primary.width)); > + if (availLeft > 0 && overflowRight > 0) { > + stageX -= Math.min(availLeft, overflowRight); > > I think trying to handle menu wider than the screen (as I understand this > doing) is a bit silly, but if you do want to do that, I think it's simpler to > do it with ordering: Not wider than the screen specifically (though that is handled). What I'm trying to handle here is that the combination of normal X + natWidth is past the right of the screen. > @@ +298,3 @@ > + this.menu.actor.width = Math.min(natWidth, (primary.x + primary.width) > - stageX); > + this.menu.actor.y = Math.floor(panelActor.y + panelActor.height); > + this.menu.actor.height = Math.min(natHeight, (primary.y + > primary.height) - this.menu.actor.y); > > If you don't add scrolling, then I don't see any point in clamping the size > like this, just let it overflow and save the code. Ok.
Created attachment 160751 [details] [review] New widget: BoxPointer Implement a rounded box with an arrow pointer.
This just fixes up for the comments.
Created attachment 160752 [details] [review] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
Review of attachment 160751 [details] [review]: ::: js/ui/boxpointer.js @@ +159,3 @@ + cr.moveTo(this._arrowOrigin - halfBase, rise + borderWidth); + cr.lineTo(this._arrowOrigin, halfBorder); + cr.lineTo(this._arrowOrigin + halfBase, rise + borderWidth); These computations aren't right, and you can see it if you look closely at the base of the arrow - there's a discontinuity. I can't really ascii-art this or describe it in words, so come over and look at my whiteboard. The easiest way to fix this is to draw the arrow and the center fill as one object that is filled and stroked rather than trying to merge separate pieces together.
Created attachment 160760 [details] [review] New widget: BoxPointer Implement a rounded box with an arrow pointer.
Ok, drawn as one object.
Created attachment 160763 [details] [review] Make panel more menu-like, initial application menu Change the panel into a menu-like actor, where elements can add menu items. Implement the initial application menu.
Created attachment 160764 [details] [review] [panel] Port user status menu to panel menu
The updated panel patch adds a few fixes that were accidentally squashed into the user status patch. The user status patch now keeps the statusMenu.js file.
Review of attachment 160760 [details] [review]: Looks good. ::: js/ui/boxpointer.js @@ +138,3 @@ + cr.moveTo(borderRadius, halfBorder); + if (this._arrowSide == St.Side.TOP) { + cr.translate(0, -rise); Don't think this actually simplifies things any, but ok.
Review of attachment 160763 [details] [review]: Just a few things left. OK to commit with these fixes. ::: js/ui/overview.js @@ +417,3 @@ + // connect to 'relayout'. + getColumnWidth : function() { + return displayGridColumnWidth; Leftover. ::: js/ui/panel.js @@ +201,3 @@ + }, + + addMenuItem: function(menuItem) { Ordering problem is still there for people using this directly unless they use non-intuitive code, but lets just ignore until its a problem. @@ +300,3 @@ + stageX = Math.max(primary.x, stageX); + + // Now see if we're over the right edge; if so, shift left (if possible) Just shifting left if over the right edge, then shifting right if over the left edge should get the same effect without the complication of the "if possible" @@ +310,3 @@ + let panelActor = Main.panel.actor; + this.menu.actor.x = stageX; + this.menu.actor.width = Math.min(natWidth, (primary.x + primary.width) - stageX); Don't see a point in worrying about menus bigger than the screen. I doubt the result is any more attractive or usable than just overflowing, and it creates the need to do the unset/reset dance here.
Review of attachment 160764 [details] [review]: Just a couple of minor things, OK to commit with fixes. ::: js/ui/panel.js @@ +801,3 @@ this._traymanager.manage_stage(global.stage); + // We need to do this here to avoid a circular import dependency (note that circular import dependencies are only bad when inheritance is involved - we have them all over the place in other circumstances.) ::: js/ui/statusMenu.js @@ +1,1 @@ const GLib = imports.gi.GLib; You lost the emacs modeline @@ +87,2 @@ + item = new Panel.PanelImageMenuItem(_("Available"), 'gtk-yes'); + item.connect('activated', Lang.bind(this, this._setPresenceStatus, GnomeSession.PresenceStatus.AVAILABLE)); Hmm, didn't notice the vs. GTK+ difference of 'activate' vs. 'activated' in the other patch. I think this can be read as a command "do the activate thing" rather than a notification, so it might be nice to stick with the GTK+ convention. Also, looking elsewhere in our JS code, there are multiple instances of 'activate' but only MoreLink has 'activated'
Attachment 160540 [details] pushed as 78e3126 - [ShellApp] Add quit method Attachment 160760 [details] pushed as 47dae08 - New widget: BoxPointer Attachment 160763 [details] pushed as fdcb73d - Make panel more menu-like, initial application menu Attachment 160764 [details] pushed as 27bcce0 - [panel] Port user status menu to panel menu