Support application indicators

Note:
As agreed on with jcastro, this only supports showing green when plenty of time before locked -> showing red right before -> locking.

The original behavior shows gradients of green as time progresses and then flashing red right before locking, then red when locked.

Thank you for your work there, some comment:

> GtkStatusIcon *icon;
>-

Why the blank line change?

> +#ifdef HAVE_APP_INDICATOR
> +const char TYPING_MONITOR_ACTIVE_ICON[] = "bar-green";
> +const char TYPING_MONITOR_ATTENTION_ICON[] = "bar-red";

you can use "#define TYPING_MONITOR_ACTIVE_ICON "bar-green"" rather, that's what GNOME does usually

> +static gint get_time_left (DrWright *drwright);

this change seems to be code cleaning

> +update_app_indicator (DrWright *dr)
> + current_status = app_indicator_get_status (dr->indicator);
> + if (new_status != current_status) {

no need to get the current status and compare to the new one in this function, could can just call set_status on the new one, if there is no status change that will just do nothing

> +#ifndef HAVE_APP_INDICATOR
> gtk_status_icon_set_from_pixbuf (dr->icon,
> dr->composite_bar);
>+#endif
> } else {
>+#ifndef HAVE_APP_INDICATOR
> gtk_status_icon_set_from_pixbuf (dr->icon,
> dr->neutral_bar);
>+#endif

no need to have 2 ifndef there, you can as well use one since there no action to do in either case

> stop_blinking (dr);
> +

the new line adding there doesn't seem required

> + const gchar normal_msg_template[] = "Take a break now (next in %dm)";
> + const gchar less_than_one_msg_template[] = "Take a break now (next in less than one minute)";

why do you use strings different than the upstream ones? the upstream labels indicate that the indication is the time before next break where you state "take a break now", did you change how the software works? aren't just menu indications about the next break? in that's the case where should the user take a break now when looking when is the next one?

is ngettext() required there is the singular and plurial forms have no change?

> +get_time_left (DrWright *dr)
> +{
> + gint elapsed_time, min;

the min variable seems not required there, you can as well use return directly the value

> init_tray_icon (dr);
>-
> g_timeout_add_seconds (12,

the blank line change is not required

>Thank you for your work there, some comment:
>
>> GtkStatusIcon *icon;
>>-
>
>Why the blank line change?

These were probably all unintentional, I have since found a parameter
with git diff that allows me to ignore whitespace changes when
creating diffs that I will use. They partly came from calling M-x
delete-trailing-whitespace in Emacs, which possibly removed whitespace
that was there in the original code. I'll make sure there are no
extraneous lines in future patches.

>> +#ifdef HAVE_APP_INDICATOR
>> +const char TYPING_MONITOR_ACTIVE_ICON[] = "bar-green";
>> +const char TYPING_MONITOR_ATTENTION_ICON[] = "bar-red";
>
>you can use "#define TYPING_MONITOR_ACTIVE_ICON "bar-green"" rather, that's what GNOME does usually

Good hint, thanks.

>> +static gint get_time_left (DrWright *drwright);
>
>this change seems to be code cleaning

I did this so I didn't copy and paste code between the two methods
that needed this functionality between the old update_tooltip () and
the new update_menu_text () functionality.

>> +update_app_indicator (DrWright *dr)
>> + current_status = app_indicator_get_status (dr->indicator);
>> + if (new_status != current_status) {
>
>no need to get the current status and compare to the new one in this function, could can just call set_status on the new one, if there is no status change that will just do nothing

Good to know, I noticed you asking this question earlier, I'll remove
the extraneous check.

>> +#ifndef HAVE_APP_INDICATOR
>> gtk_status_icon_set_from_pixbuf (dr->icon,
>> dr->composite_bar);
>>+#endif
>> } else {
>>+#ifndef HAVE_APP_INDICATOR
>> gtk_status_icon_set_from_pixbuf (dr->icon,
>> dr->neutral_bar);
>>+#endif
>
>no need to have 2 ifndef there, you can as well use one since there no action to do in either case

Oh duh. :)

>> stop_blinking (dr);
>> +
>
>the new line adding there doesn't seem required
>
>> + const gchar normal_msg_template[] = "Take a break now (next in %dm)";
>> + const gchar less_than_one_msg_template[] = "Take a break now (next in less than one minute)";
>
>why do you use strings different than the upstream ones? the upstream labels indicate that the indication is the time before next break where you state "take a break now", did you change how the software works? aren't just menu indications about the next break? in that's the case where should the user take a break now when looking when is the next one?

This was from a suggestion from mpt. Previously, the information on
how much time left was indicated via a tooltip, which application
indicators don't support.

To quote from an e-mail from mpt:

---------- quote ------------------
For the typing break in particular, I suggest including the time in the
"Take a Break" item, like this:
   .--------------------------------.
   | Take a Break Now (next in 7m) |
   |--------------------------------|
   | Settings… |
   | About Typing Break |
   '--------------------------------'
----------- end quote ---------------

The reason we shortened the text there is the previous strings were
really long and would have made the menu strangely long. I tried not
to change the way the software works, just the wa...

Attached is an updated patch:

1) following recommendations from Sebastien above
2) updating the configure.ac with the latest suggestions from Ken

Hopefully final changes, based on what Sebastien pointed out on IRC, and another thing I noticed:
1) preprocessed out the code to blink completely in the case of using app indicators
2) the #ifdef/#endif's sometimes were separated by a bit, so I added a comment on each #endif to be

#endif /* HAVE_APP_INDICATOR */

?field.comment=Hopefully final changes, based on what Sebastien pointed out on IRC, and another thing I noticed:
1) preprocessed out the code to blink completely in the case of using app indicators
2) the #ifdef/#endif's sometimes were separated by a bit, so I added a comment on each #endif to be

#endif /* HAVE_APP_INDICATOR */

Should be ready for upstream.

This bug was fixed in the package gnome-control-center - 1:2.29.90-0ubuntu2

---------------
gnome-control-center (1:2.29.90-0ubuntu2) lucid; urgency=low

  * debian/patches/02_use_application_indicator.patch:
    - use the lucid application indicator, thanks Travis B. Hartwell
      (lp: #497857)
  * debian/patches/99_autoreconf.patch:
    - refreshed for the previous change to build
 -- Sebastien Bacher <email address hidden> Thu, 11 Feb 2010 15:10:32 +0100