GNOME Bugzilla – Bug 677206
New mouse and touchpad design
Last modified: 2012-08-23 08:58:39 UTC
The mouse and touchpad panel needs some love. Here are some mockups for an updated design: https://github.com/gnome-design-team/gnome-mockups/raw/master/system-settings/mouse-and-touchpad/mouse-and-touchpad.png
There are additional information: https://live.gnome.org/Design/SystemSettings/Mouse
Created attachment 219686 [details] [review] reorganize widget according to mockup I reorganized widget according to the mockup and removed unnecessary options. How can I calculate the pointer speed from the acceleration and the sensitivity? Where can I set the new options from mockup (touchpad double click, content sticks to fingers)? Are there any d-conf options for that? Please review attached patch.
Created attachment 219687 [details] screenshot of reorganized panel
(In reply to comment #2) > > Where can I set the new options from mockup (touchpad double click, content > sticks to fingers)? Are there any d-conf options for that? It doesn't look like we do have these currently. They should go in org.gnome.settings-daemon.peripherals.touchpad. The source file for this schema is in gnome-settings-daemon/data/org.gnome.settings-daemon.peripherals.gschema.xml.in.in You should add new settings there.
@Ondrej, The sliders are too long (too unpractical to use). It would be great if you could limit their width as in Allan design.
Information for how to implement 'content sticks to finger' (aka 'natural scrolling') can be found in bug 674716
Hi, thank you for your comments. (In reply to comment #4) > (In reply to comment #2) > > > > > Where can I set the new options from mockup (touchpad double click, content > > sticks to fingers)? Are there any d-conf options for that? > > It doesn't look like we do have these currently. > They should go in org.gnome.settings-daemon.peripherals.touchpad. > The source file for this schema is in > gnome-settings-daemon/data/org.gnome.settings-daemon.peripherals.gschema.xml.in.in > You should add new settings there. Yes, I will add them. (In reply to comment #6) > Information for how to implement 'content sticks to finger' (aka 'natural > scrolling') can be found in bug 674716 I will look at this. (In reply to comment #5) > @Ondrej, The sliders are too long (too unpractical to use). It would be great > if you could limit their width as in Allan design. I will fix this.
Created attachment 220042 [details] [review] limit slider width as in mockup
Created attachment 220242 [details] [review] add new touchpad options to panel
Created attachment 220243 [details] [review] add new touchpad options into gnome-setting-daemon schema
Created attachment 220244 [details] screenshot with limited sliders and new options
Created attachment 220570 [details] [review] sensitivity and acceleration sliders merge
Created attachment 220571 [details] [review] add touchpad natural scroll implementation
Created attachment 220574 [details] screenshot with merged acceleration and sensitivity sliders
Created attachment 220662 [details] [review] add touchpad natural scroll implementation using synaptics scrolling distance I attached an implementation for natural scrolling using synaptics scrolling distance. Previous version was misunderstanding using set button map.
Created attachment 220797 [details] [review] reorganize widgets according to mockup Hi! Mockup was slightly changed, so there are new patches. I have also fixed commiter e-mail and added the bug address into the commit messages. Please review attached patches.
Created attachment 220798 [details] [review] sensitivity and acceleration sliders merge
Created attachment 220799 [details] [review] add natural scroll toggle
Created attachment 220800 [details] [review] show mouse section only if mouse device is present
Created attachment 220801 [details] screenshot after mockup change
Necessary fixed patches for gnome-settings-daemon are at https://bugzilla.gnome.org/show_bug.cgi?id=659498 .
Created attachment 220864 [details] [review] add test your settings widget
Created attachment 220865 [details] test your settings widget screenshot
I can't test this: (gnome-control-center:17419): GLib-GIO-ERROR **: Settings schema 'org.gnome.settings-daemon.peripherals.touchpad' does not contain a key named 'natural-scroll-enabled' Where's the related gnome-settings-daemon changes?
Review of attachment 220797 [details] [review]: That looks alright, can't test the UI though. ::: panels/mouse/gnome-mouse-properties.c @@ +66,3 @@ method = g_settings_get_enum (touchpad_settings, "scroll-method"); + if (method == GSD_TOUCHPAD_SCROLL_METHOD_TWO_FINGER_SCROLLING) I'd prefer: active = (method == GSD_TOUCHPAD_SCROLL_METHOD_TWO_FINGER_SCROLLING)); gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (WID ("two_finger_scroll_toggle")), active);
Review of attachment 220798 [details] [review]: This looks fine, though did you check that particular code with somebody? ::: panels/mouse/gnome-mouse-properties.c @@ +150,3 @@ + + value = g_variant_new_double (gtk_range_get_value (scale)); + g_settings_set_value (settings, "motion-acceleration", value); Why not g_settings_set_double()? @@ +154,3 @@ + adjustment = gtk_range_get_adjustment (scale); + value = g_variant_new_int32 (gtk_adjustment_get_upper (adjustment) - gtk_range_get_value (scale) + 1); + g_settings_set_value (settings, "motion-threshold", value); Why not g_settings_set_int()?
Review of attachment 220799 [details] [review]: ::: panels/mouse/gnome-mouse-properties.c @@ +200,3 @@ WID ("tap_to_click_toggle"), "active", G_SETTINGS_BIND_DEFAULT); + g_settings_bind (touchpad_settings, "natural-scroll-enabled", This also means that you'll need to bump the dependency on gnome-settings-daemon, and upload the g-s-d patch somewhere.
Review of attachment 220800 [details] [review]: The gsd-input-helper.[ch] code lives in gnome-settings-daemon, so that's where the changes need to be made.
Review of attachment 220864 [details] [review]: Add the gnome-mouse-test.* files separately, and make sure it comes with a test-application (so that it can be debugged and tested separately from the rest of the panel). ::: panels/mouse/cc-mouse-panel.c @@ +227,3 @@ gtk_widget_reparent (prefs_widget, GTK_WIDGET (self)); + + g_idle_add (add_shell_test_button_cb, self); This is pretty gross, and shouldn't be needed.
(In reply to comment #24) > I can't test this: > (gnome-control-center:17419): GLib-GIO-ERROR **: Settings schema > 'org.gnome.settings-daemon.peripherals.touchpad' does not contain a key named > 'natural-scroll-enabled' > > Where's the related gnome-settings-daemon changes? It was in bug 659498. Note that I've changed the key name to "natural-scroll". The enabled was redundant.
Review of attachment 220864 [details] [review]: ::: panels/mouse/cc-mouse-panel.c @@ +83,3 @@ + } + + if (priv->prefs_widget) This and... @@ +89,3 @@ + } + + if (priv->test_widget) ... this won't be needed if you stuff both widgets inside a GtkNotebook. @@ +138,3 @@ + if (gtk_toggle_button_get_active (button)) + { + gtk_container_remove (GTK_CONTAINER (panel), WID ("prefs_widget")); Don't do that. Use a GtkNotebook without tabs, and switch the current page when needed. @@ +180,3 @@ + GtkWidget *parent; + + size_group = gtk_size_group_new (GTK_SIZE_GROUP_BOTH); And not removing/adding the widgets means you can remove this code. @@ +227,3 @@ gtk_widget_reparent (prefs_widget, GTK_WIDGET (self)); + + g_idle_add (add_shell_test_button_cb, self); I see that the network panel does the same thing. Please file a bug and we'll try and fix it later.
Also note that the panel is centered instead of aligned to the top when you only have a mouse.
Hi! Thank you for reviews. (In reply to comment #24) > I can't test this: > (gnome-control-center:17419): GLib-GIO-ERROR **: Settings schema > 'org.gnome.settings-daemon.peripherals.touchpad' does not contain a key named > 'natural-scroll-enabled' > > Where's the related gnome-settings-daemon changes? I am sorry, I don't have right for adding bug dependencies. (In reply to comment #25) > Review of attachment 220797 [details] [review]: > > That looks alright, can't test the UI though. > > ::: panels/mouse/gnome-mouse-properties.c > @@ +66,3 @@ > method = g_settings_get_enum (touchpad_settings, "scroll-method"); > > + if (method == GSD_TOUCHPAD_SCROLL_METHOD_TWO_FINGER_SCROLLING) > > I'd prefer: > active = (method == GSD_TOUCHPAD_SCROLL_METHOD_TWO_FINGER_SCROLLING)); > gtk_toggle_button_set_active (GTK_TOGGLE_BUTTON (WID > ("two_finger_scroll_toggle")), active); I'll fix this. (In reply to comment #26) > Review of attachment 220798 [details] [review]: > > This looks fine, though did you check that particular code with somebody? I didn't check the code with anybody, though I discussed way of computing pointer speed with aday only. > ::: panels/mouse/gnome-mouse-properties.c > @@ +150,3 @@ > + > + value = g_variant_new_double (gtk_range_get_value (scale)); > + g_settings_set_value (settings, "motion-acceleration", value); > > Why not g_settings_set_double()? > > @@ +154,3 @@ > + adjustment = gtk_range_get_adjustment (scale); > + value = g_variant_new_int32 (gtk_adjustment_get_upper (adjustment) - > gtk_range_get_value (scale) + 1); > + g_settings_set_value (settings, "motion-threshold", value); > > Why not g_settings_set_int()? I'll fix these functions.
Created attachment 221833 [details] [review] reorganize widgets according to mockup
Created attachment 221834 [details] [review] sensitivity and acceleration sliders merge
Created attachment 221835 [details] [review] add natural scroll toggle
Created attachment 221836 [details] [review] add mouse present detection for gnome-settings-daemon
Created attachment 221837 [details] [review] show mouse section only if mouse device is present
A lot of the problems I mentioned in my earlier reviews still stand. Use a Notebook and stop reparenting the widgets, this will cleanup a lot of code already, then fix the widgets being centered when there's only a mouse present.
(In reply to comment #31) > Review of attachment 220864 [details] [review]: > > ::: panels/mouse/cc-mouse-panel.c > @@ +83,3 @@ > + } > + > + if (priv->prefs_widget) > > This and... > > @@ +89,3 @@ > + } > + > + if (priv->test_widget) > > ... this won't be needed if you stuff both widgets inside a GtkNotebook. I don't understand, why this code won't be needed. Still we need to call gnome_mouse_properties_dispose and gnome_mouse_properties_test, didn't we? > > @@ +138,3 @@ > + if (gtk_toggle_button_get_active (button)) > + { > + gtk_container_remove (GTK_CONTAINER (panel), WID ("prefs_widget")); > > Don't do that. Use a GtkNotebook without tabs, and switch the current page when > needed. Ok, I'll use GtkNotebook. (In reply to comment #39) > A lot of the problems I mentioned in my earlier reviews still stand. Use a > Notebook and stop reparenting the widgets, this will cleanup a lot of code > already, then fix the widgets being centered when there's only a mouse present. Sorry, I am working on it. I forget to mention it.
(In reply to comment #40) > (In reply to comment #31) > > Review of attachment 220864 [details] [review] [details]: > > > > ::: panels/mouse/cc-mouse-panel.c > > @@ +83,3 @@ > > + } > > + > > + if (priv->prefs_widget) > > > > This and... > > > > @@ +89,3 @@ > > + } > > + > > + if (priv->test_widget) > > > > ... this won't be needed if you stuff both widgets inside a GtkNotebook. > > I don't understand, why this code won't be needed. Still we need to call > gnome_mouse_properties_dispose and gnome_mouse_properties_test, didn't we? You're right. Please use g_clear_object() though.
Created attachment 221864 [details] [review] add test your settings widget
Created attachment 221865 [details] [review] add test your settings widget toggle
Congratulations on getting this finished for GNOME 3.6, Ondrej! This will be a great new feature, and I know that a lot of people in the GNOME community are really happy about it. I've been really impressed with your work so far.
(In reply to comment #44) > Congratulations on getting this finished for GNOME 3.6, Ondrej! This will be a > great new feature, and I know that a lot of people in the GNOME community are > really happy about it. I've been really impressed with your work so far. Thank you, I enjoy working on Gnome :-)
(In reply to comment #31) > Review of attachment 220864 [details] [review]: > > @@ +227,3 @@ > gtk_widget_reparent (prefs_widget, GTK_WIDGET (self)); > + > + g_idle_add (add_shell_test_button_cb, self); > > I see that the network panel does the same thing. Please file a bug and we'll > try and fix it later. Sorry, I don't know what is wrong with that actually, so I'm not able to file a bug.
(In reply to comment #46) > (In reply to comment #31) > > Review of attachment 220864 [details] [review] [details]: > > > > @@ +227,3 @@ > > gtk_widget_reparent (prefs_widget, GTK_WIDGET (self)); > > + > > + g_idle_add (add_shell_test_button_cb, self); > > > > I see that the network panel does the same thing. Please file a bug and we'll > > try and fix it later. > > Sorry, I don't know what is wrong with that actually, so I'm not able to file a > bug. You shouldn't need to have that code in an idle handler.
*** Bug 640076 has been marked as a duplicate of this bug. ***