>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 way in which the
current information is conveyed. Would be it be clearer if the time
left was an insensitive menu item alone at the very top of the menu?
>is ngettext() required there is the singular and plurial forms have no change?
Probably not, I bet I could just use the _N() macro.
>> +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
Good catch, I'll fix that.
>> init_tray_icon (dr);
>>-
>> g_timeout_add_seconds (12,
>
>the blank line change is not required
As noted above, whoops.
--
Support application indicators
https://bugs.launchpad.net/bugs/497857
You received this bug notification because you are a member of Ubuntu
Desktop Bugs, which is subscribed to gnome-control-center in ubuntu.
--
desktop-bugs mailing list
desktop-bugs@lists.ubuntu.com
https://lists.ubuntu.com/mailman/listinfo/desktop-bugs
