GNOME Bugzilla – Bug 714644
Start notifying of new mail at session startup
Last modified: 2014-06-27 23:59:37 UTC
---- Reported by jim@yorba.org 2013-02-08 17:23:00 -0800 ---- Original Redmine bug id: 6354 Original URL: http://redmine.yorba.org/issues/6354 Searchable id: yorba-bug-6354 Original author: Jim Nelson Original description: Many users have requested that Geary notify of new mail when they login, i.e. without running Geary, or perhaps with Geary autostarting (and hidden?) at login time. This is closely related to #4570, although that ticket has other concerns as well, not merely about mail notification. Related issues: related to geary - Feature #4570: split Geary engine into separate shared library or backgr... (Open) duplicated by geary - 7519: Allow to get notifications on new message without window ... (Duplicate) ---- Additional Comments From geary-maint@gnome.bugs 2013-08-14 13:05:00 -0700 ---- ### History #### #1 Updated by Jim Nelson 6 months ago * **Subject** changed from _Notify of new mail at session startup_ to _Start notifying of new mail at session startup_ #### #2 Updated by Jim Nelson 4 months ago * **Target version** changed from _0.4.0_ to _0.5.0_ #### #3 Updated by Jim Nelson 3 months ago This is a popular request on Launchpad: https://bugs.launchpad.net/geary/+bug/1053205 --- Bug imported by chaz@yorba.org 2013-11-21 20:28 UTC --- This bug was previously known as _bug_ 6354 at http://redmine.yorba.org/show_bug.cgi?id=6354 Unknown milestone "unknown in product geary. Setting to default milestone for this product, "---". Setting qa contact to the default for this product. This bug either had no qa contact or an invalid one. Resolution set on an open status. Dropping resolution
Since Geary won't have a window on the desktop when running in the background, it would make sense to use a GtkStatusIcon to make Geary available via the system tray.
GtkStatusIcon is being deprecated. I spoke with some of the GTK folks and they've strongly advised against using it in new code, so that's not the way to go here. A user asked some questions about implementing this ticket. Our general vision (now that GtkStatusIcon is off the table) is something like this: * The user should be able to turn this on/off via a Preferences checkbox ("Notify of new mail at startup") * When enabled, Geary needs to set itself up to run at user login time (I believe it means copying a special .desktop file to a magic location in the user's home directory; disabling the option means deleting that file.) * If disabled, Geary pretty much runs as it does today. * If enabled: * The magic .desktop file launches Geary with a "--hidden" or "--startup" command-line flag. * When that's present, Geary starts as normal but its main window is hidden. * If the user launches Geary from the launcher, the window appears. * Closing the window with the close button merely hides the main window. (The composer windows should remain open.) * AppMenu -> Quit should close Geary entirely, not hide the window. That's my take on the situation. More questions should be directed to this ticket.
Created attachment 276647 [details] [review] An attempt to fix the issue. Here's my proposed fix for it. All points stated above are done and tested. - A command-line flag '--hidden' is added - An option for startup notification is added in preferences dialog - A new class StartupNotification is added under 'client/notification/' for manipulation of .desktop file. - A simple 'geary.desktop' is placed in '$HOME/.config/autostart/' when the user selects the checkbox. Just for a matter of convenience the file existence is checked whenever the preferences dialog is open and the value GSettings key 'start-notifications' is updated accordingly. - GSetting Schema updated; added a key for startup-notifications The main problem I faced is how to place a new file in a way that doesn't mess up the existing organization of the project. A attached a git patch (also here: https://github.com/MohamedIM/geary-unofficial/commit/e593e36bd238edf4ab05ed452ff49cd7598f15e7). I hope that I didn't mess up the code.
- Code updated, creating '$HOME/.config/autostart/' if it doesn't exist. Check latest commit out in this brach (https://github.com/MohamedIM/geary-unofficial/tree/patch) Sorry for that.
Mohamed, this is a very promising start. I've looked it over and there's a few things that still need attention: * The .desktop file in the autostart directory will probably need to be translated -- there may be tools out there that show which applications auto-start, and the .desktop file in the autostart directory will be parsed for that information. One solution is to create a new geary-autostart.desktop.in file in desktop/ that's installed in a safe place on the system and copied into the user's autostart directory when required. This file can be run through gettext like the "real" desktop file so translations are added. The other solution is to generate the file as you're doing today but inject strings marked for translation into the generated file. Note that most of these strings are already translated in GearyApplication, so this might be the easiest way to go. * Do you need the "shown" variable in GearyApplication? It seems to me the logic is pretty simple: if "--hidden" is on the command-line *and* Geary is starting for the first time, don't show the window, otherwise show and present the window. (Perhaps I'm missing a use-case here.) * Please move the line of code for syncing the startup notification with file state into the Preferences dialog. Makes sense to keep all that in one place. * An easier way to store long multiline strings in Vala is with Vala's heredoc variant: // Note the triple double-quotes starting and ending string private const string DESKTOP_FILE = """ [DesktopEntry] Name=Geary GenericName=... """; * Rather than store the pathname in StartupNotification as a string, store it as a File: startup_file = File.new_for_path(Environment.get_user_config_dir()).get_child(AUTOSTART_FOLDER).get_child(GEARY_DESKTOP_FILE); To convert back to an absolute path: startup_file.get_path() Much easier than storing a string and converting it to a File constantly for the various operations. * There's also some issues with code style (i.e. spaces before/after brackets, no extra spaces on line, etc.) but these are minor. Otherwise, this looks pretty good and appears to be working as we want. Keep at it!
Just a note: when the user start geary without the --hidden option, but the autostart is allowd, I wants to see geary continue to run at the background after clicking on the close button. In fact, a better setting is the pref dialog is something like „Notify of new mail at background”. To notify of new mail at startup of the session, we needs a new --hidden argumant, thas all.
(In reply to comment #6) > when the user start geary without the --hidden option, but > the autostart is allowd, I wants to see geary continue to run at the > background after clicking on the close button. That makes sense. > In fact, a better setting is the pref dialog is something like „Notify of new > mail at background”. We're trying to avoid technical-speak in the Preferences dialog. Rather than explain what's going to happen, we want to explain what problem is solved by enabling this option. That's my motivation for the wording of this checkbox. > To notify of new mail at startup of the session, we needs a new --hidden > argumant, thas all. That's not sufficient; we still need to place the .desktop file in the autostart directory (which launches Geary with the --hidden argument). This is another reason why the Preference checkbox is worded as such, because enabling it is what installs the autostart .desktop file. We *could* have two preferences -- Start Geary at login, Notify of new mail in background -- but we don't want to do that, especially since both are closely aligned.
I think we needs also a --quit option, same to nautilus, so the user can to quit geary when the window not shown. it useful for a case I want to restart geary (if it too slow, for example) from command line, wihtout open geary's window.
We could do that as well, but I want to treat that as a separate ticket/patch, as this ticket has enough to do already and a lot of people would like to see this completed.
(In reply to comment #9) > We could do that as well, but I want to treat that as a separate ticket/patch, > as this ticket has enough to do already and a lot of people would like to see > this completed. OK, I filed bug 730537 for this. I just wants to say I really think we needs this option - I use geary with this patch from yesterday, and from time to time I open geary's window, choose quit in the app menu, and than run 'geary --hidden' from the enter command dialog (Alt+F2) and than open again the geary's window.
(In reply to comment #5) Thank you so much for reviewing the patch. Here are some of the things that are done and others need further clarification: * For the .desktop: Since you gave me the choice I'll go with the “desktop/geary-autostart.desktop.in” solution but I'm still trying to figure out how to edit build-related files to include it. * For “shown” variable: I know this is a silly variable but how could I know if the window and its children widgets are already drawn or not since now there's a distinction between a “application starting for first time” and “window shown for the first time”. Also I can't replace the call to MainWindow.present() with MainWindow.show_all(), right ?. May be I'm missing something here. * For syncing file state line: Moved to PreferencesDialog.run() * For coding style: Well, I'll read the guidelines carefully again. (In reply to comment #6) > when the user start geary without the --hidden option, but > the autostart is allowd, I wants to see geary continue to run at the > background after clicking on the close button. Well, I changed it and now the "delete-event" condition is based on the GSettings value instead of the "--hidden" argument. Thank you for this, I missed it. [Repo updated with changes stated above] https://github.com/MohamedIM/geary-unofficial/tree/patch
(In reply to comment #11) > (In reply to comment #6) > > when the user start geary without the --hidden option, but > > the autostart is allowd, I wants to see geary continue to run at the > > background after clicking on the close button. > Well, I changed it and now the "delete-event" condition is based on the > GSettings value instead of the "--hidden" argument. Thank you for this, I > missed it. > > [Repo updated with changes stated above] > https://github.com/MohamedIM/geary-unofficial/tree/patch Thanks! You can to upldae the new patches to this bug?
Mohamed, what Yosef is suggesting is to pull from our master and merge those changes into your branch. Geary has had some large changes in the past two weeks, in particular, the inline composer landed and we moved to GTK+ 3.10. (In reply to comment #11) > * For the .desktop: Since you gave me the choice I'll go with the > “desktop/geary-autostart.desktop.in” solution but I'm still trying to figure > out how to edit build-related files to include it. The only change I think you need to make for that is in desktop/CMakeLists.txt. Specifically, the INTLTOOL_MERGE_DESKTOP and VALIDATE_DESKTOP_FILE commands can be used to generate and process the file (i.e. "INTLTOOL_MERGE_DESKTOP (geary-autostart po)" should do the trick.) Glad to see you got it added to POTFILES.in; that ensures the new desktop file will be translated. > * For “shown” variable: I know this is a silly variable but how could I know if > the window and its children widgets are already drawn or not since now there's > a distinction between a “application starting for first time” and “window shown > for the first time”. Also I can't replace the call to MainWindow.present() with > MainWindow.show_all(), right ?. May be I'm missing something here. Take a look at GtkApplication; it's documentation explains the ordering of the virtual methods, esp. when they're called when the app is the primary instance (starting for the first time) and when it's a secondary instance (running but will eventually signal the primary instance and exit). Don't get too worried about the shown variable; it's a minor thing and not enough to stop getting this patch landed when it's ready. If you can get rid of it, that would be great, but I'm not overly concerned. > Well, I changed it and now the "delete-event" condition is based on the > GSettings value instead of the "--hidden" argument. Thank you for this, I > missed it. Great! This is starting to shape up and I think you're quite close. Please merge in master, see if you can take care of the couple of things listed above, and I'll take another look at it.
Well, I pulled from master branch (from git://git.gnome.org/geary) and merged those changes. For the "INTLTOOL_MERGE_DESKTOP" macro the target is hard-coded as "geary.desktop" so I can't use it for "geary-autostart.desktop" (It generated an error for duplicate targets) so I had to add a new macro "INTLTOOL_AUTOSTART_MERGE_DESKTOP" for the geary-autostart.desktop file. Since I need to call MainWindow.show_all() only when window is not realized I removed the 'shown' flag and based the condition on the realization state of the window (GtkWidget.get_realized()). https://github.com/MohamedIM/geary-unofficial/tree/patch
Created attachment 278898 [details] [review] Final patch Here is the final patch (revert back if applied the previous patch). Waiting for reviewing it.. [BTW, sorry for being late in replying]
Congratulations, Mohamed! This looks great. I did made some small code changes (mostly spacing issues). I did rename the StartupNotification class to AutostartManager, as I felt that's a better description of its duties. Make sure to collect the bounty at Bountysource.com! Pushed to master, commit 6783c2
(Bountysource.com URL: https://www.bountysource.com/issues/1352599-start-notifying-of-new-mail-at-session-startup)
Wow, my first patch accepted! This means a lot to me. Thank you all for your help.