GNOME Bugzilla – Bug 595507
Add magnification functionality.
Last modified: 2013-12-04 18:02:55 UTC
The goal is to replace the current corba based GnomeMag (http://live.gnome.org/GnomeMag) with a Magnifier that is part of gnome-shell.
Created attachment 143400 [details] [review] Basic Magnifier object (patch against master) "BasicMag.patch" is a first pass at adding a Magnifier object to gnome-shell. In terms of the shell itself, it adds a new top level actor "global.uiGroup" and organizes the stage Stage Magnifier UI group Window group Chrome group Overlay group The Magnifier object hasa clone of the UI group and displays it magnified. The Magnifier is a member of global, namely global.magnifier. By default, magnification is off when gnome-shell starts up. Current public methods of the magnifier are: setActive (true|false); set|getMagFactor(); set|getSize(); set|getPosition(); setFullScreenMode(); Lots to do...
Please see: http://live.gnome.org/GnomeShell/StyleGuide For how we write write objects in Javascript. + // Set up stage hierarchy for the Magnifier. + // Note: should this be done somewhere else? + // + uiGroup = new Clutter.Group( {name: "uiGroup"} ); + global.stage.remove_actor (global.window_group); + global.stage.remove_actor (global.overlay_group); + global.stage.remove_actor (chrome.actor); + uiGroup.add_actor (global.window_group); + uiGroup.add_actor (global.overlay_group); + uiGroup.add_actor (chrome.actor); I think this is the right place to do it ... I don't think being "magnifiable" is a generic Mutter concept, it's a concept that is specific to gnome-shell and this is the gnome-shell initialization code. Comment should explain that this is being done because we need to clone everything but the magnifier itself to magnify. You should actually use: global.window_group.reparent (uiGroup); It's about the same as the remove/add but makes sure that the actor won't get lost - it's not likely to get lost here, since a JS garbage collection would have to get run between the remove and add, but better to be safe... If you search through the code, you'll find various instances of 'stage.add_actor()' - these will need to be fixed or the magnifier won't pick up those elements.
Comment on attachment 143400 [details] [review] Basic Magnifier object (patch against master) Marking as 'needs-work' to clean up my search for patches needing review.
Created attachment 144028 [details] [review] Mouse tracking and preliminary UI for prefs (patch against master) Contains all functionality from "Basic Magnifier object" plus: - track the mouse such that the magnified view scrolls to show what is under the mouse. - a very preliminary UI for preferences (a Gtk window) that: - allows adjustment of the magnification factor, - toggles showing of the magnifer - toggles full screen mode. Per Comment 2, Re: setting up stage in main.js -- okay. Re: using reparent() -- yes, that's better; thanks for the tip. The latest patch file does it that way. Re: find and fixing occurrences of 'stage.add_actor()' -- still on my plate. Thinking out loud: is there is an event or signal to listen for, such as, "add_actor"? I'm wondering if that would be a better approach. Re: style guide -- also still on my plate. I have skimmed it, and have one problem with it. It prevents having genuine private members, and requires that private members actually be public. It's not clear to me why.
Created attachment 144837 [details] [review] Mouse tracking with large pointer is magnified view (patch against master) Contains all functionality from previous patch "Mouse tracking and preliminary UI for prefs" plus: - copied the large mouse pointer image from the corba gnome-mag project to show as the mouse pointer in the magnified view. - adds the "alt-tab" actor to the UI group so it is magnified automatically when it pops up.
Created attachment 145075 [details] [review] Magnified view follows mouse (patch against master) Contains all functionality from previous patch "Mouse tracking and preliminary UI for prefs" plus: - an option to allow the magnified view to overlay the current mouse position and follow the mouse as it moves about the screen. The effect is similar to moving a magnifying glass over the screen.
Created attachment 145979 [details] [review] Multiple screen positions for magnified view (patch against master) Magnified view positioning (patch against master) Contains all functionality from previous patch "Magnified view follows mouse" plus: - allows user to select the position of the magnified view. The patch includes new UI (a combobox) to allow the user to place the magnifer at one of: * left half of screen, * right half, * top half, * bottom half, * full screen, or * "fixed" The "fixed" view is located bottom left corner two-thirds down the side, 1/2 the width and 1/3 the height of the screen. (There is no particular reason for this position). All the positions, except full screen, can be set optionally to follow the mouse.
Created attachment 146783 [details] [review] Publish magnifier as a dbus service (patch against master) Contains all functionality from previous patch "Magnified view follows mouse" plus: - refactored scrollToMousePos() into a general scrollToPosition() that other clients, besides the internal Magnifier.scrollToMousePos(), can make use of. - tightened up scrolling of content to mouse position by checking whether the mouse actually moved. - added start|stop|isTrackingMouse(). - creates an 'org.gnome.Shell.Magnifier' dbus service that publishes a single method: 'shiftContentsTo (x, y)'. - minor clean-ups.
Created attachment 147022 [details] [review] 2nd version of magnifier as dbus service (patch against master) Duplicates all functionality from previous patch, "Publish magnifier as a dbus service (patch against master)", but the magnifier code now conforms to the GnomeShell style guide.
Created attachment 147130 [details] [review] DBus method to retrieve a rectangle delineating the region magnified. Duplicates all functionality from previous patch, "2nd version of magnifier as dbus service (patch against master)", and provides a method, getROI(), for retrieving the top, left, width and height of the region of the desktop currently visible within the magnified view. The method is also published via dbus, and returns a dbus.Struct(dbus.Int32, dbus.Int32, dbus.Int32, dbus.Int32). The Int32's are the top, left, width, and height respectively.
Created attachment 147602 [details] [review] Many Magnifier methods published via DBus for testing with Orca Duplicates all functionality from previous patch, "DBus method to retrieve a rectangle delineating the region magnified". New additions: * most of the magnifier methods are published through DBus (thanks Will!). Useful for testing with latest nightly build of Orca. * the magnifier preferences window (not the one Orca provides) is no longer close-able.
Review of attachment 147602 [details] [review]: ::: js/ui/magnifier.js @@ +122,3 @@ + + // Export to dbus. + global.grab_dbus_service(); Hmm, not sure why we need a separate DBus name here; you have a separate interface and object path, which should work fine. @@ +123,3 @@ + // Export to dbus. + global.grab_dbus_service(); + magDBusService = new MagnifierDBus.ShellMagnifier(); Do we expect the magnifier interface to be primarily used out-of-process via DBus? In that case it seems cleaner to me to make this object the same as the exported one, instead of having a separate proxy object. If it's used both in-process and out then this seems fine though. @@ +124,3 @@ + global.grab_dbus_service(); + magDBusService = new MagnifierDBus.ShellMagnifier(); + DBus.session.flush(); Why the flush()? ::: src/shell-global.c @@ +709,3 @@ + */ + /* ...and the org.gnome.Shell.Magnifier service. + So we should be able to drop this code.
Review of attachment 147602 [details] [review]: Sorry for being so slow in getting to review this; the comments below cover most of what I see that needs improvement. Some of the detail comments may not matter too much once the overall API and structure is worked out some more; I think figuring out who the API is for and what should be in it is really the first order of business. (Hint on this comment - it should be more readable if you follow the link to "Review" at the top of it and look at the comments in place in the various files rather than trying to figure it out from the raw Bugzilla comment.) In general, the thing that I'm most uncertain about is the relationship between: - The prefs UI you have implemented here - The control over the UI that is exported via DBus Do you see there being persistent storage of preferences in GConf, so someone would be able to turn on the magnifier on the bottom half of the screen, then get it again the next time they logged in? Or is the eventual goal that the UI would always be controlled dynamically via DBus (either by an AT like Orca or by a simpler tool)? If the idea is that preferences are "sticky" and read from GConf on startup, then I think the right approach is to do that hookup to GConf and make any GUI an (out-of-process) GUI for changing the GConf preferences. If the idea is that it's always DBus controlled, then we should approach a GUI from that direction. In either case, I don't think in in-process GUI with GTK+ makes a lot of sense; and there are considerable problems with creating toplevel GTK+ windows from within the Mutter process. A window manager like Mutter isn't easily able to manage its own windows. (There are some attempts in Mutter to do this, but they don't work perfectly.) A short-term approach to control of the magnifier would be to add magic debugging command to control it to the Alt-F2 dialog, like the 'lg' and 'restart' commands we have currently (see http://live.gnome.org/GnomeShell/CheatSheet for a description). I'd imagine something like: mag # Turn it on, default settings mag bottom 3 # Turn it on, at the bottom, scale factor 3 mag off # Turn it off I also don't completely understand what's going on with the D-Bus interface - it seems to control many things, but not everything. For example, there is no way to trigger the overlayMouse mode via the D-Bus interface. This makes it a bit hard to review; I'd generally like to see the code look like it was built from the D-Bus interface up - what is the interface we need to support, then what do we need in our code to support that. ::: js/ui/altTab.js @@ -61,3 +61,3 @@ this._disableHover(); - global.stage.add_actor(this.actor); + global.uiGroup.add_actor (this.actor); There are lots of similar places where stage is used, and you only change this one. ::: js/ui/magnifier.js @@ +12,3 @@ + +const DEFAULT_BACKGROUND_COLOR = new Clutter.Color(); +DEFAULT_BACKGROUND_COLOR.from_pixel (0x2266bbff); Rather than repeating this here, you should use Main.DEFAULT_BACKGROUND_COLOR @@ +16,3 @@ +// Default settings +const DEFAULT_X_MAGFACTOR = 2; // Magnification factor: 2x. +const DEFAULT_Y_MAGFACTOR = 2; Does it make sense to have separate X/Y magnification factors? (If that's needed for compat with the DBus interface, it would still simpler to have a single DEFAULT_MAGFACTOR constant here - I can't imagine a case where the *default* X and Y factors would be different.) @@ +26,3 @@ + top: Shell.Global.get().screen_height/3 * 2, + width: Shell.Global.get().screen_width/2, + height: Shell.Global.get().screen_height/3 I don't like accessing Shell.Global from code that is executed when the Javascript is loaded; we are essentially separating things that are static, and things that happen at the time when Main.init() is called. There's no reason that this needs to be a constant rather than just inline in setFixedSize(). (You could potentially have these as percentages, so left: 0, top: 2/3., width: 0.5, height: 0.3. But I don't think that is necesarry.) @@ +39,3 @@ +const RIGHT = "Right Half"; +const FIXED = "Fixed"; +const SCREEN_POSITIONS = [ FULL, TOP, BOTTOM, LEFT, RIGHT, FIXED ]; Compare: const MenuType = { NONE: 0, ON_RIGHT: 1, BELOW: 2 }; In appIcon.js for for how we normally do enums. Now obviously the way you've done it here relates to a) the D-Bus interface b) the way you build the GTK+ GUI, but since I've suggested elsewhere using integer values in the D-Bus interface and getting rid of the GTK+ GUI, maybe our standard way will work out well It seems to me that if the human readable description is "full screen" or "top half" the enum value should probably be FULL_SCREEN or TOP_HALF and not FULL and HALF. @@ +41,3 @@ +const SCREEN_POSITIONS = [ FULL, TOP, BOTTOM, LEFT, RIGHT, FIXED ]; + +const global = Shell.Global.get(); Not needed - the 'global' object is stored on the magic 'window' object so any module can access global. @@ +49,3 @@ + +Magnifier.prototype = { + _init: function (options) { Our style does not have a space between function name and options [also applies here to function()]. Needs fixing throughout. @@ +52,3 @@ + + // Initialized with a preferences UI? + this.prefsUI = ((options && options.prefsUI) ? options.prefsUI : null); It doesn't seem to me necessary to have this complicated scheme where the Magnifier can be created passing in a prefsUI, or the prefsUI can be created passing in a magnifier. (If prefsUI is kept) We just added a simple abstraction for handling functions of the form blah.doSomething({ someOption: true; }); See params.js, but it shouldn't be used if you have only 0 or 1 parameters. @@ +60,3 @@ + border: BORDER_WIDTH, + border_color: BORDER_COLOR, + name: "MagnifierRoot" You don't have to name your clutter actors; names are useful for CSS styling, but I think that's better added when converting the code to CSS and not ahead of time... we generally only name actors that we want to refer to as #someThing in the CSS style sheets. @@ +66,3 @@ + // Note: by default, the Clutter.Group constructor sets the default + // size to (0,0); but that causes the height to not clip properly when + // added to _magView. Setting temporarily to (1,1) fixes this. This just doesn't make sense to me @@ +76,3 @@ + + // Add a background for when the magnified uiGroup is scrolled + // out of view (don't want to see desktop showing through). I think you just shouldn't let scrolling over the edges of the screen happen @@ +78,3 @@ + // out of view (don't want to see desktop showing through). + let background = new Clutter.Rectangle ({ color: DEFAULT_BACKGROUND_COLOR, name: "MagBkgrnd" }); + aGroup.add_actor (background); Adding a rectangle without setting a size doesn't make sense to me - I think rectangles will default to 0,0 in size so not be visible @@ +86,3 @@ + + // store <aGroup> as a local property of _magView for easy access. + this._magView.mainGroup = aGroup; I don't see that you use it anywhere @@ +119,3 @@ + + // Track the mouse (default). + this.startTrackingMouse(); You don't need to comment 'startTrackingMouse' as 'Track the mouse'; there are a few places in the code where the comment level is too high. If the code speaks for itself, let it speak for itself. @@ +126,3 @@ + DBus.session.flush(); + + }, // end _init() We don't mark the ends of blocks like this, needs removing throughout (along with the blank lines before the close } for functions) @@ +141,3 @@ + else { + this._magView.hide(); + } Braces should be removed entirely here since both blocks are single line. If they were left, then } would be on the same line as else { @@ +150,3 @@ + isActive: function() { + let visible = this._magView.get_flags() & Clutter.ActorFlags.VISIBLE; + return (visible == Clutter.ActorFlags.VISIBLE); return (this._magView.get_flags() & Clutter.ACtorFlags.VISIBLE) != 0; Or even better: return this._magView.visible; @@ +156,3 @@ + /** + * setMagFactor: + * @inXmagFactor the power to set the horizontal magnification factor to We don't name our parameters like this, needs fixing throughout. @@ +157,3 @@ + * setMagFactor: + * @inXmagFactor the power to set the horizontal magnification factor to + * of the magnified view, e.g. 2x, 10x, etc. If less than It might be clearer if you said "of the magnified view. A value of 1.0 means unscaled, 2.0 means zoomed in by a factor of 2", or something like that. the 2x, 10x makes me wonder if an "x" is supposed to be in the input. @@ +158,3 @@ + * @inXmagFactor the power to set the horizontal magnification factor to + * of the magnified view, e.g. 2x, 10x, etc. If less than + * or equal to zero, it is treated as 1x. Is there any reason for this special casing? I could see clamping it to to 0 for sanity - it's probably not useful to have a magnifier that mirrors or turns things upside down, but I don't see a reason to document the behavior. @@ +163,3 @@ + * it is treated as 1x. + */ + setMagFactor: function (inXMagFactor, inYMagFactor) { If you have control over this name (I don't know if it is being consistent with an existing D-Bus interface or not), I think it should be called setMagFactors() to take into the account that it has two separate parameters. @@ +164,3 @@ + */ + setMagFactor: function (inXMagFactor, inYMagFactor) { + let x = ( inXMagFactor <= 0 ? 1 : inXMagFactor ); Parens are excess (if thids code remains) @@ +172,3 @@ + /** + * getMagFactor: + * @return an array, {x, y}, that contains the horizontal and vertical Should use javascript notation for an array [x, y] @@ +190,3 @@ + // Reposition the mouse to the centre. + let [cx, cy] = this.getCenter(); + this._cursorActor.set_position (cx, cy); Doesn't this just result in the actual mouse position and the actor position getting out of sync? @@ +208,3 @@ + * @inX The x-coord of the new position. + * @inY The y-coord of the new position. + * @return The old position [x, y]. This return seems arbitrary and not that useful @@ +230,3 @@ + * getCenter: + * @return an array, [x, y], that gives the centre of the + * magnified view. You need to specify the coordinate system, the implication reading this and the docs for getPosition() is that they would share the same coordinate system. But with this coordinate system, I don't think it's useful to have a function that returns width/2, height/2 @@ +246,3 @@ + let [x, y] = this._magView.get_position(); + if (x == 0 && y == 0) { + [x, y] = this._magView.get_size(); You should use separate width, height variables; using x, y to mean width and height is confusing @@ +251,3 @@ + } + } + return retVal; This could be simplified as: let [x, y] = this._magView.get_position(); if (x != 0 || y != 0) return false; let [width, height] = this._magView.get_size(); if (width != ...) return false; return true; @@ +264,3 @@ + * default is *not* hide. Optional. + * @return an array, [x, y], of the new position of the clone of + * the UI group This return value seems random; I think a separate accessor would be better... it's hard for me to even find where you use this return value. Note that even for D-Bus: setPosition() [x, y] = getPosition() Is not more round-trips than: [x, y] = setPosition() But if there's a reason that for some method returning an extra value for D-Bus is an efficiency or simplicity win, I'd just add it in the wrapper. @@ +267,3 @@ + */ + scrollToPosition: function (inX, inY, hideMouse) { + // coords in _uiGroupClone are scaled by the magnification factor. It's not the coords in _uiGroupClone that are sclaed, it's the coords in _magView that are scaled @@ +268,3 @@ + scrollToPosition: function (inX, inY, hideMouse) { + // coords in _uiGroupClone are scaled by the magnification factor. + let [X, Y] = this.getMagFactor(); No initial upper case for variables, using X,Y for scaleX, scaleY is confusing to me. @@ +280,3 @@ + // then move it such that it is centred over the mouse. Don't do + // this in full screen mode, however. + if (this._overlayMouse && !this.isFullScreenMode()) { What if the magnifier is almost the size of the screen? Should the position be clamped? If the position is clamped, is the special casing of the full-screen case necessary? @@ +285,3 @@ + + // Show/hide mouse as requested. + ( hideMouse ? this._cursorActor.hide() : this._cursorActor.show() ); We don't use the ternary in a situation like this with side-effects. Showing is not mentioned in the docs, just hiding. @@ +287,3 @@ + ( hideMouse ? this._cursorActor.hide() : this._cursorActor.show() ); + + return this._uiGroupClone.get_position(); I can't think of any reasonable interpretation of the the uiGroupClone position that would be useful to the caller @@ +298,3 @@ + * coordinates. + * @return an array, [left, top, width, height], representing the bounding + * rectangle of what is shown in the magnified view. I'd say [x, y, width, height] - left/width seems a bit mixed to my ear. @@ +317,3 @@ + let top = yCentre - height/2; + + return [left, top, width, height]; Introducing the center here makes this a bit more complicated than it needs to be, I'd probably write it like let [xFactor, yFactor] = this.getMagFactor(); let [cloneX, cloneY] = this.uiGroupClone.get_position(); let [width, height] = this.getSize(); let left = (0 - cloneX) / xFactor; let top = (0 - cloneY) / yFactor; let right = (width - cloneX) / xFactor; let bottom = (height - cloneY) / yFactor; return [left, top, (right - left), (bottom - top)]; @@ +328,3 @@ + * <code>global.imagedir/magCursors</code>. + */ + setMouseCursor: function (filename) { I don't think setting the cursor from a file has any relevance to the way we want this to finally end up. I'd say get it working with, say, a small black rectangle, and then do it right with getting the cursor from the server. (As referenced in my earlier mail, most of the code for that is already in ShellRecorder.) @@ +349,3 @@ + * @inFlag A boolean to set the overlay mouse mode with. + */ + setOverlayMouse: function (inFlag) { This name is confusing to me; I'd suggest that this should just be one of the options in setScreenPosition. @@ +368,3 @@ + */ + isOverlayMouse: function() { + return this._overlayMouse; A getter that is named like a setter should in my opinion return what is set, instead of a combination of what is set and some other factor. @@ +376,3 @@ + * Magnifier view occupies the top half of the screen. + */ + setTopHalf: function() { I think the only public function should be setScreenPosition(). Having some internal functions like this is OK (though I'm not sure it's better than doing it inline in setScreenPosition()), but if you have internal functions they should start with _ @@ +378,3 @@ + setTopHalf: function() { + this.setPosition (0, 0); + this.setSize (global.screen_width, global.screen_height/2); Operators like '/' should have spaces around them (here and elsewhere) @@ +420,3 @@ + * Magnifier view occupies a certain percentage of the screen. + */ + setFixed: function() { I don't see the point in a function that sets the magnifier to an unspecified hardcoded fixed position @@ +443,3 @@ + startTrackingMouse: function() { + // initialize previous mouse coord to undefined. + var prevCoord = { x: NaN, y: NaN }; We always use let and never var as a style thing @@ +445,3 @@ + var prevCoord = { x: NaN, y: NaN }; + var id = this.mouseTrackingId; + if (!id || id < 0) { You are guaranteed that timeout ID's are never zero (but *aren't* guaranteed that they are never negative), so you should just use (!this._mouseTrackingId) then set it to null when you stop tracking. @@ +448,3 @@ + this._mouseTrackingId = Mainloop.timeout_add ( + DEFAULT_MOUSE_POLL_TIME, + Lang.bind (this, this.scrollToMousePos, prevCoord) A function that is used for Mainloop.timeout_add() should return a boolean which is whether to keep running the timeout or to stop it. Here it seems you would always want to return true to mean "keep running" @@ +479,3 @@ + * @inPrevCoord: The previous mouse coordinates. Used for stop + * scrolling if the new position is the same as the last + * one. Note: optional. I don't understand the logic of this; seems unnecessary @@ +518,3 @@ + */ + resetFullScreenMode: function() { + this.setScreenPosition (this.getScreenPosition); Missing parens on getScreenPosition(). Generally don't like this - fullscreen is a screen position, special casing it is just weird. @@ +526,3 @@ + * @return true if it can or false otherwise. + */ + fullScreenCapable: function() { Not used. ::: js/ui/magnifierDBus.js @@ +16,3 @@ + { name: "getScreenSize", + inSignature: "", + outSignature: "(ii)" Unclear why this is (ii) [a structure with two integers], while getMagFactor is dd [two integers] - I don't see a reason for the structure @@ +56,3 @@ + /** + * setActive: + * @activate Boolean to activate of de-activate the Magnifier. 'Or' not 'of' @@ +59,3 @@ + * The magnified view is shown or hidden depending + * on the sense of the argument, but remains an + * actor on the stage. I don't think whether the magnified view remains active on the stage has any real real relevance to the user of the API. @@ +80,3 @@ + * @return an array, [width, height], representing the size of the screen + */ + getScreenSize: function() { This has no obvious role in the magnifier user interface - the screen size is property of the display and can be retrieved by the caller themselves. @@ +90,3 @@ + * screen: FULL, TOP, BOTTOM, LEFT, RIGHT, FIXED + * @position: one of "Full Screen", "Top Half", "Bottom Half", + * "Left Half", "Right Half", or "Fixed" I don't like using quasi-human readable strings as enum values. I think I'd just use an integer: @position: One of 0=fullscreen, 1=top half, 2=bottom half, ... (The other option would be to use string versions of the enumeration constants) @@ +113,3 @@ + * or equal to zero, it is treated as 1x. + * @inYmagFactor the power to set the vertical magnification factor to + * of the magnified view. If less thanor equal to zero, Missing ' ' @@ +142,3 @@ + * @x: x-coordinate to scroll to. + * @y: y-coordinate to scroll to. + * @return The new position (top left corner) of the magnified view. Unclear from the docs whether this is the top left corner of the magnifier on the screen or the top left corner of the ROI. Not sure a return value is necessary (efficiency discussed above); I think it would be simpler for the caller to just ask with a separate call. ::: js/ui/main.js @@ +112,3 @@ + global.window_group.reparent (uiGroup); + global.overlay_group.reparent (uiGroup); + chrome.actor.reparent (uiGroup); Rather than having to treat differently things that are created before and after the uiGroup is created, you should set up the uiGroup before creating things like the chrome and consistently use Main.uiGroup instead of the stage. @@ +114,3 @@ + chrome.actor.reparent (uiGroup); + global.stage.add_actor (uiGroup); + global.uiGroup = uiGroup; global is things that come from the C code; rather than extending it with Javascript objects, we use Main.<whatever>, so this is unnecessar. (Yes, the distinction is a little arbitrary - some other piece of code shouldn't have to worry about whether something is implemented in C or Javascript, but that's how we are handling for the moment.) @@ +142,3 @@ + // + magnifier = new Magnifier.Magnifier(); + global.magnifier = magnifier; Similarly, this is unnecessary.
Thanks Owen! (In reply to comment #13) > Do you see there being persistent storage of preferences in GConf, so someone > would be able to turn on the magnifier on the bottom half of the screen, then > get it again the next time they logged in? Or is the eventual goal that the UI > would always be controlled dynamically via DBus (either by an AT like Orca or > by a simpler tool)? It's likely to be both. We will have situations where the user wants to use the magnifier all on its own (e.g., as people do with eZoom in Compiz), but will also want to use assistive technologies such as Orca that provide tighter integration with speech and braille. In either case, I'm all for using gconf to store the more static magnifier settings and using the D-Bus API to handle the more dynamic interaction. For example, mouse cursor colors and cross hair preferences might be more static, but the region of interest will be more dynamic. As this is thought about, I think the gconf settings should also be considered to be more of desktop preferences for magnification in general as opposed to being specific for GNOME Shell (see below). > I also don't completely understand what's going on with the D-Bus interface - > it seems to control many things, but not everything. For example, there is no > way to trigger the overlayMouse mode via the D-Bus interface. This makes it a > bit hard to review; I'd generally like to see the code look like it was built > from the D-Bus interface up - what is the interface we need to support, then > what do we need in our code to support that. The current D-Bus interface is meant only as a minimal testing interface and I expect it to change over time. My hope for this round is that we can get agreement on the initial structural changes needed elsewhere in GNOME Shell and then we work on solidifying the API. The assumption, however, is that the review of the structural changes needed in GNOME Shell are the more important thing. One those are checked in, I'm hoping we can experiment/play with different ideas in the magnifier*.js files without the need for a detailed review for each commit (assuming the surface level spacing, indentation, block delimiter, camel case, etc., stuff is worked out). For the D-Bus API, I think we should create/use org.gnome.Magnifier (or something non-GNOME Shell specific). We are currently setting up a dependency on GNOME Shell for magnification, which is a big risk since there are currently no get-well plans for GNOME Shell a11y. At the same time, the current gnome-mag maintainer (Carlos) is looking to potentially migrate gnome-mag to D-Bus. If successful, this will provide us with a solution for people who don't (or can't) use GNOME Shell. > Does it make sense to have separate X/Y magnification factors? (If that's > needed for compat with the DBus interface, it would still simpler to have a > single DEFAULT_MAGFACTOR constant here - I can't imagine a case where the > *default* X and Y factors would be different.) In general practice, I believe the X/Y magnification factors are the same for most users. The mainstream commercial magnifiers that define the state-of-the-art, however, still seem to provide settings to allow you to separate the two. > + // Add a background for when the magnified uiGroup is scrolled > + // out of view (don't want to see desktop showing through). > > I think you just shouldn't let scrolling over the edges of the screen happen There are classes of users that use high magnification and prefer to keep their visual reference right in the middle of the screen. While existing magnifiers don't seem to support scrolling beyond the edges, this is an interesting optional feature that might be of use to this class of users. > + // Reposition the mouse to the centre. > + let [cx, cy] = this.getCenter(); > + this._cursorActor.set_position (cx, cy); > > Doesn't this just result in the actual mouse position and the actor position > getting out of sync? There will be cases, such as having Orca track keyboard focus or caret position, where the mouse and region of interest will indeed be out of sync. This is both desirable and necessary. Note that there are also existing models for handling these cases and options for what to do should the user bump the mouse when the mouse is out of view. They are not handled in the current code, but will need to be implemented. > I don't see the point in a function that sets the magnifier to an unspecified > hardcoded fixed position There are features where this is needed. For example, the user may want to magnify just an existing line of text in a document. As they arrow up/down through the document, the magnifier will change position to be over the current line. Something like Orca would know where the keyboard focus and caret were, so it would want to tell the magnifier how big it should be and where it should appear on the physical display. > + this._mouseTrackingId = Mainloop.timeout_add ( > + DEFAULT_MOUSE_POLL_TIME, > + Lang.bind (this, this.scrollToMousePos, prevCoord) > > A function that is used for Mainloop.timeout_add() should return a boolean > which is whether to keep running the timeout or to stop it. Here it seems you > would always want to return true to mean "keep running" Is there any better way to get the mouse position? Depending up the polling frequency, we can end up with an obvious 'jumpy' movement. Having an event driven solution might be better (if possible). > Missing parens on getScreenPosition(). Generally don't like this - fullscreen > is a screen position, special casing it is just weird. This may be a fall over from the gnome-mag code where full screen and partial screen were handled quite differently. Since GNOME Shell implies COMPOSITE, this distinction may not be necessary. > ::: js/ui/magnifierDBus.js > @@ +16,3 @@ > + { name: "getScreenSize", > + inSignature: "", > + outSignature: "(ii)" > > Unclear why this is (ii) [a structure with two integers], while getMagFactor is > dd [two integers] - I don't see a reason for the structure My bad, it was a mistake. Note that the mag factor should also be a floating point whereas screen size is in an integer number of pixels. > This has no obvious role in the magnifier user interface - the screen size is > property of the display and can be retrieved by the caller themselves. Fair enough. On a related note, we still need to work out handling multi-headed systems, where a typical use case is one head is the zoomed view and the other head is the desktop view. > I don't like using quasi-human readable strings as enum values. I think I'd > just use an integer: > > @position: One of 0=fullscreen, 1=top half, 2=bottom half, ... > > (The other option would be to use string versions of the enumeration constants) Agreed, and these probably should ultimately end up in a D-Bus API file somewhere that can be read by AT's so they can use names of constants vs. the actual value.
(In reply to comment #13) Thanks for you comments, Owen. I've reworked the code and incorporated most of your suggestions. Nonetheless, here are some comments about the latest patch (DBusOrca2.patch) as it relates: > In general, the thing that I'm most uncertain about is the relationship > between: > > - The prefs UI you have implemented here > - The control over the UI that is exported via DBus As Will noted above, the magnifier can both be driven by AT's such as Orca, or launched in-process. It's conceivable that some users want magnification without having to go off and start up some other app. From the standalone point of view, magnifier users want to easily turn magnification on, and, also, change the magnification factor without interacting with a dialog. Having a gesture to at least turn magnification on would be useful. Even better to have something similar to quickly change the magification factor. The exact way to do this is debatable -- key presses that involve the plus/minus keys would be a way to both turn it on, and change the magnification factor. This is similar to the zooming keystrokes in FireFox. As for the prefs UI, that was mostly for debugging, and I've removed it. Using GConf is a good approach. Magnifier preferences should be set with something like a "control panel", and saved such that any client (Orca, GnomeShell, gnome-mag, and so on) could use them. At some point, I'll code an out-of-process GUI to allow users to set their magnification preferences, and store them using GConf. The Magnifier object should read those settings and/or be sensitive to changes in them. > ... it would still simpler to have a > single DEFAULT_MAGFACTOR constant here - I can't imagine a case where the > *default* X and Y factors would be different.) Point well taken. However, the DBus implementation is following the CORBA interface for gnome-mag, and it does have default magnification factors for both X and Y. Maybe its's worth diverging now, but I'm inclined to go with whatever Will recommends here. > @@ +66,3 @@ > + // Note: by default, the Clutter.Group constructor sets the default > + // size to (0,0); but that causes the height to not clip properly when > + // added to _magView. Setting temporarily to (1,1) fixes this. > > This just doesn't make sense to me It's easily replicated. If you remove the size initializers, then the height of the uiGroup ClutterClone is not clipped at the bottom of the magnified view -- it bleeds right over the border and beyond. Actually, it doesn't matter if (1,1) is used; as long as some size is explicitly passed in. Even (0,0) will work. Sorry, but I haven't found the time to figure out where things are going wrong here. The latest patch uses width and height values of (0,0). > @@ +76,3 @@ > + > + // Add a background for when the magnified uiGroup is scrolled > + // out of view (don't want to see desktop showing through). > > I think you just shouldn't let scrolling over the edges of the screen happen As Will said, this is a user preference that we will be testing. With the latest patch, this is an option. > @@ +78,3 @@ > + // out of view (don't want to see desktop showing through). > + let background = new Clutter.Rectangle ({ color: > DEFAULT_BACKGROUND_COLOR, name: "MagBkgrnd" }); > + aGroup.add_actor (background); > > Adding a rectangle without setting a size doesn't make sense to me - I think > rectangles will default to 0,0 in size so not be visible In fact, it expands to the proper size. Maybe this is related to the clipping problem above with Clutter.Group()? And maybe the use of Big.BoxPackFlags.EXPAND? > @@ +163,3 @@ > + * it is treated as 1x. > + */ > + setMagFactor: function (inXMagFactor, inYMagFactor) { > > If you have control over this name (I don't know if it is being consistent with > an existing D-Bus interface or not), I think it should be called > setMagFactors() to take into the account that it has two separate parameters. I agree with the plural form, but, again, the current CORBA interface for gnome-mag has set the precedent. > @@ +190,3 @@ > + // Reposition the mouse to the centre. > + let [cx, cy] = this.getCenter(); > + this._cursorActor.set_position (cx, cy); > > Doesn't this just result in the actual mouse position and the actor position > getting out of sync? No. This implements a mode of mouse tracking known as "centered mouse tracking". The point under the actual mouse is diplayed at the center of the magnified view. The magnified mouse cursor image is "pasted" to the center of the magnified view. It isn't out of sync. > @@ +208,3 @@ > + * @inX The x-coord of the new position. > + * @inY The y-coord of the new position. > + * @return The old position [x, y]. > > This return seems arbitrary and not that useful The rationale was to provide some support for quickly reverting the view. One of the issues magnifier users have is orienting their location on the desktop. If the view changes drastically, then they could well desire to go back to their previous location with a single gesture. However, I'm not sure this is the best way to do that. I've taken it out. > @@ +280,3 @@ > + // then move it such that it is centred over the mouse. Don't do > + // this in full screen mode, however. > + if (this._overlayMouse && !this.isFullScreenMode()) { > > What if the magnifier is almost the size of the screen? Should the position be > clamped? If the position is clamped, is the special casing of the full-screen > case necessary? As Will said, clamping is something we are going to test with users. Regarding special casing: the special case here is a movable lens mode that is the size of the screen is not very usful. It might be useful when clamping is off, but that's unlikely. A common difficulty for magnifier users is maintaining orientation. The point of a lens is that it doesn't completely cover the screen; it's something small enough to easily move aside to see what is underneath. The "lens" is too big when in full screen mode. With clamping, lens mode plus full screen mode is even less useful since the the lens can't move. > I don't think setting the cursor from a file has any relevance to the way we > want this to finally end up. Agreed. But, I don't see much difference between using a stop-gap black rectangle vs. a stop-gap image, except that the image looks like a mouse, at least. I'll look at ShellRecorder to see how to interact with the actual mouse image, and once that's working, I'll ditch the image file approach. > @@ +368,3 @@ > + */ > + isOverlayMouse: function() { > + return this._overlayMouse; > > A getter that is named like a setter should in my opinion return what is set, > instead of a combination of what is set and some other factor. I don't follow. All that isOverlayMouse() is returning is the state of this._overlayMouse, which is a boolean value. What other factor do you see here? As for the name, it could be more intuitiive. I've changed it to this._lensMode, setLensMode(), and isLensMode(). > @@ +420,3 @@ > + * Magnifier view occupies a certain percentage of the screen. > + */ > + setFixed: function() { > > I don't see the point in a function that sets the magnifier to an unspecified > hardcoded fixed position Other magnifier products allow for a small area that is docked, ususally, to the bottom part of the screen. Users are allowed to define the size of this docked area, and it has a default size. setFixed() is the first step at providing this functionality. Should I take out because it's too preliminary? > @@ +479,3 @@ > + * @inPrevCoord: The previous mouse coordinates. Used for stop > + * scrolling if the new position is the same as the last > + * one. Note: optional. > > I don't understand the logic of this; seems unnecessary The rationale is: if, say, Orca, called upon the magnifier to reposition the view, and the mouse didn't move, then, don't move the view to the track the mouse. This would happen if the user is tabbing through, say, a tool bar. The tool bar button that currently has keyboard focus should be at the center of the magnified view. But, the instant the user moves the mouse, then the magnified view should again show the point under the mouse. In order to accomplish this, it's necessary to detect if the mouse moved. Comparing the previous location of the mouse to the current location is a way to do that. I'd rather use something along the lines of a mouse movement listener, but I couldn't find one in this context. Is there a better signal to look for? > @@ +80,3 @@ > + * @return an array, [width, height], representing the size of the screen > + */ > + getScreenSize: function() { > > This has no obvious role in the magnifier user interface - the screen size is > property of the display and can be retrieved by the caller themselves. I agree, and have removed it. As before, it relates to the CORBA interface. For compatibility with that, I've left it in the DBus object.
Created attachment 149218 [details] [review] Fixes for DBusOrca.patch Second version of "Many Magnifier methods published via DBus for testing with Orca" Duplicates all functionality from previous patch with these changes: * numerous changes as suggested by Owen. * added optional clamping of content scrolling at edges. * removed in-process magnifier preferences window.
*** Bug 603973 has been marked as a duplicate of this bug. ***
Created attachment 151423 [details] [review] Track system mouse cursor Duplicates all functionality from previous patch plus: * Adds a ShellXFixesCursor GObject that tracks the system mouse pointer to (1) acquire the cursor image to magnify, (2) watches for changes in that image, and (3) hides the actual mouse pointer when it is over the magnified view. * Re-centres the contents within the magnified view when the magnification factor is changed. * Re-positions the contents within the magnified view such that it includes the mouse when changing the screen position mode (e.g. top half vs. full screen). I'm most unsure of the C code for the ShellXFixesCursor object since I don't have a lot of experience with GObject (this is my first implementation). I'd be grateful for any pointers.
(bugzilla spam: populating the new "extensions" component with bugs discussing features that are not part of the core spec, but could be implemented externally once the extension system is done.)
(In reply to comment #19) > (bugzilla spam: populating the new "extensions" component with bugs discussing > features that are not part of the core spec, but could be implemented > externally once the extension system is done.) Any pointers to the extension API? Also, given the organization of the stage that the magnifier assumes, how is this is going to work as an extension? Perhaps it's too early to answer that. Still, a top level UIGroup actor should be part of the core. The organization of the stage was described in comment #1 above; repeated here: Stage Magnifier UI group Window group Chrome group Overlay group ... Any actor that qualifies as UI actor is contained within the "UI Group".
(In reply to comment #20) > Any pointers to the extension API? it's not really done yet > Also, given the organization of the stage that the magnifier assumes, how is > this is going to work as an extension? Perhaps it's too early to answer that. > Still, a top level UIGroup actor should be part of the core. yes, that much would have to be. to some extent the idea being marking these bugs as extensions was to get an idea of what sorts of hooks we'd need in the core in order to implement useful extensions.
(In reply to comment #21) > (In reply to comment #20) > > Any pointers to the extension API? > > it's not really done yet http://live.gnome.org/GnomeShell/Extensions has the basic overview > > Also, given the organization of the stage that the magnifier assumes, how is > > this is going to work as an extension? Perhaps it's too early to answer that. > > Still, a top level UIGroup actor should be part of the core. > > yes, that much would have to be. to some extent the idea being marking these > bugs as extensions was to get an idea of what sorts of hooks we'd need in the > core in order to implement useful extensions. Hm...ok, I'm not sure I necessarily see a lot of value in having the magnifier specifically out-of-tree. I actually run xmag right now, but if we had this in core i'd clearly use it. Some of the other things marked as extension are more likely.
Created attachment 152876 [details] [review] Different mouse tracking modes; uses GConf for some settings. Duplicates all functionality from previous patch plus: * Implements four mouse tracking modes, namely, centered, proportional, push, and "none". These are documented on the GnomeShell Magnification wiki page under "User Features" (http://live.gnome.org/GnomeShell/Magnification#User_Features). * Uses GConf to initialize some of the settings. These are saved in "/desktop/gnome/accessibility/magnifier/" They are: - show/hide magnifier - mouse tracking mode - screen position - magnification factor - lens mode - scrolling contents beyond edges.
Created attachment 154072 [details] [review] Splits Magnifier into Magnifier and ZoomRegion objects. DBus interface subset of gnome-mag's. Duplicates all functionality of previous patch with new architecture: * Magnifier object now a manager of ZoomRegion objects. * Dbus interface now matches gnome-mag's, implementing a subset of its methods (http://git.gnome.org/browse/gnome-mag/tree/xml). Known issue: the colour of the magnified mouse does not match that of the actual mouse. Suspect problem lies in XFixesCursor object (shell_xfixes_cursor.h, shell_xfixes_cursor.c).
Created attachment 156012 [details] [review] Adds crosshairs functionality Duplicates all functionality of previous patch with new functionality: * Crosshairs for all zoom regions. The show/hide, colour, thickness, length, opacity, and clipping* of the crosshairs are handled by GConf. The colour, length, thickness, and clipping* can also be handled using Dbus, matching gnome-mag in this respect (http://git.gnome.org/browse/gnome-mag/tree/xml/Magnifier.xml). The previous issue where the magnified mouse colours do not match those of the actual mouse remains. *clipping = a transparent clip rectangle at the intersection of the horizontal and vertical
Review of attachment 156012 [details] [review]: ::: js/ui/magnifier.js @@ +6,3 @@ +const Gdk = imports.gi.Gdk; +const DBus = imports.dbus; +/* -*- mode: js2; js2-basic-offset: 4; indent-tabs-mode: nil -*- */ I realize this patch has been outstanding for a long time and predates our St conversion, but it'd be nice not to add any new uses of Big now. You only have a few below that could easily be replaced with a St.BoxLayout and CSS. @@ +67,3 @@ +let magDBusService = null; + +let xhiDentityCount = 0; This mysteriously-named variable appears not to be used? @@ +83,3 @@ + this._mouseSprite = new Clutter.Texture(); + if (pixbuf) + Shell.clutter_texture_set_from_pixbuf(this._mouseSprite, pixbuf); So if the only use of Shell.XFixesCursor.get_gdkpixbuf is to then convert it into a texture, why don't we just do this directly inside Shell.XFixesCursor? In fact I think that would simplify the code because you can just call cogl_texture_new_from_data and tell it the source data format and let the conversion happen inside COGL which will be more efficient too. @@ +1363,3 @@ + setOpacity: function(opacity) { + // set_opacity() throws an exception for values outside the range + // [0, 255]. I'd much rather we do: if (opacity < 0) opacity = 0; else if (opacity > 255) opacity = 255; ::: js/ui/magnifierDBus.js @@ +44,3 @@ + { name: 'setMagFactor', inSignature: 'dd', outSignature: ''}, + { name: 'getMagFactor', inSignature: '', outSignature: 'dd' }, + { name: 'setRoi', inSignature: 'ai', outSignature: '' }, If we want this to be exactly 4 integers it should be a structure, like: inSignature: '(iiii)' @@ +46,3 @@ + { name: 'setRoi', inSignature: 'ai', outSignature: '' }, + { name: 'getRoi', inSignature: '', outSignature: 'ai' }, + { name: 'moveResize', inSignature: 'ai', outSignature: '' }, Same for these two @@ +102,3 @@ + * createZoomRegion: + * Allocate a ZoomRegion instance and return that as a + * /org/freedesktop/Magnifier/ZoomRegion. I'd say "Create a new ZoomRegion and return its object path." @@ +143,3 @@ + else + return false; + }, I don't see why this method is necessary - the createZoomRegion implicitly adds it. @@ +192,3 @@ + let proxyAndZoomer = this._zoomers[objectPath]; + proxyAndZoomer.zoomRegionProxy = null; + proxyAndZoomer.zoomRegion = null; You should also call dbus.SessionBus.unexportObject(objectPath) here @@ +197,3 @@ + for (let objectPath in this._zoomers) { + log ("SHOULDN'T GET HERE (" + objectPath + ")"); + } Debugging code left over. ::: js/ui/main.js @@ +119,3 @@ + global.window_group.reparent(uiGroup); + uiGroup = new Clutter.Group(); + // Set up stage hierarchy to group all UI actors under one container. I'd like to see this as a separate patch. You can do this as follows: git reset HEAD^ git commit -c ORIG_HEAD src/shell-global.c src/shell-xfixes-cursor.[ch] js/ui/magnifier*.js That will give you a commit with just the magnifier bits. Then do: git commit -a Now you have two commits, but really they should be in the other order. So do: git rebase -i master In the editor, swap the order of the two lines. ::: src/shell-global.c @@ +27,3 @@ #define SHELL_DBUS_SERVICE "org.gnome.Shell" +#define MAGNIFIER_DBUS_SERVICE "org.freedesktop.Magnifier" So I'm not sure if you're aware, and honestly I don't think this is a big deal or something to fix now, but - there was an epic flamewar on xdg-list a while back about having some sort of standardization process for claiming "org.freedesktop" DBus names. It might be worth just using org.gnome for now. It's easy enough to grab the fd.o name later and export the same object on it. ::: src/shell-xfixes-cursor.c @@ +48,3 @@ +G_DEFINE_TYPE(ShellXFixesCursor, shell_xfixes_cursor, G_TYPE_OBJECT); + +static guint cursor_change_signal = 0; While this isn't wrong, I'd prefer the more traditional: enum { CURSOR_CHANGED, LAST_SIGNAL }; static guint signals[LAST_SIGNAL] = { 0 }; @@ +202,3 @@ + pixval = GUINT_TO_LE (cursor_image->pixels[i]); + cursor_image->pixels[i] = pixval; + } This conversion needs to be surrounded with #if G_BYTE_ORDER == G_LITTLE_ENDIAN @@ +302,3 @@ + "Stage for mouse cursor", + CLUTTER_TYPE_STAGE, + G_PARAM_READWRITE)); Since the stage will never change, you can mark this also with G_PARAM_CONSTRUCT_ONLY. @@ +320,3 @@ + NULL); +} + We probably want this to be a singleton, right? For our current pattern on this, see e.g. shell_app_system_get_default in src/shell-app-system.c @@ +353,3 @@ + * @xfixes_cursor: the #ShellXFixesCursor + * + * Return the current mouse cursor image. Annotate this as: Returns: (transfer none): Return the current mouse cursor image. ::: src/shell-xfixes-cursor.h @@ +15,3 @@ +#include <clutter/clutter.h> +#define __SHELL_XFIXES_CURSOR_H__ +/* -*- mode: C; c-file-style: "gnu"; indent-tabs-mode: nil; -*- */ The gtk-doc section normally goes in the .c file.
(In reply to comment #26) Thank you for the review, Colin! > Review of attachment 156012 [details] [review]: > ... > I realize this patch has been outstanding for a long time and predates our St > conversion, but it'd be nice not to add any new uses of Big now. You only have > a few below that could easily be replaced with a St.BoxLayout and CSS. Sure. If Std.Boxlayout works the same way that Big.Box does in this context, and that's the preferred technique, then that's the way to go. > @@ +83,3 @@ > + this._mouseSprite = new Clutter.Texture(); > + if (pixbuf) > + Shell.clutter_texture_set_from_pixbuf(this._mouseSprite, pixbuf); > > So if the only use of Shell.XFixesCursor.get_gdkpixbuf is to then convert it > into a texture, why don't we just do this directly inside Shell.XFixesCursor? > In fact I think that would simplify the code because you can just call > cogl_texture_new_from_data and tell it the source data format and let the > conversion happen inside COGL which will be more efficient too. Yeah, that might also fix the sprite colour issue. Or, maybe that problem is related to your G_BYTE_ORDER suggestion. It looks like what you're suggesting is similar to what is being done in Shell.Recorder. I'll crib from there. > ::: js/ui/magnifierDBus.js > @@ +44,3 @@ > + { name: 'setMagFactor', inSignature: 'dd', outSignature: ''}, > + { name: 'getMagFactor', inSignature: '', outSignature: 'dd' }, > + { name: 'setRoi', inSignature: 'ai', outSignature: '' }, > > If we want this to be exactly 4 integers it should be a structure, like: > inSignature: '(iiii)' Regarding Dbus: I agree with both passing a structure rather an array for methods like 'setRoi()', and with the lack of need for addZoomRegion(). However ... Orca wants to use a common magnifier service API. To that end, Will Walker advised that the GnomeShell magnifier mimic the gnome-mag Dbus API. So: to make these changes to magnifierDbus.js requires a group decision. I'll consult with others, although I believe gnome-mag is shipping with 2.30. > ::: js/ui/main.js > @@ +119,3 @@ > + global.window_group.reparent(uiGroup); > + uiGroup = new Clutter.Group(); > + // Set up stage hierarchy to group all UI actors under one container. > > I'd like to see this as a separate patch. You can do this as follows: > [git gymnastics] Okay. I presume the purpose is to separate changes to the stage's organization (in terms of a "uiGroup") from the magnifier per se. A question: should I do this after the other modifications? or before? As for the rest -- the leftover debug variables, log messages, avoiding try/catch, enumerating signals, etc. -- those strike me as simple and straight forward, and I'll just go ahead and make the changes. Thanks again.
Created attachment 159879 [details] [review] Reorganize stage in terms of a UI Group actor and everything else. In preparation for adding magnification, "uiGroup.patch", organizes the stage along the following lines: Stage *Magnifier UI group Window group Chrome group Overlay group Alt tab App display Chrome ... This allows a magnifier actor to clone and magnify the UI group. The magnifier is a sibling of the UI Group in this stage oraganization -- see the next patch, "Magnifier.patch". (*Note: this patch does not contain any Magnifier).
Created attachment 159880 [details] [review] Adds magnifier functionality Together with the patch, "uiGroup.patch", this adds the same functionality as, and replaces "Crosshairs.patch". This code incorporates many of Colin's suggested changes, including replacing the use of Big.Box with St.BoxLayout. With respect specifically to magnification as a DBus service, the name of the service has changed to "org.gnome.Magnifier" (was org.freedesktop.Magnifier), and adds a method shiftContentsTo(x,y) to the ZoomRegion object. Other than that, the DBus interface is unchanged since it is currently in flux -- there is a discussion on gnome-accessibility-list@gnome.org regarding syncing the interface up with gnome-mag, and making the service useful to apps such as Orca. As the interface firms up, changes will be reflected in future contributions.
Created attachment 160274 [details] [review] Reorganize stage in terms of a UI Group actor and everything else (A). Replaces previous "uiGroup.patch" due to recent changes in .../js/ui/chrome.js.
Review of attachment 160274 [details] [review]: Looks fine.
Review of attachment 159880 [details] [review]: A couple small style issues, otherwise I think we're about at the finish line =) ::: js/ui/magnifier.js @@ +63,3 @@ +const BORDER_WIDTH = 2; +const BORDER_STYLE = 'border: 2px solid rgba(128, 0, 0, 255);' +const NO_BORDER_STYLE = 'border-width: 0px'; This should be done in gnome-shell.css; like: .magnifier-zoom-region:active { border: 2px solid rgba(128, 0, 0, 255); } Then you can use .add_style_pseudo_class('active') and .remove_style_pseudo_class('active') @@ +604,3 @@ + this._magView = new St.BoxLayout({ style: BORDER_STYLE }); + this._magView.hide(); + global.stage.add_actor(this._magView); These two lines won't do what you want - Clutter actors are shown when they're added to a parent. You need to invert them to have the magView be in a parent, but hidden. @@ +608,3 @@ + // Append a Clutter.Group to clip the contents of the magnified view. + this._mainGroup = new Clutter.Group({ clip_to_allocation: true }); + this._magView.add_actor(this._mainGroup); Following a later comment: why not just use a St.Bin since the magView only has one child? So above: this._magView = new St.Bin({ y_fill: true, x_fill: true }) then here use: this._magView.set_child(this._mainGroup) @@ +736,3 @@ + + // Remove border is the view port is the entire screen. Otherwise, + // insure that the border is there. You mean "ensure" I think...though maybe this is a US english difference? @@ +740,3 @@ + let showCrosshairs = gConf.get_boolean(SHOW_CROSS_HAIRS_KEY); + this._addBorder(); + ); I'm not sure this is going to interact well with multiple monitors. You probably want things to be in terms of monitors, but let's punt on this for now. The shell in general has various multimonitor issues, and it makes a lot more sense to get this patch in first and then iterate on multimonitor issues later. @@ +777,3 @@ + this._mainGroup.set_size(width - BORDER_WIDTH * 2, height - BORDER_WIDTH * 2); + else + this._mainGroup.set_size(width, height); See above for how you can avoid setting the _mainGroup size here. You can then also delete the BORDER_WIDTH constant.
Created attachment 160465 [details] [review] Adds magnifier functionality (A) Modified as per Colin's review (https://bugzilla.gnome.org/show_bug.cgi?id=595507#c32). > I'm not sure this is going to interact well with multiple monitors. You > probably want things to be in terms of monitors, but let's punt on this > for now. The shell in general has various multimonitor issues, and it > makes a lot more sense to get this patch in first and then iterate on > multimonitor issues later. Agreed. Multiple monitors is one of the features to be added, where the non-magnified view is on one monitor, and the magnified view is on the other (http://live.gnome.org/GnomeShell/Magnification#Features_in_Development). But I've been told that this may not be possible with GnomeShell. "We'll burn that bridge when we come to it."
If you've fixed everything from the previous review and didn't add anything you think might need review, just go ahead and commit this please! Otherwise let me know and I'll do the interdiff again.
* Uses GConf to initialize some of the settings. These are saved in "/desktop/gnome/accessibility/magnifier/" What defines this schema? It looks like a variety of a11y schemas are defined by libgnome, but I don't see /magnifier there.
(In reply to comment #35) > * Uses GConf to initialize some of the settings. These are saved in > "/desktop/gnome/accessibility/magnifier/" > > What defines this schema? It looks like a variety of a11y schemas are defined > by libgnome, but I don't see /magnifier there. Short answer: nothing. Longer answer: With the older gnome-mag project, all of the settings were communicated solely via DBus. Using GConf for user preferences is a recent development suggested by Will Walker. I suggested the names for the GConf settings, and he said to go with them. But, the bottom line is: there are no a11y templates for them; it's a new approach. A related activity is the new AT preferences work (http://live.gnome.org/Accessibility/PreferencesRework) where some magnifier settings are noted, but they are fewer than what I have. How should I/we proceed?
Something needs to define the schemas; otherwise it's a fatal error on startup. For example try "gconftool-2 --recursive-unset /desktop/gnome/accessibility" and try starting gnome-shell. Not sure...I guess the precedent is to add them to libgnome.
(In reply to comment #37) > Something needs to define the schemas; otherwise it's a fatal error on startup. > For example try "gconftool-2 --recursive-unset /desktop/gnome/accessibility" > and try starting gnome-shell. > > Not sure...I guess the precedent is to add them to libgnome. Can we throw them into gnome-shell.schemas for now? we definitely don't want libgnome in our jhbuild, I think. The question of where shared desktop-wide schemas live with GSettings probably should be brought up on desktop-devel-list.
> > Not sure...I guess the precedent is to add them to libgnome. libgnome is deprecated and removed from GNOME 3.0. > The question of where shared desktop-wide schemas live with GSettings probably > should be brought up on desktop-devel-list. (For the similar problem with schemes in gnome-vfs see bug 600686.)
Created attachment 160745 [details] [review] Adds magnifier functionality (B). Adds magnifier functionality. GConf schemas for magnifier settings located in .../data/gnome-shell.schemas. Adds magnifier functionality. GConf schemas for magnifier settings located in .../data/gnome-shell.schemas.
Ok there's just one item left for this - can you write a commit message? The current patch has the changes since the last patch (useful for review), but we need one commit message that describes what the patch does as a whole. It doesn't have to be very long; probably just mention the overall goal, the dbus interface, the schemas etc.
Created attachment 160767 [details] [review] Adds magnifier functionality to gnome-shell. Adds the ability to create one or more zoom regions that show magnified or enhanced views of the desktop. The magnifier provides options for: * magnification factor, * four mouse tracking modes common to screen magnifiers, * positioning the magnified view in one of four screen location, or full screen, * crosshairs to accentuate the position of the mouse, * user preferences persistence via GConf (schemas in .../data/gnome-shell.schemas). * a DBus API to allow other processes to drive the magnifier as a service.
Attachment 160767 [details] pushed as 7b7c34a - Adds magnifier functionality to gnome-shell.
Great, thanks a lot for the hard work on this patch! I've gone ahead and pushed this. I noticed a small issue or two though which I'll do as separate patches for clarity.