Created attachment 100931 [details] [review]
Patch 01

Created attachment 100932 [details] [review]
Patch 02

Created attachment 100933 [details] [review]
Patch 03

Created attachment 100934 [details]
gnome-mouse-accessibility.c

Created attachment 100935 [details]
gnome-mouse-accessibility.h

Apart from a few minor style issues (sometimes no space before '(', some unnecessary casts) that looks quite good at first glance. If the daemon needs to stay separate, that's unfortunate, but so be it.

For gnome-mouse-accessibility.c you should look at GConfPropertyEditor (capplets/common/gconf-property-editor.[ch]). It will allow you to scrap all the code that simply watches for widget changes to update GConf.

(btw, it's a lot more practical to have everything as a single patch. diff -urNp is your friend (when using svn diff, that one doesn't support -p, unfortunately, but -ur is automatic, and -N should be, too, if you first svn add the new files)).

Created attachment 100995 [details] [review]
01_gnome-cc_add-mousetweaks.diff

Here's a all-in-one patch. This one should fix the style issues you mentioned and it also uses the peditor interface.

Created attachment 101003 [details] [review]
Complete mousetweaks integration patch

I completed the patch by adding the needed glade changes and adding some some fine-tuning and enhancements to gnome-mouse-accessibilty.c (e.g. population of the gesture combo boxes).

Note that this patch also contains the rest of my mockup, that is, Buttons and Motion in a single tab. For reference, here's the mockup URL:

http://ultimum-projekt.de/mockups/mouse.html

