GNOME Bugzilla – Bug 602976
Right click on places
Last modified: 2009-12-18 20:11:31 UTC
Currently when I need to unmount or eject some drive I need: - Create a new workspace, rightclick on drive on desktop and choose unmount - Open nautilus, choose Go->Computer, rightclick on drive and choose unmount - Minimalize everything, rightclick on drive and choose unmount However I should be able to just rightclick in gnome-shell left panel.
Created attachment 148694 [details] [review] Add eject buttons in places menu for places which support it This patch depends on a solution for https://bugzilla.gnome.org/show_bug.cgi?id=563025.
Review of attachment 148694 [details] [review]: Cool, thanks! Will conflict with my search patch =( But I'll deal with that after it lands. Some small comments follow. ::: js/ui/placeDisplay.js @@ +54,3 @@ + isRemovable: function() { + + }, We have this more standard at the top, like: PlaceDeviceInfo.prototype = { __proto__: PlaceInfo.prototype, _init: function () { ... @@ +61,3 @@ + + iconFactory: function(size) { + let icon=this._mount.get_icon(); Spaces around = @@ +79,3 @@ + + //this._mount.unmount(0, null, Lang.bind(this, this._removeFinish)); + this._mount.unmount(0, null, this._removeFinish, null); Why the commented out version? @@ +357,1 @@ let text = new St.Label({ style_class: 'places-item', This should be a St.Button; connect to 'clicked'. In the near future I'd like to land generic support for prelighting when over St.Button. @@ +360,3 @@ + let iconBox = new Big.Box({ y_align: Big.BoxAlignment.CENTER, + reactive: true }); + text: info.name, Also while you're here, St.Bin is better now than Big.Box for holding icons, because it defaults to centering. @@ +370,3 @@ + let removeIconBox = new Big.Box({ y_align: Big.BoxAlignment.CENTER, + let removeIcon = Shell.TextureCache.get_default().load_icon_name ('media-eject', PLACES_ICON_SIZE); + if (info.isRemovable()) { St.Bin here too. @@ +376,3 @@ + let removeIconBox = new Big.Box({ y_align: Big.BoxAlignment.CENTER, + let removeIcon = Shell.TextureCache.get_default().load_icon_name ('media-eject', PLACES_ICON_SIZE); + if (info.isRemovable()) { Ditto for using a St.Button.
(In reply to comment #2) > Review of attachment 148694 [details] [review]: > > Cool, thanks! Will conflict with my search patch =( But I'll deal with that > after it lands. I know - I already have an updated patch, just waiting for your patch to land. Some small comments follow. > > ::: js/ui/placeDisplay.js > @@ +54,3 @@ > + isRemovable: function() { > + > + }, > > We have this more standard at the top, like: > > PlaceDeviceInfo.prototype = { > __proto__: PlaceInfo.prototype, > > _init: function () { > ... Yeah, I figured this out myself - this is what I do in current code ... > > @@ +61,3 @@ > + > + iconFactory: function(size) { > + let icon=this._mount.get_icon(); > > Spaces around = Wooops, thanks for the catch! > @@ +79,3 @@ > + > + //this._mount.unmount(0, null, Lang.bind(this, this._removeFinish)); > + this._mount.unmount(0, null, this._removeFinish, null); > > Why the commented out version? The commented version follows the same syntax as connect, idle_add etc. In other words, it is the syntax I'd like to see for callback functions - unfortunately, currently gjs requires to pass null as user_data. See https://bugzilla.gnome.org/show_bug.cgi?id=603754 > @@ +357,1 @@ > let text = new St.Label({ style_class: 'places-item', > > This should be a St.Button; connect to 'clicked'. In the near future I'd like > to land generic support for prelighting when over St.Button. > > @@ +360,3 @@ > + let iconBox = new Big.Box({ y_align: Big.BoxAlignment.CENTER, > + reactive: true }); > + text: info.name, > > Also while you're here, St.Bin is better now than Big.Box for holding icons, > because it defaults to centering. > > @@ +370,3 @@ > + let removeIconBox = new Big.Box({ y_align: > Big.BoxAlignment.CENTER, > + let removeIcon = Shell.TextureCache.get_default().load_icon_name > ('media-eject', PLACES_ICON_SIZE); > + if (info.isRemovable()) { > > St.Bin here too. > > @@ +376,3 @@ > + let removeIconBox = new Big.Box({ y_align: > Big.BoxAlignment.CENTER, > + let removeIcon = Shell.TextureCache.get_default().load_icon_name > ('media-eject', PLACES_ICON_SIZE); > + if (info.isRemovable()) { > > Ditto for using a St.Button. OK, I'll update the code accordingly.
Created attachment 149932 [details] [review] Add eject buttons in places menu for places which support it Updated the patch according to the review.
Created attachment 150010 [details] [review] Add eject buttons in places menu for places which support it Updated patch to apply after landing of the search rewrite.
Created attachment 150023 [details] [review] Add eject buttons in places menu for places which support it The previous patch version broke search in the overlay.
Review of attachment 150023 [details] [review]: Some comments follow, I took the liberty of fixing them and will push. Thanks! ::: js/ui/placeDisplay.js @@ +83,3 @@ + launch: function() { + Gio.app_info_launch_default_for_uri(this._mount.get_root().get_uri(), + Main.createAppLaunchContext()); This is now global.create_app_launch_context. @@ +395,3 @@ spacing: 4 }); + label: info.name }); + let text = new St.Button({ style_class: 'places-item', This needed x_align: St.Align.START Otherwise we end up centering the label which is wrong here. @@ +406,3 @@ + let removeIconBox = new St.Bin({ child: removeIcon, + let removeIcon = Shell.TextureCache.get_default().load_icon_name ('media-eject', PLACES_ICON_SIZE); + if (info.isRemovable()) { This one should be a St.Button. @@ +408,3 @@ + let removeIconBox = new St.Bin({ child: removeIcon, + let removeIcon = Shell.TextureCache.get_default().load_icon_name ('media-eject', PLACES_ICON_SIZE); + if (info.isRemovable()) { And this should be 'clicked'
Attachment 150023 [details] pushed as 1c4c3af - Add eject buttons in places menu for places which support it