GNOME Bugzilla – Bug 653520
Add hotplug automount and autorun notifications
Last modified: 2011-07-13 18:43:37 UTC
Hi, the following patchset adds an implementation for hotplug automount and autorun notifications to gnome-shell, as seen in http://blogs.gnome.org/cosimoc/2011/06/23/hotplug-hotness/ Some notes on the implementation: - the patchset adds a sniffer DBus activatable service, which could also be moved to GVfs if gnome-shell maintainers think it's a better fit - to properly test the automount and autorun implementations, you need to disable the automount gnome-settigns-daemon plugin. As we still want the plugin for fallback mode, I have a patch for gnome-settings-daemon which moves that code, as is, in a separate process which gets autostarted in fallback mode only, using the AutostartCondition facilities provided by gnome-session (similar to what we already do for the notification-daemon and polkit authentication agent, among the others), which I will post and mark as blocked by this bug as soon as I'm done typing this :) - the UI elements could probably still be improved. It's already a bit different from my screenshots on my blog post, as I got some feedback from Lapo and fixed a few padding/icon sizes - we probably want to show docking stations in the removable devices list, but it's not implemented yet. I think this shouldn't block the review of this patchset anyway Thanks for the consideration
Created attachment 190789 [details] [review] autorun: add a AutorunManager class First implementation of AutorunManager
Created attachment 190790 [details] [review] automount: add automountManager
Created attachment 190791 [details] [review] autorun: don't add non-autorunnable volumes to the remove list Basically we want to filter out non-devices there, like network mounts, archives, or blank media.
Created attachment 190792 [details] [review] autorun: follow gsettings preferences for autorun
Created attachment 190793 [details] [review] autorun: make sure we have an app before trying to start it
Created attachment 190794 [details] [review] sniffer: add a mimetype sniffer helper executable
Created attachment 190795 [details] [review] autorun: integrate with the shell sniffer process
Created attachment 190796 [details] [review] sniffer: don't misuse x-content/audio-player and x-content/dcf-image Instead, use new custom x-content mime types (x-content/video, x-content/audio, x-content/documents, x-content/pictures). Those can be set opt-in from applications who support removable devices as content repositories to browse.
Created attachment 190797 [details] [review] automount: only autorun volumes marked as allowed Port code from g-s-d to mark volumes as allowed for autorun, and check for it when running the notification.
Created attachment 190798 [details] [review] mount-operation: add a ShellMountOperation implementation Ideally, this would be an entirely-JS implementation, but we have a couple of issues with gjs and gobject-introspection to work around, so we need a ShellMountOperation class for the time being. This first commit implements the show-processes dialog, with a system modal style very similar to the EndSession dialog. Implementations of ask-question and ask-password will follow shortly.
Created attachment 190799 [details] [review] mount-operation: implement ask-question
Created attachment 190800 [details] [review] mount-operation: don't implement ask-password We don't want the password to be asked in this context; the only mounts that require a password are network mounts though, and we never mount them, so it's fine.
Created attachment 190801 [details] [review] mount-operation: use ShellMountOperation when automounting a device
Created attachment 190802 [details] [review] autorun: prefer Safe Removal over eject/unmount if possible Basically do what NautilusPlacesSidebar does with the drive/volume/mount eject/unmount/stop priorities.
Created attachment 190803 [details] [review] sniffer: timing/padding tweaks
Created attachment 190804 [details] [review] mount-operation: implement ask-password for mounting encrypted volumes
Created attachment 190805 [details] [review] autorun: refactor code a bit
Created attachment 190806 [details] [review] automount: move some code around It's better to check if the session is active before setting the autorun flags anyway.
Created attachment 190807 [details] [review] hotplug: some style tweakings As suggested by Lapo Calamandrei
Created attachment 190808 [details] [review] automount: emit sounds when a drive is connected/disconnected
Created attachment 190809 [details] [review] automount: handle the drive-eject-button signal The implementation is untested, as the signal is not emitted from the Gdu GVfs volume monitor yet.
*** Bug 647027 has been marked as a duplicate of this bug. ***
> sniffer: timing/padding tweaks > automount: move some code around > autorun: refactor code a bit A lot of these patches seem like they could be squashed. > mount-operation: don't implement ask-password > mount-operation: implement ask-password for mounting encrypted volumes Make up your mind! :)
(In reply to comment #23) > > sniffer: timing/padding tweaks > > automount: move some code around > > autorun: refactor code a bit > > A lot of these patches seem like they could be squashed. > > > mount-operation: don't implement ask-password > > mount-operation: implement ask-password for mounting encrypted volumes > > Make up your mind! :) Yes, I already mentioned that on IRC. So far that's the main point in my review process ...
(In reply to comment #24) > Yes, I already mentioned that on IRC. So far that's the main point in my review > process ... Does this mean you're waiting for me to squash those patches before reviewing the rest of the branch?
I'd say yes. I don't think either of us wants to review code and then find out it has been overwritten (or fixed) in another patch. We don't want to review any more code than we need to.
I wanted to have a look at this and see what the position of the menu was like (blog post says bottom, me and others think it should be top) but it doesn't apply to master anymore. Another reason to rebase and squash!
Created attachment 191712 [details] [review] autorun: add a AutorunManager class First implementation of AutorunManager
Created attachment 191713 [details] [review] automount: add automountManager
Created attachment 191714 [details] [review] autorun: follow gsettings preferences for autorun
Created attachment 191715 [details] [review] sniffer: add a mimetype sniffer helper executable
Created attachment 191716 [details] [review] autorun: integrate with the shell sniffer process
Created attachment 191717 [details] [review] automount: only autorun volumes marked as allowed Port code from g-s-d to mark volumes as allowed for autorun, and check for it when running the notification.
Created attachment 191718 [details] [review] mount-operation: add a ShellMountOperation implementation Ideally, this would be an entirely-JS implementation, but we have a couple of issues with gjs and gobject-introspection to work around, so we need a ShellMountOperation class for the time being. This first commit implements the show-processes dialog, with a system modal style very similar to the EndSession dialog. Implementations of ask-question and ask-password will follow shortly.
Created attachment 191719 [details] [review] mount-operation: implement ask-question
Created attachment 191720 [details] [review] mount-operation: use ShellMountOperation when automounting a device
Created attachment 191721 [details] [review] autorun: prefer Safe Removal over eject/unmount if possible Basically do what NautilusPlacesSidebar does with the drive/volume/mount eject/unmount/stop priorities.
Created attachment 191722 [details] [review] mount-operation: implement ask-password for mounting encrypted volumes
Created attachment 191723 [details] [review] automount: emit sounds when a drive is connected/disconnected
Created attachment 191724 [details] [review] automount: handle the drive-eject-button signal The implementation is untested, as the signal is not emitted from the Gdu GVfs volume monitor yet.
New squashed/rebased patchset attached; no functional changes, but this trims down the number of total commits a bit.
(In reply to comment #27) > I wanted to have a look at this and see what the position of the menu was like > (blog post says bottom, me and others think it should be top) but it doesn't > apply to master anymore. Another reason to rebase and squash! Hi Ross; the menu is indeed in the bottom bar (aka the message tray). When we talked about it with Jon, I remember we opted for the message tray for a number of reasons: - the top bar is meant to be stable over time [ I know the bluetooth icon could in theory come and go, but USB bluetooth adapters are nowhere nearly as popular and in common use as USB flash drives, so it's a good approximation to assume you either have or don't have bluetooth for the duration of your session, in the common case ] - the top bar can already be quite crowded, and this could potentially become a problem when using the shell in portrait mode - having a transient notification for the plug-in action and a resident notification for the devices list makes it a consistent workflow Jon could probably add some more background on the choice.
I suppose, from our perspective the location question is related to the direction that we want this feature to evolve in future. If we want to use it to allow the user to track and manage 'storage' (and transient storage at that) be that local, network or USB (within the link.local scope) then it's certainly more of a notification - and in that case we should probably spend a bit more time to improve the 'import X' options and add capacity meters. If we want to use it to allow the user to track and manage locally connected devices (and again I mean in a proximate scope) such as printers, televisions, storage and other random bits and pieces that people use to accessorise their computing then it belongs in the top bar, and could embrace and extend other dialogues, such as bluetooth for example. I'd certainly like to make the latter at some point, but if people want the former then the message tray is more useful, certainly in the short term.
(In reply to comment #25) > Does this mean you're waiting for me to squash those patches before reviewing > the rest of the branch? No, I had started reviewing the (old) patches, but not a big problem ...
(In reply to comment #43) > If we want to use it to allow the user to track and manage 'storage' (and > transient storage at that) be that local, network or USB (within the link.local > scope) then it's certainly more of a notification - and in that case we should > probably spend a bit more time to improve the 'import X' options and add > capacity meters. I believe the capacity meters and a more explicit integration of the 'import' options directly from the resident notification are both good things. > If we want to use it to allow the user to track and manage locally connected > devices (and again I mean in a proximate scope) such as printers, televisions, > storage and other random bits and pieces that people use to accessorise their > computing then it belongs in the top bar, and could embrace and extend other > dialogues, such as bluetooth for example. > > I'd certainly like to make the latter at some point, but if people want the > former then the message tray is more useful, certainly in the short term. On the other hand, I don't see why we would need to mix storage with everything else here. Each one of the devices you mentioned has some degree of system integration already (which could be possibly always improved of course), and e.g. printers/televisions don't need to be ejected/don't have the concept of 'autorun' associated with them.
Cosimo, can you update the branch to the new patch series?
(In reply to comment #46) > Cosimo, can you update the branch to the new patch series? Okay, I pushed the rebased/squashed version here: http://git.gnome.org/browse/gnome-shell/log/?h=hotplug
Review of attachment 191712 [details] [review]: The commit message is useless - there should be at least a small definition of "autorun manager". Most points are minor, except for the "disappearing" icon in the resident source. ::: js/ui/autorunManager.js @@ +59,3 @@ + + guessContentTypes: function(mount) { + // guess mount's content type using GIO Minor nitpick - for consistency with the function name, the comment should use plural as well. @@ +93,3 @@ + + this._initTransientDispatcher(); + this._initResidentSource(); Delegating initialization to functions suggests that they are called multiple times, which is not true - in particular _initTransientDispatcher() looks pointless. I'd replace the functions with the actual code, in particular as in the current form it is not obvious that _initVolumeMonitor() is required to be called before _initResidentSource() ... @@ +131,3 @@ + // don't do anything if our session is not the currently + // active one + if (!Main.automountManager.ckListener.sessionActive) Main.automountManager doesn't exist yet. @@ +222,3 @@ + + // reset the notification for the next time + this._initNotification(); This doesn't work for me in testing - from the second time the source is shown, it doesn't have an icon. @@ +283,3 @@ + let mountLayout = new St.BoxLayout({ style_class: 'hotplug-resident-mount', + track_hover: true, + reactive: true }); Would it be better to use St.Button here (using the box as child)? It is a bit weird that the mount is triggered on button press, while the eject action uses button-release ... @@ +305,3 @@ + child: new St.Icon + ({ icon_name: 'media-eject', + style_class: 'hotplug-resident-eject-icon' })}); Weird indentation style. @@ +328,3 @@ + + ejectButton.connect('clicked', Lang.bind(this, function() { + Main.autorunManager.ejectMount(mount); Not sure I like this - the autorunManager "owns" the (transient|resident) dispatchers, which manage the sources, which create notifications, which then call back to the global autorunManager object. On the other hand, the alternatives are probably uglier, so maybe this is the lesser evil. @@ +341,3 @@ +AutorunTransientDispatcher.prototype = { + _init: function() { + this._sources = new Array(); ... or just this._sources = []; @@ +345,3 @@ + }, + + }, Ooops @@ +402,3 @@ + MessageTray.Source.prototype._init.call(this, mount.get_name()); + + this._mount = mount; This needs to be public, as source.mount is accessed by AutorunTransientDispatcher._getSourceForMount(). @@ +405,3 @@ + this._contentTypes = contentTypes; + + this._buildNotification(); To be honest, I prefer the style used for resident notifications, e.g. the notification prototype contains the UI logic of the notification, not the corresponding source. @@ +428,3 @@ + // and use those to present a more meaningful choice. + if (this._contentTypes.length == 0) + this._box.add (this._buttonForContentType('inode/directory'), Mmh, at least formally this breaks if there's no default app for 'inode/directory'. @@ +448,3 @@ + let box = new St.BoxLayout({ style_class: 'hotplug-notification-item', + track_hover: true, + reactive: true }); Like the comment for ResidentNotification - use an St.Button? @@ +459,3 @@ + box.add(label); + + box._delegate = app; By convention, _delegate always refers to the object to which the actor belongs (a.k.a 'this'). You can either rename the property to e.g. box._app, or (better IMO) pass the app as additional parameter to the callback: box.connect('button-press-event', Lang.bind(this, this._onAppButtonClicked, app)); @@ +518,3 @@ + // expands out + this.setTransient(true); + this.setUrgency(MessageTray.Urgency.CRITICAL); Is that designed behavior? Other than expanding the notification, setting the urgency to critical means the notification will stick around until acknowledged (and thus blocking other notifications from popping out). ::: js/ui/main.js @@ +13,3 @@ const St = imports.gi.St; +const AutomountManager = imports.ui.automountManager; Should be moved to he next patch (likewise all other automount stuff)
Review of attachment 191713 [details] [review]: Again, the commit message needs to be a lot more verbose - what is an "automountManager" and how does it relate to the previously added "autorunManager". Some additional comments: ::: js/ui/automountManager.js @@ +21,3 @@ +}; + +const ConsoleKitSession = function(sessionPath) { why const? @@ +69,3 @@ + this._ckSession.connect('ActiveChanged', + Lang.bind(this, this._onActiveChanged)); + this._ckSession.IsActiveRemote(Lang.bind(this, this._onIsActive)); onIsActive reads funny - better use an anonymous function instead? @@ +112,3 @@ + + _onSSAppeared: function(owner) { + this.GetActiveRemote(Lang.bind(this, this._onGetActive)); Dto. @@ +132,3 @@ + _init: function() { + this._settings = new Gio.Settings({ schema: SETTINGS_SCHEMA }); + this._volumeQueue = new Array(); A quick grep suggests that generally the "array = []" form is used, so it's probably better for consistency. @@ +136,3 @@ + this._initConsoleKitListener(); + this._initScreenSaverProxy(); + this._initVolumeMonitor(); Again, I'd just move the code directly into _init(). @@ +181,3 @@ + + let volumes = this._volumeMonitor.get_volumes(); + volumes.forEach(Lang.bind(this, function(volume) { Wouldn't Lang.bind(this, this._checkAndMountVolume) work (despite some additional tests, it would feel like less code duplication ...) @@ +235,3 @@ + log('Unable to mount volume ' + volume.get_name() + ': ' + + e.toString()); + return; Pointless return
Review of attachment 191714 [details] [review]: This also needs a better commit message, especially as 'automount-never' is already respected in the initial commit. ::: js/ui/autorunManager.js @@ +348,3 @@ }, + _getSettingsForType: function(contentType) { _getSettings suggests a return type of Gio.Settings - maybe _getAutorunSettingForType() is more obvious? @@ +353,3 @@ + return (type == contentType); + })) + return 'run'; Mmmh, I had a temporary parse error here due to the anonymous function in the condition - I think you should add {} around the block for clarity.
Review of attachment 191715 [details] [review]: Looks mostly good except for minor comments (and yes, the commit message could be more verbose ;-): ::: src/Makefile-hotplug-sniffer.am @@ +23,3 @@ + -DPREFIX=\""$(prefix)"\" \ + -DLIBDIR=\""$(libdir)"\" \ + -DDATADIR=\""$(datadir)"\" \ All those look unused? @@ +25,3 @@ + -DDATADIR=\""$(datadir)"\" \ + -DG_DISABLE_DEPRECATED \ + -DG_LOG_DOMAIN=\"ShellHotplugSniffer\" \ Unused - maybe you should use g_log()? ::: src/hotplug-sniffer/hotplug-sniffer.c @@ +306,3 @@ + now_t = now.tv_sec; + localtime_r (&now_t, &broken_down); + strftime (timebuf, sizeof timebuf, "%H:%M:%S", &broken_down); Aren't we expected to use GDatetime now? ::: src/hotplug-sniffer/shell-mime-sniffer.c @@ +186,3 @@ + + result.type = "x-content/video"; + result.ratio = (gdouble) state->video_count / (gdouble) state->total_items; Uhm - isn't total_items supposed to be 0 for empty file systems? @@ +474,3 @@ + g_clear_object (&self->priv->file); + g_clear_object (&self->priv->cancellable); + g_clear_object (&self->priv->async_result); Side note: I missed that function addition - nice one. @@ +561,3 @@ + ShellMimeSnifferPrivate); + init_mimetypes (); + init_mimetypes (); Needs a comment if init_mimetypes() is intentionally invoked twice.
Review of attachment 191716 [details] [review]: Looks mostly good - as always, the commit message could be improved (e.g. mention the reasoning behind moving the content type => application mapping up into ContentTypeDiscoverer) Some minor style suggestions: ::: js/ui/autorunManager.js @@ +115,3 @@ + _emitCallback: function(mount, contentTypes) { + if (!contentTypes || !contentTypes.length) + contentTypes.push('inode/directory'); Should merge my patch - does it make sense to just do if (!contentTypes) contentTypes = []; here, and rely on the fallback for (apps.length == 0)? @@ +122,3 @@ }); + let apps = new Array(); [] preferred. @@ +130,3 @@ + }); + + if (!apps.length) An explicit (apps.length == 0) is preferred @@ +369,3 @@ + let app = apps[0]; + startAppForMount(app, mount); I'd just write startAppForMount(apps[0], mount);
Review of attachment 191717 [details] [review]: To be honest, I don't quite understand the logic of the patch - assuming the code is correct, can you still answer the questions below (and maybe add some clarifying comments)? ::: js/ui/automountManager.js @@ +215,3 @@ + if (!this._settings.get_boolean(SETTING_ENABLE_AUTOMOUNT) || + !volume.should_automount() || + !volume.can_mount()) { Mmmh - so if the volume *should* automount and *can* mount, allowAutorun remains unset and evaluates to false in JS. Is that intended? @@ +259,3 @@ + + _allowAutorunExpire: function(volume) { + Mainloop.timeout_add_seconds(10, function() { Should use a const variable. ::: js/ui/autorunManager.js @@ +25,2 @@ if ((root.is_native() && !isMountRootHidden(root)) || + (volume && volume.allowAutorun && volume.should_automount())) I don't understand this - in automountManager, should_automount() is overwritten by allowAutorun, but here it is respected again?
Review of attachment 191718 [details] [review]: Looks good! ::: js/ui/shellMountOperation.js @@ +68,3 @@ + _init: function(mount) { + this._initMountOp(); + this._initIcon(mount); Just add the code directly to _init() ...
Review of attachment 191719 [details] [review]: Looks good - I'd move the automount parts from the next patch here. ::: js/ui/shellMountOperation.js @@ +182,3 @@ + ModalDialog.ModalDialog.prototype._init.call(this, { styleClass: 'mount-question-dialog' }); + + this._buildUI(icon) Could just go in _init() directly.
Review of attachment 191720 [details] [review]: I'd split up the patch and squash the changes in shellMountOperation with the initial mount operation patch, and the automount changes with the ask-question patch (consistent with the initial mount operation and ask-password patches)
Review of attachment 191721 [details] [review]: Code looks good, but the commit message should quickly describe *what* NautilusPlacesSidebar does - for the non-nautilus devs among us ;-)
Review of attachment 191722 [details] [review]: The use of a notification feels a bit weird to me, but I suppose that's the designed behavior? Otherwise looks mostly good. ::: js/ui/shellMountOperation.js @@ +289,3 @@ + this.addBody(strings[1]); + + this._buildUI(reaskPassword); ... @@ +314,3 @@ + let text = this._responseEntry.get_text(); + if (text == '') + return; Can't the empty string *be* the password?
Review of attachment 191723 [details] [review]: LGTM
Review of attachment 191724 [details] [review]: Code looks good. I'm a bit wary that this is untested, but it shouldn't be hard to fix eventual problems once the signal is actually emitted. ::: js/ui/automountManager.js @@ +239,3 @@ + if (drive.can_stop()) { + drive.stop + (Gio.MountUnmountFlags.FORCE, null, null, would it make sense to pass a mount operation? @@ +249,3 @@ + } else if (drive.can_eject()) { + drive.eject_with_operation + (Gio.MountUnmountFlags.FORCE, null, null, Dto.
Review of attachment 191714 [details] [review]: ::: js/ui/autorunManager.js @@ +352,3 @@ + if (runApp.some(function(type) { + return (type == contentType); + })) This is probably better as: if (runApp.indexOf(contentType) > -1) If they're not semantically equivalent (they should be, but I'm tired), I'd split it out and add a comment. @@ +353,3 @@ + return (type == contentType); + })) + return 'run'; I think these strings 'run', 'ignore' and 'files' should be enums. @@ +415,3 @@ + + if (app) { + startAppForMount(app, mount); startAppForMount doesn't return false if it failed to launch, so success will be true if we found an app in general. I'd rewrite the code to be like: // Don't do anything. if (settings == 'ignore') return; let app; if (settings == 'run') { // Run the default app. app = Gio.app_info_get_default_for_type(type, false); } else if (settings == 'files') { // Run the file browser. let app = Gio.app_info_get_default_for_type('inode/directory', false); } if (app) { if (startAppForMount(app, mount)) return; // App successfully launched. } // Fall back to asking the user if there was no default action, // or the app failed to launch. As well as changing startAppForMount to return whether it sucessfully launched. Of course, take all of Florian's comments into account too: "settings" is a bad name. I like "defaultAction" and "_getDefaultActionForContentType" @@ +428,3 @@ + + // we fallback here also in case the settings did not specify 'ask', + // but we failed launching the default app or the default file manager Comment is a bit misleading. One, settings cannot specify 'ask' in the first place, so it will always be a fallback... two, you don't get here if you've failed to launch the app.. See above.
Review of attachment 191718 [details] [review]: ::: js/ui/shellMountOperation.js @@ +11,3 @@ +const ModalDialog = imports.ui.modalDialog; + +function _setLabelText(label, text) { I'm not sure this is necessary: a label with no text will automatically be hidden, right? @@ +28,3 @@ + _init: function(app) { + this._app = app; + this._ITEM_ICON_SIZE = 48; Why is this an instance variable? @@ +234,3 @@ + + _setLabelsForMessage: function(message) { + let labels = message.split('\n'); Can't we possibly lose a message if there's more than one newline? ::: src/shell-mount-operation.c @@ +37,3 @@ + +enum { + SHOW_PROCESSES_2, I assume that GMountOperation already owns the not-2-signal. A comment explaining the difference between the two would be nice.
Review of attachment 191719 [details] [review]: ::: js/ui/shellMountOperation.js @@ +40,3 @@ + let labels = message.split('\n'); + + _setLabelText(dialog._subjectLabel, labels[0]); It's considered bad form to access another class's private members. @@ +141,3 @@ + + this._dialog.close(global.get_current_time()); + delete this._dialog; Don't delete the slot from the object, just set it to null. You should also set this._dialog, this._questionDialog, this._processesDialog to null in your _init.
Review of attachment 191713 [details] [review]: ::: js/ui/automountManager.js @@ +26,3 @@ + +ConsoleKitSession.prototype = { + _init: function(sessionPath) { Is there an advantage to doing this over DBus.makeProxyClass? @@ +82,3 @@ +DBus.proxifyPrototype(ConsoleKitManager.prototype, ConsoleKitManagerIface); + +const ScreenSaverIface = { We already have this interface in statusMenu.js, so it's probably best to consolidate (or use the statusMenu's proxy class) @@ +214,3 @@ + + if (this._ssProxy.screenSaverActive) { + if (this._volumeQueue.indexOf(volume) == -1) Can we be sure that we're getting the same volume instance every time? GJS just does a dumb pointer compare.
Created attachment 191830 [details] [review] screensaver: factor out a ScreenSaverProxy helper class This class will be shared between StatusMenu and the upcoming AutomountManager classes.
Created attachment 191831 [details] [review] autorun: add an AutorunManager class AutorunManager is a class that takes care of displaying and managing notifications and UI for storage devices. When a mount appears and a number of conditions are satisified, a transient notification will be displayed to immediately interact with the device. AutorunTransientDispatcher is the object that takes care of showing/hiding the notification sources as devices appear/disappear. Likewise, current mounts are kept in a list and presented within a list in a resident notification, handled by AutorunResidentSource.
Created attachment 191832 [details] [review] automount: add an AutomountManager class The AutomountManager class is the low-level counterpart of the previously introduced AutorunManager, and takes care of extracting the list of valid mounts from a GVolume or a GDrive and mounting them, provided a number of conditions and requirements are met. AutomountManager also keeps track of the current session availability (using the ConsoleKit and gnome-screensaver DBus interfaces) and inhibits mounting if the current session is locked, or another session is in use instead.
Created attachment 191833 [details] [review] autorun: follow per content-type gsettings preferences for autorun Autorun preferences can be fine-tweaked at the content-type level from the System Settings 'Removable Media' panel. Use those settings to figure out the default action for newly-mounted mounts.
Created attachment 191834 [details] [review] sniffer: add a mimetype sniffer helper executable The sniffer is a simple helper process, activated as a DBus service, that tries to crawl as many files as possible in the provided target directory (i.e. the new mount's root), for a maximum amount of time - which is set here to 1.5 seconds (i.e. it will crawl either all the files in the directory tree, or as many as it can before the specified timeout expires). Crawled files are ordered by their content type, and a generic estimation of the type of files composing the directory is returned to the caller, using generic 'x-content/*' mimetypes. The process will then set an autoquit timeout on itself, which can be disabled by setting the env variable HOTPLUG_SNIFFER_PERSIST for debugging purposes. The HOTPLUG_SNIFFER_DEBUG env variable can also be set to enable debugging output.
Created attachment 191835 [details] [review] autorun: integrate with the shell sniffer process If possible, use the results from the sniffer process in order to have less-generic alternatives to the file manager in the proposed autorun choices.
Created attachment 191836 [details] [review] automount: only autorun volumes marked as allowed Port code from g-s-d to mark volumes as allowed for autorun, and check for it when running the notification.
Created attachment 191837 [details] [review] mount-operation: add a ShellMountOperation implementation Ideally, this would be an entirely-JS implementation, but we have a couple of issues with gjs and gobject-introspection to work around, so we need a ShellMountOperation class for the time being. This first commit implements the show-processes dialog, with a system modal style very similar to the EndSession dialog. Implementations of ask-question and ask-password will follow shortly.
Created attachment 191838 [details] [review] mount-operation: implement ask-question Use a system modal dialogs for mount operation questions.
Created attachment 191839 [details] [review] autorun: prefer Safe Removal over eject/unmount if possible Basically do what NautilusPlacesSidebar does with the drive/volume/mount eject/unmount/stop priorities. We follow this pattern: - always prefer Safely Remove if available (i.e. drive.stop()) - fallback to ejecting the mount/volume/drive if that's not possible - finally, fallback to unmounting the mount if even eject is not available This also means we don't care about the distinction between Stop/Eject/Unmount at this level. Disk Utility (or Nautilus) are available for those who want that degree of control, but the common case here should do the most useful action without presenting the choice.
Created attachment 191840 [details] [review] mount-operation: implement ask-password for mounting encrypted volumes
Created attachment 191841 [details] [review] automount: emit sounds when a drive is connected/disconnected
Created attachment 191842 [details] [review] automount: handle the drive-eject-button signal The implementation is untested, as the signal is not emitted from the Gdu GVfs volume monitor yet.
Florian/Jasper: thanks for the great and thorough reviews! The commits messages should be much saner overall now: I kind of waited all the rebases to settle down before writing too much :) I'll answer some review questions inline here. (In reply to comment #48) > Review of attachment 191712 [details] [review]: > Most points are minor, except for the "disappearing" icon in the resident > source. That worked fine in my tests, and I couldn't really find a reason why my code would cause the icon to disappear. A quick investigation indeed reveals this commit [1] in git master is the one causing the icon to disappear, and reverting makes things work again. If my code is actually broken in a way I can't figure out, apologies :) Otherwise, maybe this is better addressed in a separate follow-up bug? [1] http://git.gnome.org/browse/gnome-shell/commit/?id=c727da823b8530a929268d870929a0c79d0946c6 > @@ +328,3 @@ > + > + ejectButton.connect('clicked', Lang.bind(this, function() { > + Main.autorunManager.ejectMount(mount); > > Not sure I like this - the autorunManager "owns" the (transient|resident) > dispatchers, which manage the sources, which create notifications, which then > call back to the global autorunManager object. On the other hand, the > alternatives are probably uglier, so maybe this is the lesser evil. Yeah, it's a bit convoluted; I saw the global-object-from-Main pattern is used elsewhere in the shell codebase though, so I thought it might be OK. Another option I have would be to make ejectMount() a helper function, outside the scope of the AutorunManager class; I personally don't have any preference. > @@ +518,3 @@ > + // expands out > + this.setTransient(true); > + this.setUrgency(MessageTray.Urgency.CRITICAL); > > Is that designed behavior? Other than expanding the notification, setting the > urgency to critical means the notification will stick around until acknowledged > (and thus blocking other notifications from popping out). Yes, that's by design. (In reply to comment #51) > @@ +25,3 @@ > + -DDATADIR=\""$(datadir)"\" \ > + -DG_DISABLE_DEPRECATED \ > + -DG_LOG_DOMAIN=\"ShellHotplugSniffer\" \ > > Unused - maybe you should use g_log()? I just removed this; see below. > ::: src/hotplug-sniffer/hotplug-sniffer.c > @@ +306,3 @@ > + now_t = now.tv_sec; > + localtime_r (&now_t, &broken_down); > + strftime (timebuf, sizeof timebuf, "%H:%M:%S", &broken_down); > > Aren't we expected to use GDatetime now? The debug part is copied basically as is from the calendar helper process; that's also where the log domain define came from. For this reason, I kept it as-is for now. If we care about using that in this debug code, I believe it could be done in a later separate bug for both the processes. > + > + result.type = "x-content/video"; > + result.ratio = (gdouble) state->video_count / (gdouble) state->total_items; > > Uhm - isn't total_items supposed to be 0 for empty file systems? Nice catch! > @@ +561,3 @@ > + ShellMimeSnifferPrivate); > + init_mimetypes (); > + init_mimetypes (); > > Needs a comment if init_mimetypes() is intentionally invoked twice. This wasn't intentional; I believe it's a debug leftover when I tested if the cache actually worked. (In reply to comment #52) > Review of attachment 191716 [details] [review]: > ::: js/ui/autorunManager.js > @@ +115,3 @@ > + _emitCallback: function(mount, contentTypes) { > + if (!contentTypes || !contentTypes.length) > + contentTypes.push('inode/directory'); > > Should merge my patch - does it make sense to just do > > if (!contentTypes) > contentTypes = []; > > here, and rely on the fallback for (apps.length == 0)? Yeah, totally. I merged your patch now. (In reply to comment #53) > Review of attachment 191717 [details] [review]: > > To be honest, I don't quite understand the logic of the patch - assuming the > code is correct, can you still answer the questions below (and maybe add some > clarifying comments)? The code is taken from the equivalent in gnome-settings-daemon, and there was actually a missing this._allowAutorun(volume); in the _mountVolume() method that hopefully makes things clearer. > ::: js/ui/automountManager.js > @@ +215,3 @@ > + if (!this._settings.get_boolean(SETTING_ENABLE_AUTOMOUNT) || > + !volume.should_automount() || > + !volume.can_mount()) { > > Mmmh - so if the volume *should* automount and *can* mount, allowAutorun > remains unset and evaluates to false in JS. Is that intended? Yes, because that was supposed to be set in _mountVolume() anyway. > ::: js/ui/autorunManager.js > @@ +25,2 @@ > if ((root.is_native() && !isMountRootHidden(root)) || > + (volume && volume.allowAutorun && volume.should_automount())) > > I don't understand this - in automountManager, should_automount() is > overwritten by allowAutorun, but here it is respected again? This is basically the port of an equivalent gnome-settings-daemon method, see [2]. I have to admit your comment makes sense though. Needs a bit more investigation. [2] http://git.gnome.org/browse/gnome-settings-daemon/tree/plugins/automount/gsd-autorun.c#n936 In reply to comment #58) > Review of attachment 191722 [details] [review]: > @@ +314,3 @@ > + let text = this._responseEntry.get_text(); > + if (text == '') > + return; > > Can't the empty string *be* the password? No, at least from gnome-disk-utility you don't seem to be able to use an empty password for encrypted mounts. (In reply to comment #62) > ::: js/ui/shellMountOperation.js > @@ +11,3 @@ > +const ModalDialog = imports.ui.modalDialog; > + > +function _setLabelText(label, text) { > > I'm not sure this is necessary: a label with no text will automatically be > hidden, right? Hmm, doesn't seem so. I don't want the label space to be allocated if the label is empty. > @@ +234,3 @@ > + > + _setLabelsForMessage: function(message) { > + let labels = message.split('\n'); > > Can't we possibly lose a message if there's more than one newline? No, there should be at maximum one like break there. That's also what GTK+ does FWIW, and this is what the docs for GMountOperation say: "If the message contains a line break, the first line should be presented as a heading. For example, it may be used as the primary text in a GtkMessageDialog." > ::: src/shell-mount-operation.c > @@ +37,3 @@ > + > +enum { > + SHOW_PROCESSES_2, > > I assume that GMountOperation already owns the not-2-signal. > > A comment explaining the difference between the two would be nice. There's already a comment explaining this in shell-mount-operation.c: "* Also, we need to workaround the fact that gjs doesn't support type * annotations for signals yet (so we can't effectively forward e.g. * the GPid array to JS). * See https://bugzilla.gnome.org/show_bug.cgi?id=645978 */" (In reply to comment #64) > Review of attachment 191713 [details] [review]: > > ::: js/ui/automountManager.js > @@ +26,3 @@ > + > +ConsoleKitSession.prototype = { > + _init: function(sessionPath) { > > Is there an advantage to doing this over DBus.makeProxyClass? No, I just didn't know DBus.makeProxyClass :) Here it's actually more concise than proxifying the object manually. > @@ +214,3 @@ > + > + if (this._ssProxy.screenSaverActive) { > + if (this._volumeQueue.indexOf(volume) == -1) > > Can we be sure that we're getting the same volume instance every time? GJS just > does a dumb pointer compare. Using a pointer compare is fine here; that's also what the gnome-settings-daemon code does. I remember testing it and the code worked fine. Another approach could be using the UUID for this maybe? Not sure if it's worth.
(In reply to comment #78) > (In reply to comment #48) > > Review of attachment 191712 [details] [review] [details]: > > > Most points are minor, except for the "disappearing" icon in the resident > > source. > > That worked fine in my tests, and I couldn't really find a reason why my code > would cause the icon to disappear. > A quick investigation indeed reveals this commit [1] in git master is the one > causing the icon to disappear, and reverting makes things work again. > > If my code is actually broken in a way I can't figure out, apologies :) > Otherwise, maybe this is better addressed in a separate follow-up bug? Ah, possible that commit broke something - follow-up bug sounds good then. > > @@ +328,3 @@ > > Not sure I like this - the autorunManager "owns" the (transient|resident) > > dispatchers, which manage the sources, which create notifications, which then > > call back to the global autorunManager object. On the other hand, the > > alternatives are probably uglier, so maybe this is the lesser evil. > > Yeah, it's a bit convoluted; I saw the global-object-from-Main pattern is used > elsewhere in the shell codebase though, so I thought it might be OK. Yeah, as said, the alternative may be uglier. So let's just leave it like that. > > Is that designed behavior? [...] > > Yes, that's by design. OK. > The debug part is copied basically as is from the calendar helper process; > that's also where the log domain define came from. For this reason, I kept it > as-is for now. If we care about using that in this debug code, I believe it > could be done in a later separate bug for both the processes. Yeah, or we decide that we don't care :-) > The code is taken from the equivalent in gnome-settings-daemon, and there was > actually a missing > > this._allowAutorun(volume); > > in the _mountVolume() method that hopefully makes things clearer. Yes! > > ::: js/ui/autorunManager.js > > @@ +25,2 @@ > > if ((root.is_native() && !isMountRootHidden(root)) || > > + (volume && volume.allowAutorun && volume.should_automount())) > > > > I don't understand this - in automountManager, should_automount() is > > overwritten by allowAutorun, but here it is respected again? > > This is basically the port of an equivalent gnome-settings-daemon method, see > [2]. I have to admit your comment makes sense though. Needs a bit more > investigation. Sure. Let's keep it in mind and open a follow-up bug if necessary. > In reply to comment #58) > > Can't the empty string *be* the password? > > No, at least from gnome-disk-utility you don't seem to be able to use an empty > password for encrypted mounts. OK then. > > (In reply to comment #62) > > ::: js/ui/shellMountOperation.js > > @@ +11,3 @@ > > +const ModalDialog = imports.ui.modalDialog; > > + > > +function _setLabelText(label, text) { > > > > I'm not sure this is necessary: a label with no text will automatically be > > hidden, right? No, it will have a text of "" (which is the default for ClutterText:text).
Review of attachment 191830 [details] [review]: Generally looks good, but it's way ugly that you rely on automountManager to update a public property in ScreenSaver. Suggestion below: ::: js/misc/screenSaver.js @@ +35,3 @@ + _onSSAppeared: function(owner) { + this.GetActiveRemote(Lang.bind(this, function(isActive) { + this.screenSaverActive = isActive; It is weird that the isActive property is partly updated here and partly in automountManager.js - I'd say it's cleaner to connect to the 'ActiveChanged' signal here and update the property, then emit your own 'active-changed' signal so automountManager can do its job. ::: js/ui/statusMenu.js @@ +23,3 @@ const DISABLE_LOG_OUT_KEY = 'disable-log-out'; +let ScreenSaverProxy = new ScreenSaver.ScreenSaverProxy(); That change is wrong - 'ScreenSaverProxy' used to be the prototype which is now ScreenSaver.ScreenSaverProxy, just remove that line.
Review of attachment 191831 [details] [review]: Some style nits left, otherwise looks good. ::: js/ui/autorunManager.js @@ +106,3 @@ + mounts.forEach(Lang.bind(this, function (mount) { + let discoverer = new ContentTypeDiscoverer( + Lang.bind (this, function (mount, contentTypes) { My personal indentation preference would be let discoverer = new ContentTypeDiscoverer(Lang.bind(this, function(mount, contentTypes) { foo(); })); It makes the block a bit clearer in my opinion, but feel free to ignore @@ +271,3 @@ + let labelBin = new St.Bin({ y_align: St.Align.MIDDLE }); + let mountLabel = + new St.Label ({ text: mount.get_name(), Nit: stray space before '(' @@ +286,3 @@ + expand: true }); + + let ejectButton = new St.Button({ You do let mountLabel = new St.Label({ foo: bar, and let ejectButton = new St.Button({ foo: bar, Should use a consistent style here.
Review of attachment 191832 [details] [review]: See comments. ::: js/ui/automountManager.js @@ +59,3 @@ + _onCurrentSession: function(session) { + this._ckSession = new ConsoleKitSessionProxy(DBus.system, 'org.freedesktop.ConsoleKit', session); + log(this._ckSession); Left-over debug @@ +101,3 @@ + + _screenSaverActiveChanged: function(object, isActive) { + this._ssProxy.screenSaverActive = isActive; As mentioned in the review of the ScreenSaver patch, setting the property here is kind of ugly. @@ +105,3 @@ + if (!isActive) { + this._volumeQueue.forEach(Lang.bind(this, function(volume) { + this._checkAndMountVolume(volume, true, true); Ugh, multiple boolean parameters are evil :-) I thought doing the additional checks in _startupMountAll() as well would be possible, as it turns out that it isn't, you could - revert to the previous version (e.g. don't use _checkAndMountVolume() in _startupMountAll() - add a flags parameter: this._checkAndMountVolume(volume, MountVolumeFlags.CHECK_SESSION | MountVolumeFlags.USE_MOUNT_OP); - add a Params parameter: this._checkAndMountVolume(volume, { check_session: true, use_mount_op: true });
Review of attachment 191833 [details] [review]: Looks good.
Review of attachment 191834 [details] [review]: Overall looks good, marking "needs-work" anyway because it doesn't compile ... ::: src/hotplug-sniffer/shell-mime-sniffer.c @@ +185,3 @@ + + if (state->total_items == 0) + goto out; GCC complains about 'results being possibly used uninitialized' now (as it's freed in out: without having been initialized when total_items is 0)
Review of attachment 191835 [details] [review]: Looks good, feel free to ignore the comment ::: js/ui/autorunManager.js @@ +129,3 @@ + + if (!contentTypes.length) + contentTypes.push('inode/directory'); I know this comes from my patch, but is it actually necessary? If contentTypes is an empty array, apps ends up being an empty array as well, so the default app for 'inode/directory' will be pushed into apps ...
Review of attachment 191836 [details] [review]: Looks good (as mentioned before, we can handle the should_automount() weirdness in a follow-up bug if necessary)
Review of attachment 191837 [details] [review]: Looks good, remaining comments are mostly style nits: ::: js/ui/automountManager.js @@ +157,3 @@ + if (useMountOp) { + let operation = new ShellMountOperation.ShellMountOperation(volume); Missing import for ShellMountOperation ::: js/ui/shellMountOperation.js @@ +100,3 @@ + let message = op.get_show_processes_message(); + + if (!this._processesDialog) { As spotted by Jasper, you should initialize this._processesDialog in _init (it works fine in JS, as undefined evaluates to false, but it's considered good style to "list" all properties in _init) @@ +130,3 @@ + ModalDialog.ModalDialog.prototype._init.call(this, { styleClass: 'show-processes-dialog' }); + + this._buildUI(icon); I'd still remove this. Otherwise good form would require to write: this._iconBin = null; this._subjectLabel = null; this._descriptionLabel = null; ... this._buildUI(icon);
Review of attachment 191838 [details] [review]: Looks good (feel free to ignore the 2nd comment) ::: js/ui/automountManager.js @@ +8,2 @@ const Main = imports.ui.main; +const ShellMountOperation = imports.ui.shellMountOperation; Ooops - belongs in the previous patch. ::: js/ui/shellMountOperation.js @@ +113,3 @@ _onAskQuestion: function(op, message, choices) { + let questionDialog = new ShellMountQuestionDialog(this._icon); + this._dialog = questionDialog; Mmmh, that local variable does not make sense? The schema let foo = new Foo(); this._fooProperty = foo; foo.connect('frobnicate', ...); ... makes sense to cut down on line length, but in this case 'questionDialog' is actually longer than 'this._dialog' ...
Review of attachment 191839 [details] [review]: LGTM
Review of attachment 191840 [details] [review]: LGTM
Review of attachment 191841 [details] [review]: LGTM
Review of attachment 191842 [details] [review]: Sure.
Review of attachment 191831 [details] [review]: Sorry, found another error ::: js/ui/autorunManager.js @@ +463,3 @@ + + button.connect('clicked', Lang.bind(this, function() { + startAppForMount(app, this.mount); this.mount is undefined (same for the eject button). Either add this.mount = source.mount; to _init(), or use this.source.mount instead.
Created attachment 191883 [details] [review] screensaver: factor out a ScreenSaverProxy helper class This class will be shared between StatusMenu and the upcoming AutomountManager classes.
Created attachment 191884 [details] [review] autorun: add an AutorunManager class AutorunManager is a class that takes care of displaying and managing notifications and UI for storage devices. When a mount appears and a number of conditions are satisified, a transient notification will be displayed to immediately interact with the device. AutorunTransientDispatcher is the object that takes care of showing/hiding the notification sources as devices appear/disappear. Likewise, current mounts are kept in a list and presented within a list in a resident notification, handled by AutorunResidentSource.
Created attachment 191885 [details] [review] automount: add an AutomountManager class The AutomountManager class is the low-level counterpart of the previously introduced AutorunManager, and takes care of extracting the list of valid mounts from a GVolume or a GDrive and mounting them, provided a number of conditions and requirements are met. AutomountManager also keeps track of the current session availability (using the ConsoleKit and gnome-screensaver DBus interfaces) and inhibits mounting if the current session is locked, or another session is in use instead.
Created attachment 191886 [details] [review] sniffer: add a mimetype sniffer helper executable The sniffer is a simple helper process, activated as a DBus service, that tries to crawl as many files as possible in the provided target directory (i.e. the new mount's root), for a maximum amount of time - which is set here to 1.5 seconds (i.e. it will crawl either all the files in the directory tree, or as many as it can before the specified timeout expires). Crawled files are ordered by their content type, and a generic estimation of the type of files composing the directory is returned to the caller, using generic 'x-content/*' mimetypes. The process will then set an autoquit timeout on itself, which can be disabled by setting the env variable HOTPLUG_SNIFFER_PERSIST for debugging purposes. The HOTPLUG_SNIFFER_DEBUG env variable can also be set to enable debugging output.
Created attachment 191887 [details] [review] autorun: integrate with the shell sniffer process If possible, use the results from the sniffer process in order to have less-generic alternatives to the file manager in the proposed autorun choices.
Created attachment 191888 [details] [review] mount-operation: add a ShellMountOperation implementation Ideally, this would be an entirely-JS implementation, but we have a couple of issues with gjs and gobject-introspection to work around, so we need a ShellMountOperation class for the time being. This first commit implements the show-processes dialog, with a system modal style very similar to the EndSession dialog. Implementations of ask-question and ask-password will follow shortly.
Created attachment 191889 [details] [review] mount-operation: implement ask-question Use a system modal dialogs for mount operation questions.
(In reply to comment #80) > Review of attachment 191830 [details] [review]: > It is weird that the isActive property is partly updated here and partly in > automountManager.js - I'd say it's cleaner to connect to the 'ActiveChanged' > signal here and update the property, then emit your own 'active-changed' signal > so automountManager can do its job. Yeah, totally agree; fixed now. (In reply to comment #82) > Review of attachment 191832 [details] [review]: > > @@ +105,3 @@ > + if (!isActive) { > + this._volumeQueue.forEach(Lang.bind(this, function(volume) { > + this._checkAndMountVolume(volume, true, true); > > Ugh, multiple boolean parameters are evil :-) I ended up choosing the Params way for this, looks cleaner in JS than flags IMO. (In reply to comment #84) > ::: src/hotplug-sniffer/shell-mime-sniffer.c > @@ +185,3 @@ > + > + if (state->total_items == 0) > + goto out; > > GCC complains about 'results being possibly used uninitialized' now (as it's > freed in out: without having been initialized when total_items is 0) Oops; should be fixed now, I moved the array creation before jumping out. Weird that my GCC didn't complain. (In reply to comment #85) > Review of attachment 191835 [details] [review]: > @@ +129,3 @@ > + > + if (!contentTypes.length) > + contentTypes.push('inode/directory'); > > I know this comes from my patch, but is it actually necessary? If contentTypes > is an empty array, apps ends up being an empty array as well, so the default > app for 'inode/directory' will be pushed into apps ... Yeah, that's too defensive, and just ensuring there's an element in the apps array is indeed enough.
Review of attachment 191883 [details] [review]: LGTM - bug 654550 has a patch which overlaps with this change, so might make sense to land this right away so the patch there can be rebased. ::: js/misc/screenSaver.js @@ +53,3 @@ + + getActive: function() { + return this._screenSaverActive; A public property which is *read* outside this prototype would be fine as well (e.g. the objection was only with regards to *writing* it). The method approach is obviously fine, too.
Comment on attachment 191883 [details] [review] screensaver: factor out a ScreenSaverProxy helper class > >+const ScreenSaver = imports.misc.screenSaver; > const GnomeSession = imports.misc.gnomeSession; imports should be sorted
Review of attachment 191884 [details] [review]: Sorry for still complaining, but I've changed my mind with regard to the "disappearing icon" problem - see comment below. ::: js/ui/autorunManager.js @@ +116,3 @@ + _onMountAdded: function(monitor, mount) { + let discoverer = new ContentTypeDiscoverer + (Lang.bind (this, function (mount, contentTypes) { Sorry for not mentioning it explicitly, but the style comment for _init() in the previous review assumed "... and elsewhere" ;-) @@ +203,3 @@ + + // reset the notification for the next time + this._initNotification(); I've looked a bit into the disappearing icon problem, and my conclusion was that if it worked before, it was at best incidental. Calling 'this.destroy()' and assume that the source is *not* destroyed but can be reused later feels wrong - and as it actually stopped working now, we should fix it before landing. Quick suggestion: connect to the source's 'destroy' signal (from autorunManager) and recreate the resident source in the callback. Based on that, some other suggestions: - remove _initNotification: just move its code into _init() - remove _redisplay: the function is called from two places, addMount and removeMount. It effectively reads if (calledFromRemoveMount) { doSomething(); return; } doSomethingElse(); It makes more sense to me to move the code into the appropriate functions.
Review of attachment 191885 [details] [review]: Looks good!
Review of attachment 191886 [details] [review]: Looks good!
Review of attachment 191887 [details] [review]: Sure.
Review of attachment 191888 [details] [review]: Yup
Review of attachment 191889 [details] [review]: LGTM
(In reply to comment #104) > - remove _redisplay: the function is called from two places, addMount and > removeMount. > It effectively reads > if (calledFromRemoveMount) { > doSomething(); > return; > } > doSomethingElse(); > It makes more sense to me to move the code into the appropriate functions. Scrap that, apparently that assumes that there's only a single mount at a time. Sorry for the noise.
Created attachment 191906 [details] [review] screensaver: factor out a ScreenSaverProxy helper class This class will be shared between StatusMenu and the upcoming AutomountManager classes.
Created attachment 191907 [details] [review] sniffer: add a mimetype sniffer helper executable The sniffer is a simple helper process, activated as a DBus service, that tries to crawl as many files as possible in the provided target directory (i.e. the new mount's root), for a maximum amount of time - which is set here to 1.5 seconds (i.e. it will crawl either all the files in the directory tree, or as many as it can before the specified timeout expires). Crawled files are ordered by their content type, and a generic estimation of the type of files composing the directory is returned to the caller, using generic 'x-content/*' mimetypes. The process will then set an autoquit timeout on itself, which can be disabled by setting the env variable HOTPLUG_SNIFFER_PERSIST for debugging purposes. The HOTPLUG_SNIFFER_DEBUG env variable can also be set to enable debugging output.
Created attachment 191908 [details] [review] autorun: add an AutorunManager class AutorunManager is a class that takes care of displaying and managing notifications and UI for storage devices. When a mount appears and a number of conditions are satisified, a transient notification will be displayed to immediately interact with the device. AutorunTransientDispatcher is the object that takes care of showing/hiding the notification sources as devices appear/disappear. Likewise, current mounts are kept in a list and presented within a list in a resident notification, handled by AutorunResidentSource.
Review of attachment 191906 [details] [review]: OK to commit with the following change: ::: js/ui/statusMenu.js @@ +13,2 @@ const GnomeSession = imports.misc.gnomeSession; +const ScreenSaver = imports.misc.screenSaver; Still wrong - should go between PopupMenu and Util (e.g. sorted alphabetical by the variable name)
(In reply to comment #104) > Review of attachment 191884 [details] [review]: > > Quick suggestion: connect to the source's 'destroy' signal (from > autorunManager) and recreate the resident source in the callback. That indeed fixes it; you can find it in the latest iteration of this patch I just attached. I also have new versions of the following; * sniffer: add a mimetype sniffer helper executable -> fixes some issues with DBus service files generation for the non-recursive autotools setup used in gnome-shell, spotted by Ross * screensaver: factor out a ScreenSaverProxy helper class -> just a cosmetic change to let the imports be properly sorted
Review of attachment 191907 [details] [review]: Sure.
Review of attachment 191908 [details] [review]: Run, Forrest, run!
Pushing everything to master. Attachment 191833 [details] pushed as 436dd6e - autorun: follow per content-type gsettings preferences for autorun Attachment 191836 [details] pushed as 98327b0 - automount: only autorun volumes marked as allowed Attachment 191839 [details] pushed as 5c1dd4e - autorun: prefer Safe Removal over eject/unmount if possible Attachment 191840 [details] pushed as 0b77ea4 - mount-operation: implement ask-password for mounting encrypted volumes Attachment 191841 [details] pushed as 5133c3e - automount: emit sounds when a drive is connected/disconnected Attachment 191842 [details] pushed as b4f5e42 - automount: handle the drive-eject-button signal Attachment 191885 [details] pushed as acc0533 - automount: add an AutomountManager class Attachment 191887 [details] pushed as 6786aee - autorun: integrate with the shell sniffer process Attachment 191888 [details] pushed as 297d135 - mount-operation: add a ShellMountOperation implementation Attachment 191889 [details] pushed as e1c6871 - mount-operation: implement ask-question Attachment 191906 [details] pushed as 6004e3d - screensaver: factor out a ScreenSaverProxy helper class Attachment 191907 [details] pushed as 0b9c726 - sniffer: add a mimetype sniffer helper executable Attachment 191908 [details] pushed as 534b371 - autorun: add an AutorunManager class
Thanks for the reviews! Please use new reports for further bugs or possible improvements.