Ubuntu

Support application indicators

Reported by Jorge O. Castro on 2009-12-17
10
This bug affects 1 person
Affects Status Importance Assigned to Milestone
gnome-control-center
Fix Released
Wishlist
gnome-control-center (Ubuntu)
Wishlist
Sebastien Bacher

Bug Description

Binary package hint: gnome-control-center

This application should be investigated to be ported to use Application Indicators for Lucid - https://edge.launchpad.net/indicator-application

Changed in gnome-control-center (Ubuntu):
importance: Undecided → Wishlist
status: New → Triaged
Jorge O. Castro (jorge) on 2010-02-01
Changed in gnome-control-center (Ubuntu):
assignee: Canonical Desktop Experience Team (canonical-dx-team) → Travis B. Hartwell (nafai)
Changed in gnome-control-center (Ubuntu):
status: Triaged → In Progress
Travis B. Hartwell (nafai) wrote :

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.

Changed in gnome-control-center (Ubuntu):
assignee: Travis B. Hartwell (nafai) → Ken VanDine (ken-vandine)
status: In Progress → Fix Committed
Changed in gnome-control-center (Ubuntu):
assignee: Ken VanDine (ken-vandine) → Sebastien Bacher (seb128)
Sebastien Bacher (seb128) wrote :

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

Travis B. Hartwell (nafai) wrote :
Download full text (3.7 KiB)

>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...

Read more...

tags: added: patch
Travis B. Hartwell (nafai) wrote :

Attached is an updated patch:

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

Travis B. Hartwell (nafai) wrote :

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.

Launchpad Janitor (janitor) wrote :

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

Changed in gnome-control-center (Ubuntu):
status: Fix Committed → Fix Released
Changed in gnome-control-center:
status: Unknown → Fix Released
Changed in gnome-control-center:
importance: Unknown → Wishlist
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.