GNOME Bugzilla – Bug 676125
autorun: add a notification when unmounting drives
Last modified: 2012-07-12 00:07:07 UTC
This is proposed as a temporary solution for GNOME 3.4 - for 3.6 we want to integrate a signal that will allow a cleaner implementation directly from GtkMountOperation (see https://bugzilla.gnome.org/show_bug.cgi?id=676111). See also https://bugzilla.redhat.com/show_bug.cgi?id=819492
Created attachment 214145 [details] [review] autorun: add a notification when unmounting drives Initial patch by Adel Gadllah <adel.gadllah@gmail.com> https://bugzilla.redhat.com/show_bug.cgi?id=819492
Review of attachment 214145 [details] [review]: ::: js/ui/autorunManager.js @@ +286,3 @@ + this._deviceName = device.get_name(); + + this._mountOp.connect('show-processes-2', Lang.bind(this, You need to explicitly disconnect from these signals and set this._mountOp to null when done with it, otherwise it will hang around forever.
Created attachment 214146 [details] [review] autorun: add a notification when unmounting drives Don't set mountOperation as a member of UnmountNotifier.
Review of attachment 214146 [details] [review]: One minor fixup. A quick commit message explaining the problem would also be good. ::: js/ui/autorunManager.js @@ +336,3 @@ + + createNotificationIcon: function() { + return new St.Icon ({ icon_name: 'media-removable', No whitespace.
Created attachment 214207 [details] [review] autorun: add a notification when unmounting drives Improved patch: avoids showing notifications when ejecting optical drives.
Patch in comment 5 would add new translatable strings (would need request for string freeze break before committing to stable branch, however no 3.4.3 is GNOME-wide planned, do not know about gnome-shell specifically).
Review of attachment 214207 [details] [review]: One minor fix-up that isn't your fault (I pushed a cleanup patch since this was rewritten) ::: js/ui/autorunManager.js @@ +354,3 @@ + }, + + createNotificationIcon: function() { This is not needed anymore. Simply pass the icon name and type to Source._init.
Created attachment 218466 [details] [review] mount-operation: implement show-unmount-progress New patch, depends on the new GIO/GVfs API of bug 676111
Review of attachment 218466 [details] [review]: Overall, looks fine. Very minor whitespace nit. ::: js/ui/shellMountOperation.js @@ +248,3 @@ + + show: function(message) { + let [ header, text ] = message.split('\n'); Should be [header, text]. Also, is there a guarantee there will only be two lines in the message? This might need to be message.split('\n', 1);
Attachment 218466 [details] pushed as 168e0b5 - mount-operation: implement show-unmount-progress Thanks, pushed to master with the suggested fix to the limit we apply when splitting the message string.