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
--
Support application indicators
https://bugs.launchpad.net/bugs/497857
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
--
ubuntu-bugs mailing list
ubuntu-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
