GNOME Bugzilla – Bug 579135
Touchpad practice area
Last modified: 2012-08-30 10:37:44 UTC
It would be nice to have a little practice area where you could test out the various touchpad settings. Perhaps a grey rectangle with an indicator for clicked and scrolling.
I think it's a good idea. Mac OS X includes videos to demonstrate what do the selected settings; having that would be ideal, but for now a grey rectangle for testing should be enough.
Created attachment 222139 [details] [review] new testing are according to mockup There is new mockup for testing area: https://live.gnome.org/Design/SystemSettings/Mouse I attached testing patch for it.
Created attachment 222224 [details] [review] new testing area according to mockup There are some changes according designers ideas. Please review attached patch.
Created attachment 222225 [details] new testing area screenshot
The timeout for the text test feedback should fade out at the same time as the display itself. Right now, the circle will have gone back to its original state, and the text will still say "double-click, primary button". The scroll wheel on a mouse also doesn't work if you bash the mouse buttons over the test area, sometimes. And finally, you're not cleaning up your timeouts when exiting the panel, so this can happen:
+ Trace 230728
Review of attachment 222224 [details] [review]: As per comment, needs-work ::: panels/mouse/gnome-mouse-test.c @@ +38,3 @@ +/* Click test button sizes. */ +#define BUTTON_SIZE 180.0 That should really be dependent on the size of the test area, so that we can only see the button area, and the little girl, not more. @@ +39,3 @@ +/* Click test button sizes. */ +#define BUTTON_SIZE 180.0 +#define SHADDOW_SIZE 10.0 shadow @@ +232,3 @@ } + /* Draw shaddow. */ shadow.
*** Bug 636130 has been marked as a duplicate of this bug. ***
Review of attachment 222224 [details] [review]: ::: panels/mouse/gnome-mouse-test.c @@ +112,3 @@ + switch (button_state) { + case 1: + button_text = _("<b>primary button</b>"); Please keep the markup out of the translatable content - translators are prone to get this wrong. @@ +135,3 @@ + } + + label_text = g_strconcat (click_text, button_text, NULL); Concatenating translated fragments like this makes it often impossible to correctly translate. Better to have a larger list of complete sentences like Single click, primary button ... Double click, secondary button choose the right one according to the state, and then do label_text = g_strconcat ("<b>", message, "</b>", NULL); or, if you want to go the extra mile: label_text = g_markup_printf_escaped ("<b>%s</b>", message);
I really love the visuals here, btw. Great work
(In reply to comment #5) > The timeout for the text test feedback should fade out at the same time as the > display itself. Right now, the circle will have gone back to its original > state, and the text will still say "double-click, primary button". > The timeout for the circle is set to the double click time and the timeout for information label has fixed time. Longer time for information label was aday wish. However both can have same double click time, but user doesn't manage to read the information label in case of short time. I think setting timeout to static value is confusing for user, because the circle change its state before/after the double click time. > The scroll wheel on a mouse also doesn't work if you bash the mouse buttons > over the test area, sometimes. > It's weird, I'm not able to reproduce it. > And finally, you're not cleaning up your timeouts when exiting the panel, so > this can happen: > You are right, I'll fix it. (In reply to comment #6) > Review of attachment 222224 [details] [review]: > > As per comment, needs-work > > ::: panels/mouse/gnome-mouse-test.c > @@ +38,3 @@ > > +/* Click test button sizes. */ > +#define BUTTON_SIZE 180.0 > > That should really be dependent on the size of the test area, so that we can > only see the button area, and the little girl, not more. > Ok, I'll fix it. > @@ +39,3 @@ > +/* Click test button sizes. */ > +#define BUTTON_SIZE 180.0 > +#define SHADDOW_SIZE 10.0 > > shadow > > @@ +232,3 @@ > } > > + /* Draw shaddow. */ > > shadow. Sorry for my English, I'll fix it.
(In reply to comment #8) > Review of attachment 222224 [details] [review]: > > ::: panels/mouse/gnome-mouse-test.c > @@ +112,3 @@ > + switch (button_state) { > + case 1: > + button_text = _("<b>primary button</b>"); > > Please keep the markup out of the translatable content - translators are prone > to get this wrong. > > @@ +135,3 @@ > + } > + > + label_text = g_strconcat (click_text, button_text, NULL); > > Concatenating translated fragments like this makes it often impossible to > correctly translate. Better to have a larger list of complete sentences like > I didn't know it, I'll fix it. (In reply to comment #9) > I really love the visuals here, btw. Great work Thank you.
Created attachment 222621 [details] [review] new testing area according to mockup There is new patch according reviews. Timeouts are set to 2.5 seconds currently in case of double click, however the circle timeout is still set to the double click time in case of single click. Should I set all timeouts to fixed value or how can I set them?
Review of attachment 222621 [details] [review]: Looks good overall. ::: panels/mouse/gnome-mouse-test.c @@ +203,3 @@ + cairo_pattern_t *pattern; + + size = MIN (gtk_widget_get_allocated_width (widget), gtk_widget_get_allocated_height (widget)); Add fallback, just in case, if you don't want to divide by zero in some circumstances. @@ +267,3 @@ + gtk_adjustment_get_upper (adjustment)); + + gdk_rgba_parse (&color, "#565854"); Do we have a bug somewhere about getting a symbolic colour for this? @@ +291,3 @@ + + if (information_label_timeout_id != 0) + g_source_remove (information_label_timeout_id); Dispose can be called multiple times, so reset the _id to zero. @@ +294,3 @@ + + if (button_drawing_area_timeout_id != 0) + g_source_remove (button_drawing_area_timeout_id); Ditto.
Created attachment 222718 [details] [review] reset timeouts id and size fallback
(In reply to comment #13) > Review of attachment 222621 [details] [review]: > > @@ +267,3 @@ > + gtk_adjustment_get_upper (adjustment)); > + > + gdk_rgba_parse (&color, "#565854"); > > Do we have a bug somewhere about getting a symbolic colour for this? > We already have: https://bugzilla.gnome.org/show_bug.cgi?id=682925 Can you write there more information please what you mean with that?
Review of attachment 222718 [details] [review]: Please split this out in 2 patches. But looks good.
Created attachment 222731 [details] [review] reset timeouts id to zero
Created attachment 222732 [details] [review] widget allocated size fallback
Review of attachment 222731 [details] [review]: Looks good
Review of attachment 222732 [details] [review]: Looks good
Thanks Ondrej, I'm looking forward to trying this!