GNOME Bugzilla – Bug 622414
Add preferences dialog for the magnifier
Last modified: 2013-12-04 18:03:05 UTC
See patch. Working fine, but we need to get in touch with a11y people to be sure they don't plan to access the GConf settings of the magnifier from somewhere else - I bet OTC that it will help them, but better check. One stylistic issue is that I'm not sure the way closures are indented is right, since it make really wide lines. OTOH breaking the line before Lang.bind may look slightly weird. This is something we'd need to add the the style guide!
Created attachment 164315 [details] [review] Port magnifier to GSettings The Shell is the only user of the magnifier, so there's no reason to keep using GConf for its settings. Since it's a session-wide tool, use a distinct schema, org.gnome.accessibility.magnifier, stored in /desktop/gnome/accessibility/magnifier just like before. Put these settings in a separate schema file for clarity. Old enums in GConf were stored as integers, we now use the facilities provided by GSettings to save them as strings, and convert them to integers automatically thanks to the mapping stored in the schemas.
(In reply to comment #1) > Created an attachment (id=164315) [details] [review] > Port magnifier to GSettings A couple of initial thoughts. > The Shell is the only user of the magnifier, Strictly speaking, the other major user of the magnifier is Orca. It is currently accessing the magnifier mostly via Dbus, but one of the settings (mouse tracking) is handled via GConf. And, there is a discussion as to which aspects of the magnifier is better handled by GConf/GSettings and which by DBus. However, Orca is switching to GSettings as well (isn't everyone?), so "there's not reason to keep using GConf" -- agreed. > Old enums in GConf were stored as integers, we now use the > facilities provided by GSettings to save them as strings, > and convert them to integers automatically thanks to the > mapping stored in the schemas. Cool. (I prefer strings).
Thanks for the feedback! Do you know if there's a schedule for Orca to move to GSettings? I've found the bug report, but it's not clear how much time it is supposed to take at all... Anyway in the short term, patch can wait in Bugzilla, since it doesn't change many files in the tree, it will be easy to update later.
Created attachment 164745 [details] [review] Port magnifier to GSettings Rebased version against master.
(In reply to comment #3) > Thanks for the feedback! Do you know if there's a schedule for Orca to move to > GSettings? I've found the bug report, but it's not clear how much time it is > supposed to take at all... > > Anyway in the short term, patch can wait in Bugzilla, since it doesn't change > many files in the tree, it will be easy to update later. Hi Milan, As expected we are moving orca's settings to GConf/GSettings as you noticed bug #619398. We're developing a hybrid approach. Why? Because Orca never manage settings via GConf or GSettings and we want to allow compatibility between different gnome versions. Also, move from GConf to GSettings is extremely easy. It's not so easy to move from current orca's settings to any GNOME setting system. We define an architecture to support both, probably in the future we drop GConf but meanwhile we're developing various settings flavours. Schedule, I expect that at the end of July we'll finished both backends. You can check current work in this branch http://git.gnome.org/browse/orca/log/?h=new-settings if you want. Cheers.
so... should this patch be delayed until orca is updated? or what?
(In reply to comment #6) > so... should this patch be delayed until orca is updated? or what? Well, I have mixed feelings about this. But from a practical standpoint: 1. Any Orca user who requires more than magnification in order to access his/her Desktop can't use GNOME Shell at the moment. 2. By the time that issue is resolved -- and actually much sooner -- Orca will be updated to use GSettings. So given that the current number of impacted users (Orca + GNOME Shell) is zero, I am fine with getting the patch in now rather than expecting you to wait. Thanks for asking!
So, OK to commit the patch based on Joanmarie's comment?
(In reply to comment #8) > So, OK to commit the patch based on Joanmarie's comment? I have but a minor practical objection, and it's simply that I need to update this stand-alone preferences dialog to use GSettings instead of GConf (http://live.gnome.org/GnomeShell/Magnification#Magnifier_Preferences_Dialog). Since I know little about GSettings, I'll be asking for guidance :-). But, I have no principled objection as long as applying the patch does not block accessing the settings from an out-of-process app like the preferences dialog or Orca.
Created attachment 166910 [details] [review] Port magnifierPrefs tool to GSettings Here's a rough patch to port the tool to GSettings. I've only made systematic substitutions based on what I know is needed, but I can't test the result since it requires Python bindings to be aware of additions in the unstable version. And I've never really coded in Python, so I don't know how to do this. Could you test the patch for me?
Created attachment 166911 [details] [review] Port magnifierPrefs tool to GSettings Hm, the actual patch instead of the source file, plus some fixes.
(In reply to comment #11) > Created an attachment (id=166911) [details] [review] > Port magnifierPrefs tool to GSettings > > Hm, the actual patch instead of the source file, plus some fixes. Thanks for this Milan! I have applied the patch and run it only to get a message to the effect that the "Settings" module for Gio can't be found. Upon investigation, it looks like the Python bindings for Gio are not really there yet -- see this comment in Orca's bugzilla: https://bugzilla.gnome.org/show_bug.cgi?id=619398#c2 I've discovered that the JavaScript bindings for Gio/GSettings are in much better shape. Based on the python code the preferences dilaog, and using your patch, I have created a JavaScript version, which is described here: http://live.gnome.org/GnomeShell/Magnification/GSettings The main issues, and where I need help, are: * the location of the GSettings templates. * the location of the users' preferences. * access to the magnifier's DBus service from a JavaScript client. The last one (DBus) is not a GSettings issue; I mention it for completeness. Can you clarify what I should do so that this Dialog will find the templates, and where the user's preferences are actually stored?
Nice to get a JS version of the tool, this will be more consistent with the rest of the Shell! I've checked your code, and it works well here as long as you apply the patch to the Shell and install it: we need the schemas to be installed before the tool tries to access them. A remark about your code: You should not hard code default values like DEFAULT_CROSSHAIRS_COLOR and DEFAULT_MAG FACTOR. These are stored in the schemas, and there's no need to duplicate them. So when checking the stored value for nullity, you should call g_settings_reset() and get the value again, to use the default (you could also use g_settings_get_mapped(), but that would be a little more complex than needed). DEFAULT_CROSSHAIRS_LENGTH and DEFAULT_CROSSHAIRS_OPACITY are not even used in the code. Note that for DEFAULT_CROSSHAIRS_LENGTH, you only check for == 0.0, but you're only able to show values from 1 to 10, which could theoretically lead to issues. About DBus and JS, I really can't help you, but have a look at js/ui/magnifierDBus.js - or ask on IRC, where others can help you.
Created attachment 167364 [details] [review] Port magnifier to GSettings New patch rebased against master, with additional cleaning: I removed all DEFAULT_* constants that were used to set values on objects creation. They are completely useless since we always override them before showing the objects, using the values from GSettings. Two values that are not defaults but internal constants are renamed for clarity. So I think it's ready for review and commit.
Review of attachment 167364 [details] [review]: Looks good! Some minor style nitpicks, otherwise good to commit. ::: js/ui/magnifier.js @@ +1,3 @@ /* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */ +const Gio = imports.gi.Gio; Nitpick: this should come after the clutter import, so g-i imports are ordered alphabetically (and yes, this was wrong before) @@ +476,3 @@ + this._settings.connect('changed::' + LENS_MODE_KEY, Lang.bind(this, this._updateLensMode)); + this._settings.connect('changed::' + CLAMP_MODE_KEY, Lang.bind(this, this._updateClampMode)); + this._settings.connect('changed::' + MOUSE_TRACKING_KEY, Lang.bind(this, this._updateMouseTrackingMode)); Those could really use some line breaks (too long for splinter == reviewing PITA) @@ +505,3 @@ + }, + + _updateScreenPosition: function(settings) { You could remove the parameter and use this._settings inside the function (and likewise below). Feel free to ignore this comment.
Pushed with suggested changes as e88f080. Thanks for the review!
(In reply to comment #12) > I've discovered that the JavaScript bindings for Gio/GSettings are in much > better shape. Based on the python code the preferences dilaog, and using your > patch, I have created a JavaScript version, which is described here: > http://live.gnome.org/GnomeShell/Magnification/GSettings Quoting from there: "To deal with the templates, for the present, the launcher script (launchPrefs.sh) assumes that GnomeShell is installed, and that the templates are stored in the standard place, namely $HOME/gnome-shell/install/share/glib-2.0/schemas/. It exports that path as a environment variable before starting JavaScript." This is of course wrong, but it's tricky to handle this from outside the gnome-shell build environment. So the first question which comes to mind: where is that code supposed to end up? Does it make sense to add this dialog to the shell itself (similar to gnome-shell-clock-preferences)? I did only take a quick peek into the tarball, so no real review here, just some observations - launchPrefs.sh is a poor name, as it is the executable which users are dealing with; something like gnome-magnifier-preferences would be more descriptive (and also less likely to create conflicts) - the glade suffix is used for UI files in the libglade format - it would be better to use the ui suffix for GtkBuilder files > The main issues, and where I need help, are: > * the location of the GSettings templates. Tricky. gsettings.m4, which is distributed with GIO and contains macros for autoconf integration, defines gsettingsschemadir to ${datadir}/glib-2.0/schemas - not really helpful in this case as the problem is the uncommon datadir used by gnome-shell (e.g. neither /usr/local nor /usr). You could add some configure script which runs "pkg-config --variable prefix glib-2.0" to find the correct prefix (datadir == ${prefix}/share) and tell people to make sure that glib from the gnome-shell moduleset is used. Of course, the easiest way would be to include the magnifier preferences in the gnome-shell sources ... > * the location of the users' preferences. It depends on the backend used by gsettings - if dconf is used, it will be in $HOME/.config/dconf; if gconf is used, it will be somewhere in $HOME/.gconf; if the fallback backend is used, the preferences will only exist in RAM during the execution of the program ... So the short answer is: you shouldn't care, just use the abstraction provided by GSettings. The problem is of course that you will want to use the same backend as gnome-shell (I'd say long-term everyone is expected to use dconf anyway, but right now it's still a little rough)
(In reply to comment #17) > This is of course wrong, but it's tricky to handle this from outside the > gnome-shell build environment. So the first question which comes to mind: where > is that code supposed to end up? Does it make sense to add this dialog to the > shell itself (similar to gnome-shell-clock-preferences)? That's what I thought too when quickly porting the code to GSettings: it's hard, if not impossible, to behave correctly from outside the Shell's package. Is there any reason this tool shouldn't be shipped with the Shell? Where would it go else? How does it relate to other accessibility configuration tools?
(In reply to comment #17) Thanks Florian. This is very helpful. > ... > So the first question which comes to mind: where > is that code supposed to end up? Does it make sense to add this dialog to the > shell itself (similar to gnome-shell-clock-preferences)? In my opinion, these preferences should be accessed through the "Personal/Assistive Technologies" control center preferences, because a user's magnifier preferences shouldn't depend on the thing that implements the magnifier. That control panel is definitely outside of GnomeShell. However, that's also a long term goal, since the current Assistive Technologies control panel doesn't provide access to any magnifier preferences, and it's still under debate which ones it should (see: http://live.gnome.org/Accessibility/PreferencesRework). I'm not sure at what point one puts the templates in their proper place for this. As soon as possible? Or, when things have firmed up, more? In a similar vein, I've modified some of Orca's magnifier preferences handling to use GConf to communicate changes to gs-mag. Again, Orca is outside of GnomeShell. Orca doesn't do magnification itself. Instead it calls upon some other magnifier to do the heavy lifting. Currently, if GnomeShell is running, it will use gs-mag. But, in the long run, one doesn't want Orca snooping to see whether GnomeShell is running. Orca should interact with the user's persistent magnifier preferences, and, whichever magnifier is running should respond appropriately. As for putting the magnifier preferences dialog inside gnome-shell, I have no objection to that. Ironic: The first version of the dialog was actually part of the magnifier patch -- it was inside the shell. However, both Will Walker and Owen suggested that it be moved to an out-of-process app for persistent preferences. See https://bugzilla.gnome.org/show_bug.cgi?id=595507#c14 for the discussion. > - launchPrefs.sh is a poor name, ... > > - the glade suffix is used for UI files in the libglade format - it would > be better to use the ui suffix for GtkBuilder files Points well taken. I will make those changes. (As well as the one's Milan suggested above re: hard coding default values like DEFAULT_CROSSHAIRS_COLOR) > > * the location of the users' preferences. > ... > So the short answer is: you shouldn't care, just use the abstraction provided > by GSettings. D'oh! Of course. As long as everyone (e.g., Control center preferences, Orca, gnome-shell, and anyone else) accesses the same persistent store.
(In reply to comment #19) > In my opinion, these preferences should be accessed through the > "Personal/Assistive Technologies" control center preferences, because a user's > magnifier preferences shouldn't depend on the thing that implements the > magnifier. [...] > > [...] But, in the long run, one doesn't want Orca snooping to > see whether GnomeShell is running. Orca should interact with the user's > persistent magnifier preferences, and, whichever magnifier is running should > respond appropriately. OK, this makes sense. It's probably a good idea to move the settings schema from gnome-shell into gnome-control-center when the control-center panel falls into place though. > As for putting the magnifier preferences dialog inside gnome-shell, I have no > objection to that. Ironic: The first version of the dialog was actually part > of the magnifier patch -- it was inside the shell. However, both Will Walker > and Owen suggested that it be moved to an out-of-process app for persistent > preferences. See https://bugzilla.gnome.org/show_bug.cgi?id=595507#c14 for the > discussion. Oh, I was not suggesting to put the preferences inside the gnome-shell process, I was just talking about the source code (see gnome-shell-clock-preferences, which has its source in the gnome-shell tree but is running in a separate process). Anyway, your argumentation above makes more sense anyway. > I'm not sure at what point one puts the templates in their proper place for > this. As soon as possible? Or, when things have firmed up, more? If there is a definitive answer to that question it's one I'm unaware of :-) So I think we agree that long-term we want the preferences in control center (and take the schema with it). The questions now are how long it will take to get them there and whether we want a temporary solution in the meantime (e.g. your preference dialog). If you think the temporary solution would be helpful (e.g. to help exposing the magnifier feature earlier), it's easier to add it to the shell sources.
http://live.gnome.org/Design/SystemSettings/UniversalAccess has some of the work that I did after discussions with Willie Walker. My understanding is that Thomas Wood took that as a starting point and implemented something in control center 3. I haven't actually seen what he's done yet. The basic idea is that the primary interface for magnification should be global hotkeys or gestures. This is because if you can't see the screen you can't really expect the user to navigate to, find, and change the seeing of the system before they can see it. What is this stuff about crosshairs? Why don't we just use the cursor? In any case, I don't expect to see most or any of the stuff in http://live.gnome.org/GnomeShell/Magnification#Magnifier_Preferences_Dialog to be in the Universal Access panel.
(In reply to comment #20) > ... The questions now are how long it will take to > get them there and whether we want a temporary solution in the meantime (e.g. > your preference dialog). If you think the temporary solution would be helpful > (e.g. to help exposing the magnifier feature earlier), it's easier to add it to > the shell sources. I see. Okay, I'll take a look at how the source tree is organized for gnome-shell-clock-preferences and see what can be done for a comparable gnome-shell-magnifier preferences. I should open another bug for that, too.
(In reply to comment #21) > What is this stuff about crosshairs? Why don't we just use the cursor? We do use the cursor. The use of crosshairs is a common magnifier technique for enhancing the location of the cursor. > > In any case, I don't expect to see most or any of the stuff in > http://live.gnome.org/GnomeShell/Magnification#Magnifier_Preferences_Dialog to > be in the Universal Access panel. Why not? To put a finer point on it: why would there be separate magnifier preferences stored for gs-mag, Orca, and the emerging gnome-mag DBus service if they all represent the same user preferences?
(In reply to comment #22) > I see. Okay, I'll take a look at how the source tree is organized for > gnome-shell-clock-preferences and see what can be done for a comparable > gnome-shell-magnifier preferences. Basically, the dialog's ui is in data/, the code lives in js/prefs/ and the wrapper script to call the javascript is located in src/. > I should open another bug for that, too. OK, though we could reopen/rename this bug to not loose the discussion here.
(In reply to comment #24) > OK, though we could reopen/rename this bug to not loose the discussion here. OK, but I don't appear to have the permissions to do that.
We should have opened a new report before, but now that the comments are here, let's go on...
Created attachment 170513 [details] [review] Add preferences dialog for the magnifier. Added a preferences dialog for configuring the magnifier.
Review of attachment 170513 [details] [review]: The dialog's UI definitively could use some HIG-love, but as it's a temporary solution anyway, it shouldn't matter that much. ::: data/Makefile.am @@ +1,3 @@ desktopdir=$(datadir)/applications +desktop_DATA = gnome-shell.desktop gnome-shell-clock-preferences.desktop \ + gnome-shell-magnifier-preferences.desktop When going multi-line, each filename should appear on its own line: desktop_DATA = \ gnome-shell.desktop \ gnome-shell-clock-preferences.desktop \ gnome-shell-magnifier-preferences.desktop @@ +19,3 @@ dist_images_DATA = \ close-black.svg \ + magnifier.svg \ The backslash should be aligned with tabs ::: js/prefs/magnifierPreferences.js @@ +24,3 @@ +const GLib = imports.gi.GLib; +const Gtk = imports.gi.Gtk; +const Gdk = imports.gi.Gdk; The list of gi imports should be sorted alphabetically ... @@ +28,3 @@ +const Signals = imports.signals; + +const Gettext = imports.gettext; ... and be separated from "normal" imports by a blank line. The blank line above Gettext is wrong (it's wrong in clock-preferences, I know) @@ +32,3 @@ +const ScreenPosition = + { NONE: 0, FULL_SCREEN: 1, TOP_HALF: 2, BOTTOM_HALF: 3, LEFT_HALF: 4, RIGHT_HALF: 5, DOCKED: 6 }; +const MouseTrackingMode = { NONE: 0, CENTERED: 1, PUSH: 2, PROPORTIONAL: 3 }; Someone told me recently that it would be a good idea to sync those with Orca :-) @@ +49,3 @@ + +// Actual opacity values in the range [0,255] +const CROSSHAIRS_OPACITY_RANGE = 255; Nitpicking, but '255' is not a range - CROSSHAIRS_MAX_OPACITY? @@ +62,3 @@ + _init: function(uiFile) { + + let that = this; I admit that I first thought this was a joke :-) What you want is Lang.bind() ... (I'll add an example below) @@ +66,3 @@ + // Given a string of the form '#rrggbb', turn it into a Gdk.Color. + // Gdk.Color components are of the form rrrrggggbbbb. + // Note: Gdk.color_parse() should do this, but I can't get it to work. Grrr ... looks alright as a workaround right now, but we'd obviously want Gdk.color_parse() fixed ... @@ +92,3 @@ + // Initialize Dbus connection to the magnifier. + // TODO (JS): this is how it's done in Python, but doesn't work in + // JavaScript Uhm - can't you use the magnifierDBus methods here? @@ +98,3 @@ + this._magProxy = dBus.get_object('org.gnome.Magnifier', '/org/gnome/Magnifier'); + } + catch (e) { Style error - should be: } catch(e) { @@ +106,3 @@ + // Initialize GSettings + let gSettings = new Gio.Settings({ schema: 'org.gnome.accessibility.magnifier' }); + this._gSettings = gSettings; this._settings appears to be the canonical name used elsewhere. What is the point of 'let a = new X(); this._a = a;' instead of 'this._a = new X();'? @@ +123,3 @@ + else { + showToggle.set_label("Show Magnifier"); + } We don't use brackets if both branches are one liners, e.g. if (x) doX(); else doY(); If brackets are used, the if's closing bracket is on the same line as the else block. @@ +125,3 @@ + } + showToggle.connect('clicked', function() { + that.doShowHide(showToggle) No, the correct way of doing it is: showToggle.connect('clicked', Lang.bind(this, this.doShowHide)); @@ +248,3 @@ + + // Update label + // TODO - JS: should make this label internationalizable. Add it to po/POTFILES.in then ... @@ +257,3 @@ + doMouseTracking: function(combobox) { + let currentVal = combobox.get_active(); + log("current mouse tracking combobox value is: '" + currentVal + "'"); Current convention is to use double quotes (") for translatable string and single quotes (') otherwise. Parameters to log() should not be localized. @@ +353,3 @@ + +function main(params) { + if (params.progName) I prefer params['progName'] to stress that params is used as a hash rather than an object.
I don't want to see a temporary settings dialog. I think we should fix the magnifier defaults as in #629884. Fix the proportional mode as in #629950. Add hotkeys to control the magnification. And help thos get the Universal Access settings finished.
Review of attachment 170513 [details] [review]: Thanks Florian! I'll make the suggested changes. ::: js/prefs/magnifierPreferences.js @@ +32,3 @@ +const ScreenPosition = + { NONE: 0, FULL_SCREEN: 1, TOP_HALF: 2, BOTTOM_HALF: 3, LEFT_HALF: 4, RIGHT_HALF: 5, DOCKED: 6 }; +const MouseTrackingMode = { NONE: 0, CENTERED: 1, PUSH: 2, PROPORTIONAL: 3 }; :-) Like ships, they were patches passing in the night. I'll match what is going to happen in 629884. @@ +62,3 @@ + _init: function(uiFile) { + + let that = this; It's a "Crockford-ism". @@ +92,3 @@ + // Initialize Dbus connection to the magnifier. + // TODO (JS): this is how it's done in Python, but doesn't work in + // JavaScript No, or at least not as far as I can tell. magnifierDBus is a declaration and implementation of the *service* side of the equation (I wrote magnifierDbus.js). The preferences dialog is a *client* that needs to make a connection with DBus, specifically the magnifier DBus service. I know how to do that in python, but not from a javascript client. I've yet to find any documentation on the DBus client javascript bindings. @@ +125,3 @@ + } + showToggle.connect('clicked', function() { + that.doShowHide(showToggle) IMO, 'that.doShowHide(showToggle)' is succinct and transparent. However, I'll modify to follow the GNOME style. @@ +248,3 @@ + + // Update label + // TODO - JS: should make this label internationalizable. Thanks for the hint. I'll look into that. One concern here is that the label changes depending on state, and I'm not sure how to do that in the potfile.
Created attachment 173881 [details] [review] Added a preferences dialog for testing magnifier configurations.
Created attachment 173882 [details] [review] Add preferences dialog for the magnifier Added a preferences dialog for testing magnifier configurations.
Created attachment 174104 [details] [review] Added a preferences dialog for testing magnifier configurations. Added a preferences dialog for testing magnifier configurations. The actual patch I meant to attach with the proper alignment of back slashes in Makefile.am, and, in magnifierPreferences.js, using the array operator for the params argument to main() instead of dotted notation.
Review of attachment 174104 [details] [review]: The code of the dialog itself looks mostly good, except for some comments. The Makefile changes will need cleanup though, and I'm unsure whether it makes sense at all to have an icon/desktop file without installing it. You should also add gnome-shell-magnifier-preferences to .gitignore (and its desktop file as well, if you decide to keep it). ::: tests/Makefile.am @@ +1,3 @@ +noinst_desktopdir=$(builddir)/magnifier-prefs +noinst_DATA = $(noinst_desktopdir)/gnome-shell-magnifier-preferences.desktop +magprefsdir=`pwd`/magnifier-prefs The use of `pwd` looks wrong - you probably want $builddir here. Although I don't see a point in providing a desktop file without installing it - we consider applications without desktop files broken, but as the dialog will appear just as broken as if it didn't have one, I'd vote for removing it :-) There are a lot of unnecessary whitespace changes in this file which need to be removed from the patch. ::: tests/magnifier-prefs/magnifierPreferences.js @@ +62,3 @@ + // Gdk.Color components are of the form rrrrggggbbbb. + // Note: Gdk.color_parse() should do this, but I can't get it to work. + function parseColour(colorString) { Usually we avoid nested function definitions. Also you should use Color consistently :-) @@ +103,3 @@ + magOnRadio.set_active(aSetting); + magOffRadio.set_active(!aSetting); + magOnRadio.connect('clicked', Lang.bind(this, this.doShowHide, magOnRadio)); At least for this key, it feels wrong to not listen to key changes - the universal access status menu provides an interface to change this key (well, as of bug 636151, at least it should) and it's a bit weird to not have the dialog's UI following suit. Also note that signal handlers always pass the object which emits the signal in the parameter list, you don't have to pass it manually in Lang.bind(). Same applies for the other calls to Lang.bind(). @@ +167,3 @@ + let crosshairsOpacity = builder.get_object('xHairsOpacitySlider'); + aSetting = this._gSettings.get_int(CROSSHAIRS_OPACITY_KEY); + aSetting = aSetting / CROSSHAIRS_MAX_OPACITY; Just out of curiosity - why the mapping from the [0..255] to the [0..1] range? @@ +273,3 @@ + doCrosshairsLength: function(lengthSlider) { + this._gSettings.set_int(CROSSHAIRS_LENGTH_KEY, lengthSlider.get_value()); + } *sigh* All this code, and it only does half of the work (e.g. it doesn't update the controls when the associated setting changes) ... it really sucks that g_settings_bind() does not work, I'm pretty sure that writing this dialog in C would be shorter than the Javascript. Nothing you can do, just ranting. @@ +356,3 @@ + */ + doMoveZoomRegion: function(viewPort) { + if (this.haveFirstZoomRegionPath()) { I'd write this as: if (this._firstZoomRegionPath == null) return; // what you have now At least in my opinion the extra function makes the code less readable - it's too simple to justify having to scroll through the code to look up what it does. Reverting the condition and using a return statement saves one level of indentation, which is always a good thing :-)
(In reply to comment #34) > Review of attachment 174104 [details] [review]: I'm finally getting to this -- thanks for your input. > ... > > You should also add gnome-shell-magnifier-preferences to .gitignore (and its > desktop file as well, if you decide to keep it). Oh, right; good point. > > ::: tests/Makefile.am > @@ +1,3 @@ > +noinst_desktopdir=$(builddir)/magnifier-prefs > +noinst_DATA = $(noinst_desktopdir)/gnome-shell-magnifier-preferences.desktop > +magprefsdir=`pwd`/magnifier-prefs > > The use of `pwd` looks wrong - you probably want $builddir here. The point of $magprefsdir is to insert a full path into the desktop file. I'd use $builddir, but it's not a full path; it's just ".". Is there a way of getting a full path without using `pwd`? > Although I > don't see a point in providing a desktop file without installing it - we > consider applications without desktop files broken, but as the dialog will > appear just as broken as if it didn't have one, I'd vote for removing it :-) Having a double-clickable icon to launch the dialog has proven beneficial to those who don't know the first thing about terminal windows nor command lines. My vote is to keep it just for the convenience. > > There are a lot of unnecessary whitespace changes in this file which need to be > removed from the patch. If I had a nickel for every minute I spent on getting the tabs correct in these files... Hopefully this time. Fingers crossed. > Usually we avoid nested function definitions. Also you should use Color > consistently :-) Done. > + magOnRadio.connect('clicked', Lang.bind(this, this.doShowHide, > magOnRadio)); > > At least for this key, it feels wrong to not listen to key changes - the > universal access status menu provides an interface to change this key (well, as > of bug 636151, at least it should) and it's a bit weird to not have the > dialog's UI following suit. I went ahead and connected all the keys. > Just out of curiosity - why the mapping from the [0..255] to the [0..1] range? User friendliness. Users think of opacity/transparency in terms of, for example, "half transparent", not in terms of an 8 bit number. Clutter, on the other hand uses [0..255]. The opacity slider is for users, but, then, its value needs to be mapped to what Clutter expects. (Aside: there's a weak argument that the GSetting itself should be a floating point number, or perhaps a percentage. It's weak because, users shouldn't be looking directly at the GSetting). > *sigh* > All this code, and it only does half of the work (e.g. it doesn't update the > controls when the associated setting changes) ... it really sucks that > g_settings_bind() does not work, I'm pretty sure that writing this dialog in C > would be shorter than the Javascript. > > Nothing you can do, just ranting. I had a look at the documentation for g_settings_bind(). Very nice. When will it work in JavaScript (mostly rhetorical)? > > @@ +356,3 @@ > + */ > + doMoveZoomRegion: function(viewPort) { > + if (this.haveFirstZoomRegionPath()) { > > I'd write this as: > > if (this._firstZoomRegionPath == null) > return; > > // what you have now > > At least in my opinion the extra function makes the code less readable - it's > too simple to justify having to scroll through the code to look up what it > does. Reverting the condition and using a return statement saves one level of > indentation, which is always a good thing :-) Done. Okay, fixes have been made, and some changes to the UI based on suggestions from our designers. A new patch is coming up.
Created attachment 177787 [details] [review] Fixed tabs/backslashes in tests/Makefile.am. Added a preferences dialog for testing and demonstrating magnifier functions.
Comment on attachment 177787 [details] [review] Fixed tabs/backslashes in tests/Makefile.am. Goofed royally in terms of uploading the patch. Ignore this one.
Created attachment 178038 [details] [review] Added a preferences dialog for testing magnifier functionality. Added a preferences dialog for testing magnifier functionality.
(In reply to comment #35) > The point of $magprefsdir is to insert a full path into the desktop file. I'd > use $builddir, but it's not a full path; it's just ".". Is there a way of > getting a full path without using `pwd`? $(abs_builddir) should work then. > Having a double-clickable icon to launch the dialog has proven beneficial to > those who don't know the first thing about terminal windows nor command lines. It's a test script buried in a source tree - not exactly the friendliest environment for those kind of users ;-) Anyway, I don't object to keeping it ... > > Just out of curiosity - why the mapping from the [0..255] to the [0..1] range? > > User friendliness. OK, even though that argument is a bit weak - it's a testing tool, not an end user application.
Review of attachment 178038 [details] [review]: Except for the Makefile changes, the code looks pretty good. ::: tests/Makefile.am @@ +1,1 @@ +noinst_desktopdir=$(builddir)/magnifier-prefs Using $(builddir) in the variable is either unnecessary (if used with generated files) or wrong (if used with source files). You could make this noinst_desktopdir=magnifier-prefs, but it's not really shorter or more readable than using the directory name directly (especially as it's used for non-desktop files as well). It is common to have $(builddir) == $(srcdir), but you cannot rely on this. Especially "make distcheck" uses a separate build tree. Fortunately autotools is pretty good in figuring out whether $(builddir) or $(srcdir) should be prefixed, so just omitting it should work (with one exception noted below). @@ +1,3 @@ +noinst_desktopdir=$(builddir)/magnifier-prefs +noinst_DATA = $(noinst_desktopdir)/gnome-shell-magnifier-preferences.desktop +magprefsdir=`pwd`/magnifier-prefs As noted previously, you can use $(abs_builddir). @@ +26,3 @@ interactive/table.js \ testcommon/border-image.png \ + testcommon/ui.js \ Unnecessary whitespace change left. @@ +29,2 @@ unit/format.js EXTRA_DIST += $(TEST_JS) You will also need something like MAGNIFIER_PREFS = \ magnifier-prefs/magnifierPreferences.js \ magnifier-prefs/magnifier-preferences-icon.png \ magnifier-prefs/magnifier-preferences.ui EXTRA_DIST += $(MAGNIFIER_PREFS) Then you should add tests/magnifier-prefs/gnome-shell-magnifier-preferences.desktop.in.in tests/magnifier-prefs/magnifier-preferences.ui to po/POTFILES.skip @@ +40,3 @@ $< > $@ && chmod a+x $@ +$(noinst_desktopdir)/gnome-shell-magnifier-preferences: $(noinst_desktopdir)/gnome-shell-magnifier-preferences.in Apart from being wrong (as noted above), the target directory may not exist, so you'll need something like @ $(MKDIR_P) $(builddir)/magnifier-prefs before running the sed command (this should be the only place where you actually need $(builddir). ::: tests/magnifier-prefs/magnifier-preferences.ui @@ +9,3 @@ + <property name="default_width">300</property> + <property name="type_hint">normal</property> + <property name="has_separator">False</property> This property has been removed from GtkDialog. No biggie, but removing this line will get rid of an ugly warning. ::: tests/magnifier-prefs/magnifierPreferences.js @@ +114,3 @@ + + // Mouse tracking mode radio buttons ... + aSetting = this._gSettings.get_string(MOUSE_MODE_KEY); Unused. @@ +120,3 @@ + + // Screen position radio ... + aSetting = this._gSettings.get_string(SCREEN_POSITION_KEY); Unused. @@ +294,3 @@ + /*GObject*/ connect_object, + /*GConnectFlags*/ flags, + /*gpointer*/ user_data ) { The user_data parameter is always removed in JS. As you don't use connect_object and flags, you can omit those. I'd rather omit the type comments as well, and the style needs some fixing (no spaces before or after '(', no space before ')', camelCase). So I'd write that simply as _gtkConnectSignals: function(builder, widget, signal, handlerName) @@ +296,3 @@ + /*gpointer*/ user_data ) { + // Small hack (a hick?) -- 'connect_object' is null, but since 'this' is + // the handler object, that's ok -- use 'this' instead. The code looks perfectly fine to me, I'd remove the comment about the 'hack'.
(In reply to comment #40) Thanks very much! I have two niggling questions. > Review of attachment 178038 [details] [review]: > > Except for the Makefile changes, the code looks pretty good. > ... > @@ +26,3 @@ > interactive/table.js \ > testcommon/border-image.png \ > + testcommon/ui.js \ > > Unnecessary whitespace change left. I'm unclear as to what you are saying here. The version of Makefile.am in the repository, for the "testcommon/ui.js" line, has a bunch of spaces between the ".js" and the backslash. I replaced them with tabs. Was that wrong? Are you saying that they should be spaces? > ... > Apart from being wrong (as noted above), the target directory may not exist, > so you'll need something like > > @ $(MKDIR_P) $(builddir)/magnifier-prefs > > before running the sed command (this should be the only place where you > actually need $(builddir). My guess is that MKDIR_P is needed for each of the sed commands that relate to $(noinst_desktopdir). Is that correct?
(In reply to comment #41) > > Unnecessary whitespace change left. > > I'm unclear as to what you are saying here. The version of Makefile.am in the > repository, for the "testcommon/ui.js" line, has a bunch of spaces between the > ".js" and the backslash. I replaced them with tabs. Was that wrong? Are you > saying that they should be spaces? I didn't really look what the change actually did. You are correct that the backslashes should be aligned with tabs, but it's a change which has nothing to do with the rest of the patch. Doing a separate patch for that would be cleaner, but is likely overkill here - I'd say up to you if you leave it in or not. > My guess is that MKDIR_P is needed for each of the sed commands that relate to > $(noinst_desktopdir). Is that correct? Ah, guess the %.desktop.in:%.desktop.in.in rule needs this as well, right (the %.desktop:%.desktop.in rule doesn't need it, as it depends on the first rule having been built first).
Created attachment 179000 [details] [review] Minor fixes to magnifier preferences dialog code. [ To squash with magnifier preferences dialog patch ] * removed parseColor() from JS code and replaced it with Gdk.color_parse(). * tightened up ./tests/Makfile.am * added some ./tests/magnifier-prefs/ files to POTFILES.skip * removed has_separator property for GtkDialog in .ui file
I've discovered that the two magnifier preferences patches won't apply with the latest nightly. I'm in the process of creating a single patch to replace the two. It will contain the fixes from the latest review -- its purpose is to provide a patch that can be applied to the latest gnome-shell build. Coming up...
Created attachment 180014 [details] [review] Added a preferences dialog for testing and/or demonstrating magnifier functionality.
What is blocking getting this in currently, to get some more testing of it before 3.0?
(In reply to comment #46) > What is blocking getting this in currently, to get some more testing of it > before 3.0? One thing: Since the last patch, the gsettings schemas have moved (see 642034). That entails a change to a make file inside the current patch. I will upload a new one with that change sometime in the next two days. Of course, there may have been other changes since Feb. 3 that affect this. I'll check.
(In reply to comment #46) > What is blocking getting this in currently, to get some more testing of it > before 3.0? Andre - this is a test program, not a user-visible feature.
Created attachment 182396 [details] [review] Added a preferences dialog for testing and/or demonstrating magnifier functionality. Added a preferences dialog for testing and/or demonstrating magnifier functionality.
(In reply to comment #29) > I don't want to see a temporary settings dialog. I think we should fix the > magnifier defaults as in #629884. Fix the proportional mode as in #629950. > Add hotkeys to control the magnification. And help thos get the Universal > Access settings finished. As of this comment, I'll close the bug as duplicate of bug 643086 (which is about adding "real" ui to System Settings). Joseph - sorry for throwing away the work you put into this bug, but it's better to spend the effort on the actual user-facing interface. *** This bug has been marked as a duplicate of bug 643086 ***