(@Francesco: I changed the text for the dwell click checkbox to "Initiate click when stopping pointer movement" to make clear that stopping pointer movement doesn't have to mean a click with mouse gestures - it may be "initiated", but not completed in that case.)

@Denis

The "Initiate..." is more accurate for the click with gesture, but might pose a problem for the click with the click type window: The user might ask how to complete the click. (It solves one inaccuracy, but creates a new one.) 

As the text of the dwell checkbox from the former version (http://ultimum-projekt.de/mockups/mouse.html) was at least clear about the function, I wonder whether that version was not better. 

What about this third alternative: 
- dwell checkbox: Click by software (or Click without using any hardware button) 
- radio button 1: Perform selected click automatically after timeout (I suppose that the user will be able to deduce that he has to select the click in the click type window) 
- radio button 2: Perform click on user's movement after timeout (More accurate but longer: "Perform click on user's movement during a short delay after timeout." Again, I suppose that the user will understand the relation to the 4 popups.) 
- replace Delay with Timeout and move both sliders to the bottom, where at least "Timeout" has already been introduced by the radio buttons.
- what about replacing "Motion Treshold" with "Ignore Movements" (Small/Bigger); would it not be clearer? 


There is also an inaccuracy in "Simulated Secondary Button": In fact, this mousetweaks function can only simulate the secondary click; it cannot simulate for example a secondary click and hold. The word "Button" should perhaps be replaced with "Click" in the title of the function. 

Gerd's patch looks good to me (there's still one instance of an unnecessary G_OBJECT cast fwiw). One question I still have (since I can't check myself atm): Does it work correctly when the mousetweaks schemas are not installed and the respective GConf hierarchy does not yet exist? If it does, thumbs up from me.

(In reply to comment #9)
> What about this third alternative: 
> - dwell checkbox: Click by software (or Click without using any hardware
> button) 

I find the term "click by software" exceptionally bad. It doesn't tell you anything about how you actually do it, and the "software" part is an implementation detail anyway. Add to that that "hardware clicks" don't really exist at all...

> - replace Delay with Timeout and move both sliders to the bottom, where at
> least "Timeout" has already been introduced by the radio buttons.

Actually, someone from the usability team (I think it was Calum) commented that we should try to get rid of the term "timeout" wherever possible since it's a lot more technical and non-obvious than e.g. "delay".

@Jens

What do you think about "Click without using any hardware button"? There are no hardware clicks, but usually users perform the clicks by using hardware buttons. 

The fact that the checkbox does not tell how to do the clicks is solved by the following radio buttons where the user chooses how he wants the clicks to occur: automatically after the delay or on a users gestures after the delay. 

In fact, this alternative tried to solve the problem that exist when the checkbox already tries to indicate how to perform the clicks under dwelling. As there are two different ways, it will be difficult to catch both. 

I am however aware that my proposition tends to turn the checkbox into a simple de-/activation element. By replacing the radio buttons with checkboxes, we could even get rid of that main dwell activating checkbox; but that convinces me even less, as the two ways to perform the dwell click would seem to be independent one from another. 

Please, take my suggestions for what they are: propositions that try to help. 

Finally, thanks for pointing me to the issue with the term "timeout".

Created attachment 101110 [details] [review]
02_gnome-cc_add-mousetweaks.diff

Removed the unnecessary cast

I've just tested the patches and missing schemas don't cause any problems. But I noticed that some upper and lower boundaries are wrong. Delay values should be between 0.5 - 3.0 (default: 1.2) and motion threshold between 0 - 30 (default: 10).

Also the combo box entries are missing. The "Complete .." patch doesn't include gnome-mouse-accessibility.[ch]. Is there a reason why the entries shouldn't be directly in the glade file? 

I have enhanced config-mouse.xml to include information about the new accessibility features. I based my description on the following mockup: 
http://ultimum-projekt.de/mockups/mouse.html

Instead of attaching a diff to this thread, I will post the complete config-mouse.xml file because it might be simpler to review it. (Be aware that I did not change anything on the already existing description; I only added new information.) 

Of course, once the gui is definitely chosen, I can revise it. 

Should I already send a diff to Gerd so that he can include it to the already combined patch? 

By the way, when I click on the help button in the capplet that comes with my Ubuntu 7.10 distribution, I get a help file that is different from the one that I have enhanced. Could anybody please tell me what file that is, as it will probably also have to be enhanced with the information about the new mouse accessibility features. 

Created attachment 101142 [details]
Mouse capplet help file with info about the a11y features

(In reply to comment #14)
> Instead of attaching a diff to this thread, I will post the complete
> config-mouse.xml file because it might be simpler to review it. (Be aware that
> I did not change anything on the already existing description; I only added new
> information.) 

Thanks a lot. A lot of what's already in there isn't accurate anymore, so we'll basically have to go over everything anyway.
 
> Should I already send a diff to Gerd so that he can include it to the already
> combined patch? 

Doesn't really matter much. We can keep the docs separate if that's easier to work with.

> By the way, when I click on the help button in the capplet that comes with my
> Ubuntu 7.10 distribution, I get a help file that is different from the one that
> I have enhanced. Could anybody please tell me what file that is, as it will
> probably also have to be enhanced with the information about the new mouse
> accessibility features. 

No idea what Ubuntu does.


One additional comment on the code:

> +set_mousetweaks_daemon (gboolean dwell_enabled, gboolean delay_enabled)
> +{
> +	gchar *comm;
> +
> +	comm = g_strdup_printf ("mousetweaks %s",
> +				(dwell_enabled || delay_enabled) ? "" : "-s");
> +	g_spawn_command_line_async (comm, NULL);

Especially if m-t becomes a separate package, I guess we should check for the result of the spawn here and popup an error if starting the daemon fails. Otherwise people will be filing "doesn't work" bugs all over the place.

One point I'm not so sure about is whether we want to keep the /apps/mousetweaks GConf context or switch it to something below /desktop/gnome when the module if officially accepted.

(In reply to comment #16)
> One additional comment on the code:
> 
> > +set_mousetweaks_daemon (gboolean dwell_enabled, gboolean delay_enabled)
> > +{
> > +	gchar *comm;
> > +
> > +	comm = g_strdup_printf ("mousetweaks %s",
> > +				(dwell_enabled || delay_enabled) ? "" : "-s");
> > +	g_spawn_command_line_async (comm, NULL);
> 
> Especially if m-t becomes a separate package, I guess we should check for the
> result of the spawn here and popup an error if starting the daemon fails.
> Otherwise people will be filing "doesn't work" bugs all over the place.
> 

How about:

if (!g_spawn_command_line_async (comm, &error)) {
	if (error->code == G_SPAWN_ERROR_NOENT) {
		dialog = gtk_message_dialog_new (NULL, 0,
						 GTK_MESSAGE_WARNING,
						 GTK_BUTTONS_OK,
							 _("Couldn't find mousetweaks"));
		gtk_message_dialog_format_secondary_text (GTK_MESSAGE_DIALOG (dialog),
							  _("This feature requires mousetweaks to "
							  "be installed on your system."));
		gtk_window_set_title (GTK_WINDOW (dialog), _("Mouse Preferences"));
		gtk_window_set_icon_name (GTK_WINDOW (dialog), "gnome-dev-mouse-optical");
		gtk_dialog_run (GTK_DIALOG (dialog));
		gtk_widget_destroy (dialog);
	}
	g_error_free (error);
}

This will show the dialog only for 'file not found' errors, to avoid masking other spawn errors. I'm not sure about the message content though.


> One point I'm not so sure about is whether we want to keep the
> /apps/mousetweaks GConf context or switch it to something below /desktop/gnome
> when the module if officially accepted.
> 

Yes, that's actually why I used the MT_GCONF_HOME macro. We could put the keys in /desktop/gnome/accessibility/mouse similar to the AccessX (keyboard) keys or maybe directly under /desktop/gnome/peripherals/mouse/accessibility.

(In reply to comment #17)
> if (!g_spawn_command_line_async (comm, &error)) {
>         if (error->code == G_SPAWN_ERROR_NOENT) {

> This will show the dialog only for 'file not found' errors, to avoid masking
> other spawn errors. I'm not sure about the message content though.

I think we only need to show the message if starting the daemon fails, not when stopping it (that'll quickly get annoying). How about "Could not enable mouse accessibilty features." as primary and "Mouse accessibility requires the mousetweaks daemon to be installed on your system." as secondary text?

Reply to comment 16:

>> By the way, when I click on the help button in the capplet that comes with my
>> Ubuntu 7.10 distribution, I get a help file that is different from the one that
>> I have enhanced. Could anybody please tell me what file that is, as it will
>> probably also have to be enhanced with the information about the new mouse
>> accessibility features. 

> No idea what Ubuntu does.

I was able to track down what happens on my Ubuntu installation: 

The help button on the Mouse capplet does not open the config-mouse.xml file; it opens /usr/share/gnome/help/user-guide/C/user-guide.xml at the Mouse Preferences paragraph, which is located under the chapter "Configuring Your Desktop-Hardware".

However the documentation of the Mouse capplet is not in the user-guide.xml file itself; it is in the file goscustdesk.xml that is located in the same directory as user-guide.xml and that is loaded by the user-guide.xml file.

Now I wonder whether on a standard gnome installation /usr/share/gnome/help/control-center/C/config-mouse.xml gets used at all. 

Created attachment 101330 [details] [review]
03_gnome-cc_add-mousetweaks.diff

This patch adds the g-s-d dialog and moves the gconf keys to /desktop/gnome/accessibility/mouse. I've also removed some unnecessary code from gnome-mouse-accessibility.c and changed every 'right' click to 'secondary'. This changes one widget name (dwell_gest_right -> dwell_gest_secondary).

Created attachment 101948 [details] [review]
Update complete mousetweaks patch

This bug really merges the last patch and the needed glade changes (forgot to "svn add" gnome-mouse-accessibility.[ch] last time, I know... ;)).

@Gerd: I updated the upper and lower bounds of the delay and motion threshold sliders according to your suggestions (though I found the ones I used in the mousetweaks schema file on launchpad; maybe you didn't update it?).

@Francesco: Thanks for explanations on the "Simulated Secondary Button" label, I changed that to "Simulated Secondary Click". I left the the dwell click label unchanged for now, though; while the "initiate" element indeed leaves kind of an ambiguity for click type window based dwelling, I think a user can quickly find out that initiating and completing is the same in that case, latest when he tries out the feature. And for the those who fear to just try it, there is still the help documents.

P.S.: Happy new year to all of you! :)

@Denis

Is there an image from the last gnome-mouse-accessibility layout that you have prepared anywhere online?

@all

Should I prepare a patch for goscustdesk.xml based on that layout? 

Happy new year. 

i'd also love to see a screenshot how it integrates into the existing control-center UI.

Here are screenshots of the mouse capplet with the patch. Note that I merged the former two tabs to one to keep the number of tabs small (those two had very few settings anyway).

http://ultimum-projekt.de/mockups/mouse.html

Created attachment 102241 [details]
Documentation of the mouse capplet for the gnome control center

I have rewritten the config-mouse.xml file for the /usr/share/gnome/help/control-center/C/ directory so that it corresponds to the layout indicated by Denis message just above this one. 
(I am not sure about the explanation that I give for the mouse acceleration and sensitivity setting.) 

Created attachment 102520 [details] [review]
Patch for new gsd

This is a similar g-s-d patch but for the new standalone module.

2008-01-14  Denis Washington  <denisw@svn.gnome.org>

	Integrate mousetweaks settings into the mouse capplet. (Bug #503547)

	* capplets/mouse/gnome-mouse-properties.glade:
	Merge the previous "Buttons" and "Motion" tabs into one, and add a new
	"Accessibility" tab with the mousetweaks preferences. Additionally, re-add
	the "Locate Pointer" preference which disappeared in 2.20. (Bug #480457)

	* capplets/mouse/gnome-mouse-properties.c:
	Call setup function for the a11y tab, update for a small UI change regarding
	handness preferences, and implement the locate-pointer checkbox.

	* capplets/mouse/gnome-mouse-accessibility.[ch]:
	Added.

	* capplets/mouse/Makefile.am:
	Add gnome-mouse-accessibility.c.
	
	* gnome-settings-daemon/gnome-settings-mouse.c:
	Mousetweaks support.


2008-01-14  Denis Washington  <denisw@svn.gnome.org>

	* help/C/config-mouse.xml:
	Updated to reflect the changes to the mouse capplet, e.g. the new
	Accessibility tab.

Closing then