GNOME Bugzilla – Bug 770750
Allow to run polari in the background
Last modified: 2016-11-24 18:34:16 UTC
This has been on the feature list for a while, but the corresponding design pages have been stalling for a while. I finally got around having a quick chat about this with Allan at Guadec though, and this is the pattern he suggested. (This adds a new feature, new UI and a fair share of new translatable strings though, so I'm afraid it'll have to wait till next cycle)
Created attachment 334641 [details] [review] app: Add option to run in background Like other messaging applications, IRC clients don't require constant attention from the user, but can sit in the background and notify the user when an important message was received. We currently don't have any explicit support for this mode, so users need to ignore the window the best they can (by moving it to another workspace or keeping it at the bottom of the window stack). This is obviously not great, but neither are status icons that have traditionally been used by applications to offer this mode. Instead, implement a pattern for background applications that was agreed upon with the GNOME design team: When set up to run in the background, the application: - sets itself up to start in the background on login - keeps running after the last window has been closed - can be stopped via the 'quit' action in the application menu
Created attachment 334642 [details] [review] mainWindow: Offer to keep running in background on close The background mode added by the previous commit is unintrusive, but also hard to discover. To address this, add a one-time dialog that presents the available choice to the user when the main window is closed.
Review of attachment 334641 [details] [review]: tested it, looked it through and it looks good to me
Review of attachment 334642 [details] [review]: looks good to me, one comment: ::: data/resources/main-window.ui @@ +14,3 @@ + <property name="text" translatable="yes">Keep running in background?</property> + <property name="secondary-text" translatable="yes"> + When running in the background, Polari will stay connected and keep sending notifications, both now and the next time you log in. You can change this preference from the application menu. s/You can change this preference from the application menu./This preference can be changed from Polari's application menu/ ? If i recall guidelines correctly, I don't think we usually address the user directly by 'you', maybe im wrong though. also if you inspect the dialog and look at the 'label' property it says "\n text text \n" it looks like the indentation and newlines you have there is included in the label, i think that wasnt on purpose?
Created attachment 339621 [details] [review] application: Add application style only once We currently add the application style when initializing the main window, which is fine as long as the window is truly unique. However once we allow running the app in the background, this won't be the case anymore - the window can be closed and a new one opened at a later point. So to make sure the custom style is only added once, add it during application startup instead.
Created attachment 339655 [details] [review] chatView: Cancel backlog fetching on destroy Trying to insert backlogs into a widget that has been destroyed will result in errors that are likely harmless, but still better avoided by properly cancelling backlog fetching on destroy.
Review of attachment 334641 [details] [review]: so i was looking through this patch again and have 1 question: ::: src/application.js @@ +501,3 @@ + file.get_parent().make_directory_with_parents(null); + } catch(e if e.matches(Gio.IOErrorEnum, Gio.IOErrorEnum.EXISTS)) { + // not an error, carry on do you really need this catch clause?
Review of attachment 339621 [details] [review]: looks good to me
(In reply to Bastian Ilsø from comment #7) > do you really need this catch clause? Yes. We first make sure that the autostart directory exists (~/.config/autostart), then we create a symlink to the autostart .desktop file there. If the first step throws an exception because the directory already exists, we'll never get to the second step (creating the link). That's the correct behavior if we couldn't ensure the directory where we want to create the link, but if the failure was that the directory already exists and we don't have to do anything to ensure it's there, then we should go on and try to create the link. (We *could* instead test whether the directory exists and only try to create it otherwise, but that adds an additional race condition where something can create the directory after we check for it and before we try to create it)
Created attachment 340191 [details] [review] entryArea: Exclude confirmation label from show_all() The visibility is set programmatically (and controls the state or visibility of other elements via property bindings), so make sure to not show it accidentally via show_all(). (For now, the window is usually shown before rooms are added, but this won't be the case anymore when we allow running the application in the background). Ha, I knew there was something else I wanted to fix before landing this!
Review of attachment 339655 [details] [review]: looks good to me
Review of attachment 340191 [details] [review]: looks good to me.
Attachment 334641 [details] pushed as 00ab02c - app: Add option to run in background Attachment 334642 [details] pushed as 10c512e - mainWindow: Offer to keep running in background on close Attachment 339621 [details] pushed as 232f63a - application: Add application style only once Attachment 339655 [details] pushed as 40d173c - chatView: Cancel backlog fetching on destroy Attachment 340191 [details] pushed as d1a33dc - entryArea: Exclude confirmation label from show_all() (In reply to Bastian Ilsø from comment #4) > s/You can change this preference from the application menu./This preference > can be changed from Polari's application menu/ ? Changed according to a suggestion from Allan. > it looks like the indentation and newlines you have there is included in the > label, i think that wasnt on purpose? No indeed. Fixed.
This is a great new feature! Thanks a bunch. Would it be also possible to add a quit command line option to stop polari running in the background, for example, by ssh-ing into my office desktop computer, so that I can launch an instance on my home laptop without nickname conflicts?
Something like gapplication action org.gnome.Polari quit should already work (assuming access to the same DBus session bus), but I guess we could expose that as command line option - would you file a bug for that and mark it with the newcomers keyword?