Comment 2 for bug 497857

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