After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 714644 - Start notifying of new mail at session startup
Start notifying of new mail at session startup
Status: RESOLVED FIXED
Product: geary
Classification: Other
Component: notifications
master
Other All
: High normal
: 0.8.0
Assigned To: Geary Maintainers
Geary Maintainers
review
Depends on:
Blocks:
 
 
Reported: 2013-02-09 01:23 UTC by Jim Nelson
Modified: 2014-06-27 23:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
An attempt to fix the issue. (13.85 KB, patch)
2014-05-16 02:47 UTC, Mohamed Ibrahim
none Details | Review
Final patch (15.17 KB, patch)
2014-06-21 16:28 UTC, Mohamed Ibrahim
none Details | Review

Description Charles Lindsay 2013-11-21 20:28:29 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 

Comment 1 Jim Nelson 2014-01-09 19:32:35 UTC
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.
Comment 2 Jim Nelson 2014-05-15 02:20:48 UTC
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.
Comment 3 Mohamed Ibrahim 2014-05-16 02:47:55 UTC
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.
Comment 4 Mohamed Ibrahim 2014-05-16 05:15:42 UTC
- 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.
Comment 5 Jim Nelson 2014-05-19 21:09:53 UTC
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!
Comment 6 Yosef Or Boczko 2014-05-21 10:46:04 UTC
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.
Comment 7 Jim Nelson 2014-05-21 17:55:57 UTC
(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.
Comment 8 Yosef Or Boczko 2014-05-21 17:59:47 UTC
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.
Comment 9 Jim Nelson 2014-05-21 18:27:41 UTC
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.
Comment 10 Yosef Or Boczko 2014-05-21 18:32:14 UTC
(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.
Comment 11 Mohamed Ibrahim 2014-05-30 16:39:41 UTC
(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
Comment 12 Yosef Or Boczko 2014-06-01 00:00:09 UTC
(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?
Comment 13 Jim Nelson 2014-06-03 22:33:28 UTC
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.
Comment 14 Mohamed Ibrahim 2014-06-12 18:13:26 UTC
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
Comment 15 Mohamed Ibrahim 2014-06-21 16:28:42 UTC
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]
Comment 16 Jim Nelson 2014-06-26 20:35:24 UTC
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
Comment 18 Mohamed Ibrahim 2014-06-27 23:59:37 UTC
Wow, my first patch accepted! This means a lot to me.
Thank you all for your help.