GNOME Bugzilla – Bug 690550
Make a longer timeout and fade-in/out effect for buttons in OSD
Last modified: 2013-07-17 16:03:01 UTC
And possibly for other buttons as well. I'd say a total "on" state of 300ms would be appreciable, though I'll leave the exact behaviour to the designers.
We can easily increase the timeout for touchring/touchstrip but other /regular/ buttons don't use any timeout atm. Actually, the main use of the timeout is to /hide/ the fast press/release events that the ring/strips generate, so that the user has a chance to visualize the event. We could extend this to other regular buttons as well, although those are meant to reflect the actual button state, ie remain depressed as long as the user keeps the real button depressed, and return to a normal state as soon as the real button is released. I did play and tried to use timeouts for other buttons as well when I initially added the timeout and found it would make the OSD look weird, sluggish, it feels slow as if the UI would not react fast enough, imho. Anyway, adding the timeout to regular buttons is a 1 line patch so no big issue technically (I can even post a patch for ppl to try)
Created attachment 232032 [details] [review] Proposed patch to increase timeout to 300ms
Created attachment 232033 [details] [review] Proposed patch to use the same timeout on regular buttons That's the second variant, if we want to use a timeout for regular buttons as well.
Maintainer change
Bastien, Olivier's patches work but they're only for visualization purposes in the OSD window; the buttons are still being pressed/released with their original timeouts. Since I don't know the original purpose of this report, could you confirm if this is only about the OSD and, in which case, if we should commit these?
(In reply to comment #5) > Bastien, Olivier's patches work but they're only for visualization purposes in > the OSD window; the buttons are still being pressed/released with their > original timeouts. It was only for the visualisation. > Since I don't know the original purpose of this report, could you confirm if > this is only about the OSD and, in which case, if we should commit these? See keywords.
Jakub, could you please check which of the options you prefer?
Review of attachment 232032 [details] [review]: Code-wise, it's fine.
Review of attachment 232033 [details] [review]: Code looks fine.
I'm only guessing what the timeout is for. You want to avoid accidental triggers, but how could these happen? The touchstrip being close to the rest position of a hand? The touchstrip possibly registering opposing direction movements? 300ms seems like quite a long threshold to register a tap, seems ok for the strip. I'd love to try it out :/ My model only has regular click buttons, which I hope would not be treated this way.
From IRC: <hadess> jimmac, the touchstrip timeout if for display in the OSD <hadess> jimmac, otherwise the button only flickers on <jimmac> hadess, OH! <jimmac> hadess, disregard my completely confused comment and go for it for everything. having a fade would be extra awesome <hadess> jimmac, and in terms of the duration? <jimmac> 300-500 range Let's make it 300 for now, and we'll tweak it before committing.
Created attachment 246789 [details] [review] wacom: Make the highlight duration of buttons in the OSD longer It also adds a fade-in/fade-out effect to the buttons.
Created attachment 246790 [details] [review] wacom: Remove the auto_now property from the OSD's buttons And everything related to it as it is no longer necessary.
Review of attachment 246789 [details] [review]: The code looks fine, but I'm not really sure that the animation matches what Jakub was expecting.
Review of attachment 246790 [details] [review]: auto_now?
(In reply to comment #15) > Review of attachment 246790 [details] [review]: > > auto_now? Huh, yeah, auto_off :) I'll change it.
Created attachment 247463 [details] [review] wacom: Remove the auto_off property from the OSD's buttons And everything related to it as it is no longer necessary.
Hi, I recorded a video with the transition, should it be easier for Jakub to check it out: https://docs.google.com/file/d/0B9mCgEAuiw_IWmYwZXhkZFZhQnM/edit?usp=sharing
Created attachment 247987 [details] [review] wacom: Make the highlight duration of buttons in the OSD longer It also adds a fade-in/fade-out effect to the buttons. This new version makes the buttons stay on if they're pressed again before the on state starts transitioning to off which makes e.g. strip/ring buttons stay on if they're constantly pressed. Jakub was the one to suggest this enhancement and said that the transition is okay for him so, if the code looks good, we should commit this ;)
Jakub has tried this so I am removing the ui-review keyword.
Review of attachment 247463 [details] [review]: Looks good.
Review of attachment 247987 [details] [review]: Getting there! ::: plugins/wacom/gsd-wacom-osd-window.c @@ +247,3 @@ GsdWacomTabletButtonPos position; gboolean active; + gboolean next_state; /* what the next state should be "active" or not */ Missing a comma in the sentence: /* what the next state should be, "active" or not */ @@ +251,3 @@ guint auto_off; guint timeout; + gint timer; Could you separate and group all the animation variables and add a comment what they're used for? /* Animations */ guint...; etc. Also, what's "timer"? I would have expected a "GTimer" object instead. Time left? Time to next animation frame? @@ +355,3 @@ + gint total_timeout; + gdouble transition_step; + gboolean remove_source = G_SOURCE_CONTINUE; remove_source isn't very well named. How about simply "ret"? @@ +413,2 @@ osd_button->priv->active = active; + osd_button->priv->timeout = g_timeout_add (BUTTON_TIMER_STEP, timeout_id = ... @@ +415,3 @@ + (GSourceFunc) gsd_wacom_osd_button_timer, + osd_button); + No need for the empty line here. @@ +664,3 @@ + green = rgb_value_to_hex(color->green * 255); + blue = rgb_value_to_hex(color->blue * 255); + hex_color = g_strdup_printf ("#%s%s%s", red, green, blue); Wouldn't something like: g_strdup_printf ("#%02X%02X%02X", color->red * 255, color->green * 255...); work? Saves you the intermediate steps as well. @@ +688,3 @@ + +static gchar * +gsd_wacom_osd_button_get_color_str (GsdWacomOSDButton *osd_button) The code in here is overly complicated. from_color = ... to_color = ... if (timeout_id > 0) percentage = priv->transition_percentage; else percentage = 1.0; And optimise transition_rgba_colors() for 0.0 and 1.0 values. @@ +743,3 @@ pango_layout_set_markup (layout, markup, -1); g_free (markup); + g_free (color_str); free the color_str as soon as it's not needed anymore, so after the g_strdup_printf() above.
Created attachment 249276 [details] [review] wacom: Make the highlight duration of buttons in the OSD longer Latest changes addressing Bastien's comments.
Created attachment 249277 [details] [review] wacom: Make the highlight duration of buttons in the OSD longer New version addressing Bastien's comments.
Review of attachment 249277 [details] [review]: Rest looks good. ::: plugins/wacom/gsd-wacom-osd-window.c @@ +48,3 @@ +#define BUTTON_HIGHLIGHT_DURATION 250 /* ms */ +#define BUTTON_TRANSITION_DURATION 150 /* ms */ +#define BUTTON_TIMER_STEP 25 /* ms */ align the comment to the others. @@ +251,3 @@ + GdkRGBA active_color; + GdkRGBA inactive_color; + /* Animations */ A linefeed just before this section. @@ +254,3 @@ + gboolean next_state; /* what the next state should be, "active" or not */ + guint timeout_id; + gint elapsed_time; mention the unit (ms).
Pushed with the requested changes. commit 11a3dbdb1e4723bd3c87e8f7f2df3b8ff38b6059 commit af4cea132d6c22931900f348db7f28635102f418