GNOME Bugzilla – Bug 756863
Port Mouse & Touch panel to the new design
Last modified: 2015-11-27 14:30:24 UTC
Created attachment 313739 [details] screenshot https://wiki.gnome.org/Design/SystemSettings/Mouse#Tentative_Design * Natural scrolling is now a General setting, which means it applies for mouse and touchpad. Therefore, in this implementation we mirror the "natural-scroll" key for org.gnome.settings-daemon.peripherals.mouse and org.gnome.settings-daemon.peripherals.touchpad; * The "Multitouch Gestures" entry is missing because we still don't have the api for that; * Disabling the Touchpad makes the touchpad options insensitive (maybe we want it to hide the touchpad options below instead); * Disabling the "Two Finger Scrolling" switch sets the "scroll-method" key to Disabled (instead of "edge-scrolling", do we want it this way?); * We show the "Edge Scrolling" entry just when there's no "Two Finger Scrolling" support; * Disabling the "Edge Scrolling" switch sets the scroll-method key to Disabled too;
Created attachment 313740 [details] [review] mouse: port panel to the new design https://wiki.gnome.org/Design/SystemSettings/Mouse
(In reply to Felipe Borges from comment #0) > Created attachment 313739 [details] > screenshot > > https://wiki.gnome.org/Design/SystemSettings/Mouse#Tentative_Design > > * Natural scrolling is now a General setting, which means it applies for > mouse and touchpad. Therefore, in this implementation we mirror the > "natural-scroll" key for org.gnome.settings-daemon.peripherals.mouse and > org.gnome.settings-daemon.peripherals.touchpad; That's what we had at first, but we should have separate settings in the panel instead. > * The "Multitouch Gestures" entry is missing because we still don't have the > api for that; > > * Disabling the Touchpad makes the touchpad options insensitive (maybe we > want it to hide the touchpad options below instead); I would hide the touchpad items in a GtkRevealer. > * Disabling the "Two Finger Scrolling" switch sets the "scroll-method" key > to Disabled (instead of "edge-scrolling", do we want it this way?); That's what we want. > * We show the "Edge Scrolling" entry just when there's no "Two Finger > Scrolling" support; That's fine. > * Disabling the "Edge Scrolling" switch sets the scroll-method key to > Disabled too; That's fine. Note that there may be more than one touchpad in a machine (builtin touchpad + external one for example), so there's that to keep in mind when building the UI. We should also have a patch to Universal Access to add the double-click delay.
Review of attachment 313740 [details] [review]: I mentioned this on IRC, but I would really have preferred a commit that moved all the detection code to be separate first, and then do the redesign. Please make sure not to include white space changes in your patch either, this only makes reviewing harder. ::: panels/mouse/gnome-mouse-panel.desktop.in.in @@ +1,2 @@ [Desktop Entry] +_Name=Mouse & Touch A bit premature to be doing that when we don't have touchscreen gestures support yet. ::: panels/mouse/gnome-mouse-properties.c @@ +254,3 @@ get_touchpad_enabled (GSettings *settings) { + GDesktopDeviceSendEvents send_events; White space changes?
Created attachment 313851 [details] [review] mouse: decouple the scroll-methods detection code from the UI file
Comment on attachment 313740 [details] [review] mouse: port panel to the new design lets start with decoupling the detection code first.
Review of attachment 313851 [details] [review]: The code in cc-mouse-caps-helper.c is a copy/paste, right? ::: panels/mouse/cc-mouse-caps-helper.c @@ +22,3 @@ + +static ScrollingMethods * +synaptics_check_capabilities_x11 () (void). The only time you should ever see that is when you call a function. @@ +108,3 @@ + +ScrollingMethods * +synaptics_check_capabilities () Again here. @@ +113,3 @@ + return synaptics_check_capabilities_x11 (); + /* else we unconditionally show all touchpad knobs */ + return NULL; You just made g-c-c crash under Wayland. ::: panels/mouse/cc-mouse-caps-helper.h @@ +34,3 @@ +} ScrollingMethods; + +ScrollingMethods *synaptics_check_capabilities (void); The usual way to handle this would be either the caller has that method on the stack, or you return each of the elements. So: gboolean synaptics_check_capabilities (gboolean *have_two_finger_scrolling, gboolean *have_edge_scrolling, gboolean *have_tap_to_click); Is what I would prefer.
Created attachment 313858 [details] [review] mouse: decouple the scroll-methods detection code from the UI file
yes, it is the same code used before on synaptics_check_capabilities_x11 at gnome-mouse-properties.c
Review of attachment 313858 [details] [review]: ::: panels/mouse/cc-mouse-caps-helper.h @@ +28,3 @@ +#include <X11/extensions/XInput.h> + +gboolean synaptics_check_capabilities (gboolean *have_two_finger_scrolling, Where's have_edge_scrolling gone? Or is that coming back when you move another function into the same file. If that's the case, looks good to be merged.
exactly, in the current UI we don't have a toggle for edge scrolling. But it is coming back on the next patch. https://git.gnome.org/browse/gnome-control-center/commit/?id=5cd231fd59075bf3f727d05b19d9abf7deac90c8
Created attachment 313861 [details] [review] mouse: port panel to the new design https://wiki.gnome.org/Design/SystemSettings/Mouse
Review of attachment 313861 [details] [review]: Looks good overall, needs testing and UI review. ::: panels/mouse/gnome-mouse-properties.c @@ +260,3 @@ touchpad_enabled_set_mapping, NULL, NULL); + White space change here.
Review of attachment 313861 [details] [review]: Bugs found during testing: - the testing area should be in the main window, not as a separate dialogue. - there's extra padding all around the panel, which is very visible as the right-handside scrollbar is a couple of pixels away from the edge of the window, and you can see the dotted line showing more content is available isn't at the bottom (or top if you scrolled down) of the window either. - the mouse speed and touchpad speed sliders should be expanded, like the double-click delay slider in the universal access panel. - I see a mouse speed slider, even though I don't have any mice (I have 2 touchpads, one external one, and one in my closed laptop). This isn't a regression, but worth filing a separate bug about. - I think there's too much padding on the sides, but Allan can double-check that.
Review of attachment 313861 [details] [review]: ::: panels/mouse/cc-mouse-caps-helper.c @@ +41,2 @@ *have_two_finger_scrolling = FALSE; + *have_edge_scrolling = FALSE; You should also set have_edge_scrolling to TRUE unconditionally if xorg-x11-drv-synaptics is used...
Created attachment 313916 [details] screencast (Test Your Settings approach) (In reply to Bastien Nocera from comment #13) > Review of attachment 313861 [details] [review] [review]: > > Bugs found during testing: > - the testing area should be in the main window, not as a separate dialogue. > - there's extra padding all around the panel, which is very visible as the > right-handside scrollbar is a couple of pixels away from the edge of the > window, and you can see the dotted line showing more content is available > isn't at the bottom (or top if you scrolled down) of the window either. > - the mouse speed and touchpad speed sliders should be expanded, like the > double-click delay slider in the universal access panel. Got it. > - I see a mouse speed slider, even though I don't have any mice (I have 2 > touchpads, one external one, and one in my closed laptop). This isn't a > regression, but worth filing a separate bug about. Will do it. It seems that mouse_is_present in panels/common/gsd-device-manager is returning True even without a mice. I can reproduce it here too. I will try to fix that. > - I think there's too much padding on the sides, but Allan can double-check > that. I got the same margin values from the Privacy panel (although the Mouse panel is wider). This screencast includes a way to show the _Test widget_ that could possibly be done differently. Opinions?
(In reply to Felipe Borges from comment #15) > Created attachment 313916 [details] > screencast (Test Your Settings approach) > > (In reply to Bastien Nocera from comment #13) > > Review of attachment 313861 [details] [review] [review] [review]: > > > > Bugs found during testing: > > - the testing area should be in the main window, not as a separate dialogue. > > - there's extra padding all around the panel, which is very visible as the > > right-handside scrollbar is a couple of pixels away from the edge of the > > window, and you can see the dotted line showing more content is available > > isn't at the bottom (or top if you scrolled down) of the window either. > > - the mouse speed and touchpad speed sliders should be expanded, like the > > double-click delay slider in the universal access panel. > > Got it. > > > - I see a mouse speed slider, even though I don't have any mice (I have 2 > > touchpads, one external one, and one in my closed laptop). This isn't a > > regression, but worth filing a separate bug about. > > Will do it. It seems that mouse_is_present in > panels/common/gsd-device-manager is returning True even without a mice. I > can reproduce it here too. I will try to fix that. You can file a bug and handle it later, that's fine. Just make sure to test whether the UI looks fine without any mice. > > - I think there's too much padding on the sides, but Allan can double-check > > that. > > I got the same margin values from the Privacy panel (although the Mouse > panel is wider). OK then. > This screencast includes a way to show the _Test widget_ that could possibly > be done differently. Opinions? That looks fine to me, much better than the popup dialogue.
Created attachment 313936 [details] [review] mouse: port panel to the new design https://wiki.gnome.org/Design/SystemSettings/Mouse
Review of attachment 313936 [details] [review]: I made just a quick look without testing. Overall looks good to me, just: Bastien wanted all sliders to be expanded, but I think they should be in size group to have same width (but maximal possible). Different width looks weird from the screencast... ::: panels/mouse/gnome-mouse-properties.c @@ +117,3 @@ + gtk_widget_show (WID ("two-finger-scrolling-row")); + else if (have_edge_scrolling) + gtk_widget_show (WID ("edge-scrolling-row")); It seems to me you should use gtk_widget_set_visible there, or call somewhere gtk_widget_hide before. Imagine the situation that you unplug touchpad with two finger scroll available and plug another one with edge scroll only, consequently you will see both switches, but you should see only edge scroll switch...
Review of attachment 313936 [details] [review]: I've tested the panel a bit. It seems it works quite good, but: The toggle for primary button doesn't work correctly for me. E.g. If left is active and I click on right using LEFT mouse button, both buttons looks inactive (however gsettings property is changed properly). It works correctly if I click on right using RIGHT mouse button and vice versa. Shouldn't be the speed sliders inverted? Slow on the left and fast on the right as was before? (I miss separate natural scroll setting for mouse/touchpad and I miss possibility to allow edge scrolling if natural scroll is supported, but it is probably what designers wants...)
Created attachment 314224 [details] [review] mouse: tweak the mouse panel * decouple Natural Scrolling for mice and touchpad. Now each kind of device has its own entry; * align the speed sliders in the same size group; * handle better the visibility of scroll-method entries; * make slow-to-fast in the speed sliders be from left-to-right direction.
Review of attachment 314224 [details] [review]: I'd prefer separate fixes for those changes. Don't see any obviously wrong, but you also have a change to move the slider marks, which isn't listed above.
Created attachment 314229 [details] [review] mouse: decouple Natural Scrolling for mice and touchpads Now each kind of device has its own entry.
Created attachment 314230 [details] [review] mouse: decouple Natural Scrolling for mice and touchpads Now each kind of device has its own entry.
Created attachment 314231 [details] [review] mouse: align the speed sliders in the same size group
Created attachment 314232 [details] [review] mouse: make slow-to-fast in the speed sliders go from left-to-right
Created attachment 314233 [details] [review] mouse: handle better the visibility of scroll-method entries Fixes the situation where you unplug a touchpad device which supports two-finger-scrolling and plug in another one with edge-scrolling only, we would have two switches. These entries are mutually exclusive.
Created attachment 314234 [details] [review] mouse: make the scale cursors point to the bottom to match the mockups https://wiki.gnome.org/Design/SystemSettings/Mouse#Tentative_Design
Review of attachment 314230 [details] [review]: Looks fine otherwise. ::: panels/mouse/gnome-mouse-properties.ui @@ +258,3 @@ + <property name="hexpand">True</property> + <property name="xalign">0</property> + <property name="label" translatable="yes" comments="Translators: This switch reverses the scrolling direction for touchpads. The term used comes from OS X so use the same translation if possible. ">Natural Scrolling</property> Please fix the comment, this isn't for touchpads, but for mouse wheels.
Review of attachment 314231 [details] [review]: Looks fine otherwise. ::: panels/mouse/gnome-mouse-properties.c @@ +282,3 @@ +static void +create_dialog (CcMousePropertiesPrivate *d) add_scales_to_sizegroup() instead?
Review of attachment 314232 [details] [review]: Sure.
Review of attachment 314233 [details] [review]: Looks good.
Review of attachment 314234 [details] [review]: Yep.
I've done a quick design review. In general it looks great, most of these points are minor visual tweaks: * Primary button - toggle switches aren't vertically centered inside the row * Primary button - description is rather sucky, although I'm struggling to come up with anything better. "Sets the main button on mice and touchpads."? * The content of some of the rows isn't vertically centered, such as Natural Scrolling and Touchpad. In some cases this seems to be a result of odd packing - the separator below the Touchpad row being contained within its own row, for example. This introduces another visual glitch - the separator doesn't connect with the edges of the list box (the other separators do). * The mouse speed row is much taller than all the other rows. If possible, the rows in the mouse and touchpad sections should all be the same height. If this isn't possible, the rows with sliders should at least be the same height as one another. * The rows with switches seem a bit short to me. * Something's slightly off about the alignment of the headings - I'll provide a patch for that. * I'm not sure about the sliders with the marks - the pointy handle looks a bit much, and it isn't obvious exactly what the mark refers to. Would it be possible to have a sticky point on the slider, but lose the mark and have a round handle? :) * Why are there two Natural Scrolling switches? * Natural scrolling probably needs an explanation as a subtext, but need to answer the question above first.
(In reply to Allan Day from comment #33) > I've done a quick design review. In general it looks great, most of these > points are minor visual tweaks: > > * Primary button - toggle switches aren't vertically centered inside the row > * Primary button - description is rather sucky, although I'm struggling to > come up with anything better. "Sets the main button on mice and touchpads."? It doesn't affect touchpads. It only affects touchpads with physical buttons, and touchpads with "button areas" (like, bottom right and bottom left of the touchpad act as buttons). > * The content of some of the rows isn't vertically centered, such as > Natural Scrolling and Touchpad. In some cases this seems to be a result of > odd packing - the separator below the Touchpad row being contained within > its own row, for example. This introduces another visual glitch - the > separator doesn't connect with the edges of the list box (the other > separators do). > * The mouse speed row is much taller than all the other rows. If possible, > the rows in the mouse and touchpad sections should all be the same height. > If this isn't possible, the rows with sliders should at least be the same > height as one another. Vertical groupsizes! > * The rows with switches seem a bit short to me. > * Something's slightly off about the alignment of the headings - I'll > provide a patch for that. > * I'm not sure about the sliders with the marks - the pointy handle looks a > bit much, and it isn't obvious exactly what the mark refers to. Would it be > possible to have a sticky point on the slider, but lose the mark and have a > round handle? :) Sticky point without a mark is going to feel really weird. The mark is supposed to be the default. Maybe we can do without a default mark... > * Why are there two Natural Scrolling switches? One for mice and their scroll wheels, and touchpads. > * Natural scrolling probably needs an explanation as a subtext, but need to > answer the question above first. (you didn't have one in your mockups ;)
(In reply to Ondrej Holy from comment #19) > Review of attachment 313936 [details] [review] [review]: > > I've tested the panel a bit. It seems it works quite good, but: > > The toggle for primary button doesn't work correctly for me. E.g. If left is > active and I click on right using LEFT mouse button, both buttons looks > inactive (however gsettings property is changed properly). It works > correctly if I click on right using RIGHT mouse button and vice versa. I wonder why you can't reproduce it, however I found what is wrong. You set the button to be active on "button_release_event", consequently the button is toggled (probably by the default signal handler). It can be workarounded with g_signal_connect_after. However I found another bug with the toggles. If you click on active button, it makes both buttons inactive. This is also reproducible using keyboard. Also focus behaves weirdly in some cases. I think that correct fix is usage of GtkRadioButton as we have before, but with draw-indicator = FALSE to look like ordinary buttons. You can inspire in GtkStackSwitcher...
Also the toggles should react on left-handed property changes (i.e. use g_settings_bind).
(In reply to Bastien Nocera from comment #34) ... > > * Primary button - description is rather sucky, although I'm struggling to > > come up with anything better. "Sets the main button on mice and touchpads."? > > It doesn't affect touchpads. It only affects touchpads with physical > buttons, and touchpads with "button areas" (like, bottom right and bottom > left of the touchpad act as buttons). OK, so "Sets the order of physical buttons on mice and touchpads."? ... > > * I'm not sure about the sliders with the marks - the pointy handle looks a > > bit much, and it isn't obvious exactly what the mark refers to. Would it be > > possible to have a sticky point on the slider, but lose the mark and have a > > round handle? :) > > Sticky point without a mark is going to feel really weird. The mark is > supposed to be the default. Maybe we can do without a default mark... I'm not sure it would be weird. But yes, maybe we could do without them. Right now it feels too much. > > * Why are there two Natural Scrolling switches? > > One for mice and their scroll wheels, and touchpads. Sure, but the question is: shouldn't the behaviour for scroll wheels and touchpad scrolling be the same? It seems strange to be able to change the behaivour independently. > > * Natural scrolling probably needs an explanation as a subtext, but need to > > answer the question above first. > > (you didn't have one in your mockups ;) Well, I never claimed that my mockups are always 100% perfect. ;)
Created attachment 314314 [details] [review] header spacing patch Here's the promised patch for the heading padding.
Created attachment 314315 [details] [review] mouse: Fix right-/left-handed buttons getting out of sync Make the two independent buttons into a group of radio buttons without an indicator to make sure they stay in sync (one enabled, one disabled).
Created attachment 314316 [details] [review] mouse: Change UI when mouse handedness when setting changes This simplifies the way that we keep the settings and the UI in sync.
Created attachment 314317 [details] [review] mouse: Use GtkGesture to handle secondary button Instead of hacking the release signal.
Created attachment 314318 [details] [review] mouse: Tweak header spacing There was slightly too much padding between the headings and the list boxes below.
(In reply to Allan Day from comment #37) > (In reply to Bastien Nocera from comment #34) > ... > > > * Primary button - description is rather sucky, although I'm struggling to > > > come up with anything better. "Sets the main button on mice and touchpads."? > > > > It doesn't affect touchpads. It only affects touchpads with physical > > buttons, and touchpads with "button areas" (like, bottom right and bottom > > left of the touchpad act as buttons). > > OK, so "Sets the order of physical buttons on mice and touchpads."? Fixed. > ... > > > * I'm not sure about the sliders with the marks - the pointy handle looks a > > > bit much, and it isn't obvious exactly what the mark refers to. Would it be > > > possible to have a sticky point on the slider, but lose the mark and have a > > > round handle? :) > > > > Sticky point without a mark is going to feel really weird. The mark is > > supposed to be the default. Maybe we can do without a default mark... > > I'm not sure it would be weird. But yes, maybe we could do without them. > Right now it feels too much. Removed. > > > * Why are there two Natural Scrolling switches? > > > > One for mice and their scroll wheels, and touchpads. > > Sure, but the question is: shouldn't the behaviour for scroll wheels and > touchpad scrolling be the same? It seems strange to be able to change the > behaivour independently. It's a very personal choice. We used to have a single setting for both, and people would get really angry at it. If you ever play first person shooters, it's like the vertical axis inversion. Personally, I switch the touchpad but not the trackball (which gets mouse settings applied). We could have a single setting, and have people switch them around when switching from mouse to touchpad or vice-versa, but that would only increase frustration, and we'd need to expose it in gnome-tweak-tool. This is easier. > > > * Natural scrolling probably needs an explanation as a subtext, but need to > > > answer the question above first. > > > > (you didn't have one in your mockups ;) > > Well, I never claimed that my mockups are always 100% perfect. ;) What did you want the description to say here then?
Created attachment 314319 [details] [review] mouse: Fix "Primary button" description
Created attachment 314320 [details] [review] mouse: Remove default value marks on scales They weren't that helpful.
Attachment 314315 [details] pushed as d6970f1 - mouse: Fix right-/left-handed buttons getting out of sync Attachment 314316 [details] pushed as 1d95200 - mouse: Change UI when mouse handedness when setting changes Attachment 314317 [details] pushed as 1a76afe - mouse: Use GtkGesture to handle secondary button Attachment 314318 [details] pushed as f872293 - mouse: Tweak header spacing Attachment 314319 [details] pushed as d6ed423 - mouse: Fix "Primary button" description Attachment 314320 [details] pushed as 3d95415 - mouse: Remove default value marks on scales
Updated issue list: * Primary button - toggle buttons aren't vertically centered inside the row. * Primary button - toggle buttons don't have the standard button height. * Primary button - toggle button labels aren't vertically centered. * Mouse speed and touchpad speed rows are different heights. * Oddities around the touchpad/natural scrolling separator - it doesn't reach the edges of the list box, nor is it flush with the hover highlight. * The rows with switches look a bit short. * I'm still not sure we need two natural scrolling switches. * Natural scrolling probably needs an explanation.
(In reply to Bastien Nocera from comment #46) > Attachment 314315 [details] pushed as d6970f1 - mouse: Fix > right-/left-handed buttons getting out of sync > Attachment 314316 [details] pushed as 1d95200 - mouse: Change UI when mouse > handedness when setting changes > Attachment 314317 [details] pushed as 1a76afe - mouse: Use GtkGesture to Primary button toggle seems to work correctly with your recent patches...
Created attachment 314404 [details] [review] mouse: make all rows the same height
Created attachment 314405 [details] screenshot speed row height I hate to arbitrarily set margin values. :p
*** Bug 735148 has been marked as a duplicate of this bug. ***
Review of attachment 314404 [details] [review]: Looks good.
(In reply to Allan Day from comment #47) > Updated issue list: > > * Primary button - toggle buttons aren't vertically centered inside the row. > * Primary button - toggle buttons don't have the standard button height. > * Primary button - toggle button labels aren't vertically centered. > * Mouse speed and touchpad speed rows are different heights. > * The rows with switches look a bit short. I'm guessing this should be fixed by the above patch. > * Oddities around the touchpad/natural scrolling separator - it doesn't > reach the edges of the list box, nor is it flush with the hover highlight. That's not fixed. > * I'm still not sure we need two natural scrolling switches. One for mouse (wheels), one for touchpad. Because they're different types of pointing devices and that it grates people the wrong way to not have the devices behave how they expect. > * Natural scrolling probably needs an explanation. Does it when we can test the settings? It's also explained in the Help: " You can drag content as if sliding a physical piece of paper using the touchpad. "
That should just leave the label for the "Natural scrolling" sections.
I've added guidance for labels to the mockups, including a label for natural scrolling. In doing so, I've also added two natural scrolling switches - one for mouse and one for touchpad. If we are going to stick with two natural scrolling switches, we need to make sure that natural scrolling is enabled by default - it would suck to have to hunt for two separate switches in order to turn it on.
Created attachment 314742 [details] [review] mouse: add a label description for the 'Natural Scrolling' entries
Created attachment 314746 [details] [review] mouse: make the touchpad options separator non-selectable Since the first entry in the Touchpad section causes the sensitive of the items below to change, those items are in a diferent ListBox. To make it look like it is all the same ListBox, we arbitrarily add a separator between the first item and the items below.
Review of attachment 314742 [details] [review]: The label itself is a bit weird to me, but I can't think of a better one.
Review of attachment 314746 [details] [review]: Sure.
Created attachment 314747 [details] [review] schemas: Enable natural scrolling by default
Created attachment 314839 [details] [review] mouse: primary buttons entry is now vertically centered in the row
Review of attachment 314839 [details] [review]: Sure.
Created attachment 314901 [details] [review] mouse: properly wrap and scale the row description texts In the designs at https://wiki.gnome.org/Design/SystemSettings/Mouse, the description texts of each row is slightly smaller than their labels. Besides, the description of the primary-button row has an arbitrary line break.
Created attachment 314902 [details] [review] mouse: make primary-buttons homogeneous and same size as mockups
Review of attachment 314901 [details] [review]: Looks fine though. ::: panels/mouse/gnome-mouse-properties.ui @@ +109,3 @@ </style> + <attributes> + <attribute name="scale" value="0.90000000000000002"/> Can you clean this up to be "0.9"?
Review of attachment 314902 [details] [review]: Looks fine to me.
It seems that this commit inverts my track point scrolling, using $ rpm -q gsettings-desktop-schemas gsettings-desktop-schemas-3.19.2-1.fc24.x86_64 I must say this is crazy setting and my track point scrolling become unusable :(( the option in $ rpm -q control-center control-center-3.18.2-1.fc24.x86_64 has no influence on anything with regards of track point :(( Please revert the change. [1] https://github.com/GNOME/gsettings-desktop-schemas/commit/6f7b78c1b6bc38a8679a3b51ec30c90bd05de9e7