GNOME Bugzilla – Bug 647074
GNOME Shell Magnifier should track focus and the caret
Last modified: 2014-01-20 23:44:27 UTC
GNOME Shell Magnifier does not track focus or the caret. As a result, GNOME Shell Magnifier users who prefer the keyboard must either regularly move the mouse to see the active area, or use Orca to cause the area of interest to be displayed by the magnifier. Screen magnification solutions in other platforms are able to track both focus and caret independently. Ideally, GNOME Shell Magnifier should display the focused area and the caret (where applicable) on its own. For those users who require both magnification and speech output and/or braille, Orca and GNOME Shell Magnifier should happily coexist, each doing the jobs they are intended to do without interfering with one another. Aside from being (what I believe to be) the RightThing(tm) to do, as a happy side effect, were Orca expected merely to co-exist with GNOME Shell Magnifier rather than control it, its GUI would be much, much simpler and smaller. ;-) ;-) Thanks in advance for your consideration of this request.
we'll want focus/caret tracking for the on-screen keyboard too, so this should be implemented independently of the magnifier, as an object/utility that both the magnifier and the keyboard can use.
(In reply to comment #1) > we'll want focus/caret tracking for the on-screen keyboard too, so this should > be implemented independently of the magnifier, as an object/utility that both > the magnifier and the keyboard can use. I agree. I'd go further: focus tracking is a general function that most adaptive technologies make use of. It's not just magnifiers and onscreen keyboards. And, other things not typically considered adaptive technology would likely benefit.
Has work on this started ?
Matthias -- looks like it should start soon: https://live.gnome.org/ThreePointFive/Features/FocusCaretTracking I really hope so -- it's pretty essential to make the on-screen keyboard useful on tablets.
Created attachment 219583 [details] [review] Preliminary focus/caret tracker object. This a preliminary patch for a focus/caret tracking object. This is NOT the final version. It's put here at the request of some developers who want to see the code, and to make suggestions. Tracking is done by using AT-SPI's EventListener. There are known problems: 1. Tracking focus and caret changes works great with GTK widgets, and even within web pages with ARIA widgets. But, whenever a GNOME Shell widget is focussed, the system freezes for about 5 - 10 seconds. When the UI is responsive again, the AT-SPI event passed back has no accessible component within it. Examples of GNOME Shell widgets include the task switcher, the menus at the top right, and the debugger. 2. Registering for 'object:text-caret-moved' works too well. Background: for debugging purposes, there is an onFocus() function at the bottom of focusCaretTracker.js that can be used to report the caret-moved events. Tracking is logged to the terminal. How it "works too well": If FireFox is the front-most window, and the caret is moving in FireFox's location bar, that is duly reported in the terminal window. But, immediately thereafter, the cursor movement in the terminal window (due to these very same log messages) is also reported. Is there any way to focus (pun intended) the caret-tracking events to only the active window, or focussed widget? Any help is appreciated.
(In reply to comment #5) > Created an attachment (id=219583) [details] [review] > Preliminary focus/caret tracker object. > ...skip... > Any help is appreciated. Not really something to help but a explanatory comment, as not all the people CCed to this bug were included on the previous discussion. Joseph recently opened a thread about this on gnome-accessibility, basically due all the problems that he found: https://mail.gnome.org/archives/gnome-accessibility-list/2012-July/msg00007.html The final conclusion is that most of the problems that he found (like that 5-10 second wait) only happens implementing this with javascript. All that functionality seems to work fine using python or C. And it seems that Joseph also proposed to implement the core of the functionality on C: https://mail.gnome.org/archives/gnome-accessibility-list/2012-July/msg00015.html So I think that it is worth to mention here, to see what other gnome-shell developers think about having this feature part implemented on C, part implemented on javascript. FWIW, I really think that this shouldn't be a really big issue. ShellGenericContainer is still on gnome-shell, and created because at that moment javascript didn't have some gobject subclassing features. And AFAIK, some of the possible reasons to have stuff written on C would be ensure a proper performance, as this could be the case.
(In reply to comment #6) > ... <skip> ... > And it seems that Joseph > also proposed to implement the core of the functionality on C: > https://mail.gnome.org/archives/gnome-accessibility-list/2012-July/msg00015.html > > ... <skip> ... > And AFAIK, > some of the possible reasons to have stuff written on C would be ensure a > proper performance, as this could be the case. I ported the JavaScript focus tracker to a GObject, and got the same results. So, something about the way AT-SPI is being accessed is not right (the language doesn't matter). (Aleiva, I know you are working on something similar; hopefully you're getting further than I am). Patch forthcoming showing the GObject code.
Created attachment 220022 [details] [review] Testing GObject version of the focus tracker. Port of the JavaScript focus tracker to a GObject. Still very preliminary, and put here to show the approach.
Mike Gorse and I discussed the JavaScript focus tracker (comment 5), and he wondered if it's a reentrant issue in Atspi. In a followup discussion with API, he suggested a way to double-check that is to run the GObject version (comment 8) as a standalone process running independently of gnome-shell, and see if the delay still happens with gnome-shell UI widgets. It doesn't. In fact, one can run the JavaScript focus tracker standalone, in a gjs process separate from gnome-shell, and it receives focus events from gnome-shell widgets in a timely manner. So: if the tracker is running in a process outside of gnome-shell, there is no delay in receiving events from gnome-shell. But if the event emitter and listener are in the same process, then it doesn't work properly. Time to look more closely at Atspi.
Hey i haven't been following this bug too closely, but are there synchronous d-bus calls going on "under the hood" from gnome-shell to gnome-shell when you see the pauses?
(In reply to comment #10) > Hey i haven't been following this bug too closely, but are there synchronous > d-bus calls going on "under the hood" from gnome-shell to gnome-shell when you > see the pauses? AT-SPI is *supposed* to run a dbus_connection_read_write_dispatch loop, rather than calling dbus_connection_send_with_reply_and_block, when the AT and the application are using the same D-Bus connection. Also, I don't think I've reproduced the bug as described. I have, however, seen a deadlock that can happen if two ATs that are also applications are running and they are querying each other at the same time. In this case, the normal dbus_connection_send_with_reply_and_block gets called, and neither process replies to the message from the other process because they are blocking while waiting for a reply to their own call. So I wonder if working around this would fix the other freeze. Also, it is possible to monitor D-Bus traffic as follows: dbus-monitor --address `xprop -root | grep AT_SPI_BUS | sed -e 's/.*= "//' | sed -e 's/"$//'` However, traffic is sent using direct D-Bus connections by default, to avoid the expense of routing lots of messages through dbus-daemon, so running dbus-monitor in this way will not normally capture method calls. This behavior can be changed by passing --disable-p2p when configuring at-spi2-atk.
*** Bug 699692 has been marked as a duplicate of this bug. ***
Created attachment 246474 [details] [review] Preliminary focusCaretTracker: Updated I have updated Joseph's patch from last year so it can be applied to the latest build. Please let me know what you think. Thanks.
Created attachment 246556 [details] [review] Preliminary focusCaretTracker: Updated Added log statement for printing the results from the get_role_name() function.
Created attachment 246590 [details] [review] Preliminary focusCaretTracker: Updated Clearer log messages
Created attachment 248531 [details] [review] Complete focusCaretTracker This focusCaretTracker.js is complete. The GNOME Shell magnifier can make use of it.
Review of attachment 248531 [details] [review]: There's no use of this anywhere. Do you have a patch that uses this, so I can review the API in context? We tend not to land things like this on some premonition that it will be used in the future. ::: js/Makefile.am @@ +6,2 @@ misc/config.js: misc/config.js.in Makefile + [ -d $(@D) ] || $(mkdir_p) $(@D) ; \ Unnecessary whitespace change. @@ +85,3 @@ ui/screencast.js \ ui/screenshot.js \ + ui/screenShield.js \ Unnecessary whitespace change. ::: js/ui/focusCaretTracker.js @@ +26,3 @@ +const Signals = imports.signals; + +let _atspiCallback = null; Is there a reason this is global? Why not just do Atspi.EventListener.new(Lang.bind(this, this._changed)); @@ +32,3 @@ + + _init: function() { + Atspi.init(); Hm, doesn't something else (either in GTK+, Clutter, or gnome-shell) already init atspi? I'd prefer not to have this in a constructor, though. @@ +42,3 @@ + /** + * shutDown. + */ Remove these useless comments. @@ +43,3 @@ + * shutDown. + */ + shutDown: function() { Does something ever call this? @@ +57,3 @@ + if (this._trackingFocus) { + return true; + } Bad style. Remove leading whitespace and braces. @@ +71,3 @@ + } + catch (err) { + log(err); Needs to be removed. @@ +73,3 @@ + log(err); + focusRegistered = false; + selectRegistered = false; You don't seem to use selectRegistered? If selectRegistered fails, we'll also incorrectly clear focusRegistered. What errors here could pop up in a well-configured shell session? @@ +96,3 @@ + } + catch (err) { + log(err); When could this happen? @@ +183,3 @@ + */ +FocusCaretTracker.prototype._connect = FocusCaretTracker.prototype.connect; +FocusCaretTracker.prototype.connect = function(name, callback) { This is absolutely awful. You need to figure out something better here. Why do we want to lazily initialize focus / caret events? DBus network traffic? Make it an explicit ref-counted thing, then. ::: js/ui/main.js @@ +160,3 @@ wm = new WindowManager.WindowManager(); magnifier = new Magnifier.Magnifier(); + focusCaretTracker = new FocusCaretTracker.FocusCaretTracker(); Does this need to be a singleton? Why can't whatever uses the FocusCaretTracker just create one of its own? @@ +163,2 @@ if (LoginManager.canLock()) screenShield = new ScreenShield.ScreenShield(); Bad whitespace.
(In reply to comment #17) Thanks for the review, Jasper. Replies inline for some of your comments. > Review of attachment 248531 [details] [review]: > > There's no use of this anywhere. Do you have a patch that uses this, so I can > review the API in context? > > We tend not to land things like this on some premonition that it will be used > in the future. Magdalen is currently working on integrating this with the magnifier. That work is too rough to include here yet. I'm not sure how to proceed. Should we wait until all of it is done? There was interest in the tracker from the onscreen keyboard developers -- see comment 1. > ::: js/ui/focusCaretTracker.js > @@ +26,3 @@ > +const Signals = imports.signals; > + > +let _atspiCallback = null; > > Is there a reason this is global? Why not just do > Atspi.EventListener.new(Lang.bind(this, this._changed)); This is probably a holdover from debugging, and it is likely safe to discard now. But, just for clarification, it is at least namespaced, and not fully global, i.e., FocusCaretTracker._atspiCallback. > > @@ +32,3 @@ > + > + _init: function() { > + Atspi.init(); > > Hm, doesn't something else (either in GTK+, Clutter, or gnome-shell) already > init atspi? I'd prefer not to have this in a constructor, though. I asked API about this, and he said it was necessary. API? Anyhow, if it is required, and if not in the constructor, then where? Note that it's safe to call Atspi.init() multiple times since it is documented as checking whether it has been initialized: https://developer.gnome.org/libatspi/stable/libatspi-atspi-misc.html#atspi-init > > @@ +43,3 @@ > + * shutDown. > + */ > + shutDown: function() { > > Does something ever call this? Actually, no -- good point. *If* the tracker is a singleton, then shutdown() should be called when gnome-shell is quit/restarted. I'm guessing that is in main.js somewhere. > +FocusCaretTracker.prototype._connect = FocusCaretTracker.prototype.connect; > +FocusCaretTracker.prototype.connect = function(name, callback) { > > This is absolutely awful. You need to figure out something better here. Why do > we want to lazily initialize focus / caret events? DBus network traffic? Make > it an explicit ref-counted thing, then. This is partly due to the fact that the tracker is a singleton and multiple clients could connect to it. The _registerX/_derigsterX methods are hidden from the clients, in order that one client doesn't deregister while another is still connected. Also, any event registration, regardless of whether it involves DBus, slows down the system. If no one is connected, why register needlessly? Registration is needed when at least one client connects. However, if you know otherwise -- that registering for atspi events is not especially taxing on the system -- then registration can done in the constructor regardless of whether any client connects. > > ::: js/ui/main.js > @@ +160,3 @@ > wm = new WindowManager.WindowManager(); > magnifier = new Magnifier.Magnifier(); > + focusCaretTracker = new FocusCaretTracker.FocusCaretTracker(); > > Does this need to be a singleton? Why can't whatever uses the FocusCaretTracker > just create one of its own? My gut says that a singleton is all that's needed. If the Atpsi experts do not see any reason to avoid multiple instances of the tracker, hence multiple AtspiEventListeners and multiple registrations, then I'm fine with multiple instantiations. mgorse, do you have any insight? Thanks again.
(In reply to comment #17) > Hm, doesn't something else (either in GTK+, Clutter, or gnome-shell) already > init atspi? I'd prefer not to have this in a constructor, though. Probably not. atspi_init() opens up a D-Bus connection and adds match rules for the purpose of listening for events and signals to update the cache. Afaik gtk+ and clutter only include libatk-bridge, which provides an application-side AT-SPI implementation. Ie, I don't think that they already have any code that listens for AT-SPI events and thus would have a reason to call atspi_init().
(In reply to comment #18) > My gut says that a singleton is all that's needed. If the Atpsi experts do not > see any reason to avoid multiple instances of the tracker, hence multiple > AtspiEventListeners and multiple registrations, then I'm fine with multiple > instantiations. mgorse, do you have any insight? It looks as though having multiple instantiations should be okay. I was worried that this might lead to event listeners being called multiple times, since libatspi doesn't currently have any code to stop it from adding the same match rule multiple times, but registering and deregistering multiple event listeners is working correctly from what I can tell, so I guess dbus-daemon knows not to deliver a signal multiple times if a connection has multiple match rules registered for it.
(In reply to comment #18) > (In reply to comment #17) > Magdalen is currently working on integrating this with the magnifier. That > work is too rough to include here yet. I'm not sure how to proceed. Should we > wait until all of it is done? There was interest in the tracker from the > onscreen keyboard developers -- see comment 1. Wait until we have at least one client that uses it in a meaningful way. We land features, not APIs. > Actually, no -- good point. *If* the tracker is a singleton, then shutdown() > should be called when gnome-shell is quit/restarted. I'm guessing that is in > main.js somewhere. Do we actually need this though? The match rules should go away when the process exits. > Also, any event registration, regardless of whether it involves DBus, slows > down the system. If no one is connected, why register needlessly? Registration > is needed when at least one client connects. > > However, if you know otherwise -- that registering for atspi events is not > especially taxing on the system -- then registration can done in the > constructor regardless of whether any client connects. If you think you want to cut down on traffic, add APIs so we add/remove the event listeners by clients directly.
how is this looking ? we're coming down to the wire for 3.9.90 Ideally, I need to have stuff merged tomorrow
Created attachment 253159 [details] [review] Tracker with Magnification
Review of attachment 253159 [details] [review]: Was this tested at all? ::: js/ui/focusCaretTracker.js @@ +34,3 @@ + _init: function() { + Atspi.init(); + this.atspiListener = Atspi.EventListener.new(Lang.bind(this, this._onChanged)); This should probably be private: this._atspiListener = ... @@ +35,3 @@ + Atspi.init(); + this.atspiListener = Atspi.EventListener.new(Lang.bind(this, this._onChanged)); + this.atspiListener._update = this; What's the point of this line? @@ +38,3 @@ + + this._caretId = this.Atspi.connect('caret-changed', Lang.bind(this, this._startCaretTracking)); + this._caretId = this; This seems a bit broken... these two lines should just be: this._caretId = this._atspiListener.connect('caret-changed', Lang.bind(this, this._startCaretTracking)); ... except that seems wrong to me. Why would we start caret tracking when the caret changes? @@ +40,3 @@ + this._caretId = this; + this._focusId = this.Atspi.connect('focus-changed', Lang.bind(this, this._startFocusTracking)); + this._focusId = this; ditto @@ +73,3 @@ + } + + _isTrackingCaret: function() { Unused. Remove. @@ +102,3 @@ +Signals.addSignalMethods(FocusCaretTracker.prototype); + +function addTrackingMethods(proto) { Seems to be unused. ::: js/ui/magnifier.js @@ +178,3 @@ + this.xMouse = xPoint; + this.yMouse = yPoint; + sysMouseOverAny = false; Seems to be mostly unrelated whitespace changes. @@ +189,3 @@ this.showSystemCursor(); } + return sysMouseOverAny; If this is an intentional change, please update the documentation. @@ +228,3 @@ */ addZoomRegion: function(zoomRegion) { + Remove whitespace. @@ +701,2 @@ Main.layoutManager.connect('monitors-changed', + Lang.bind(this, this._monitorsChanged)); Unrelated whitespace change. @@ +706,3 @@ + let extents = [-1, -1, -1, -1]; + + if (event.type.indexOf(STATECHANGED) == 0 && event.detail1 == 1) { This seems to be undefined. Was this meant to be FocusCaretTracker.STATECHANGED? @@ +794,3 @@ + + if (mode >= GDesktopEnums.MagnifierFocusTrackingMode.NONE && + mode <= GDesktopEnums.MagnifierFocusTrackingMode.PUSH) We don't tend to do validation like this, as gsettings does it for us. @@ +813,3 @@ + + if (mode >= GDesktopEnums.MagnifierCaretTrackingMode.NONE && + mode <= GDesktopEnums.MagnifierCaretTrackingMode.CENTERED) We don't tend to do validation like this, as gsettings does it for us. @@ +1041,3 @@ this._followingCursor = false; + this._changeROI({ xCenter: xPoint, + yCenter: yPoint }); Unrelated change; if intentional, please update the documentation. @@ -1077,3 @@ - // hide the magnified region from CLUTTER_PICK_ALL - Shell.util_set_hidden_from_pick (this._magView, true); - Hm, this was removed recently. Make sure you've rebased on current git master. @@ +1234,2 @@ + this._viewPortX = xPos; + this._viewPortY = yPos; Any reason for the change in variable names? @@ +1310,3 @@ + this._viewPortWidth != global.screen_width) || ( + this._viewPortHeight != global.screen_height)) + return false; I don't see the reason for the change here. @@ +1323,3 @@ return this._centerFromMousePush(xMouse, yMouse); + else if (this._mouseTrackingMode == GDesktopEnums.MagnifierMouseTrackingMode.CENTERED) + this._centerFromMouseCentered(xMouse, yMouse); You missed a "return" here. @@ +1333,3 @@ + return this._centerFromFocusProportional(xfocus, yfocus); + else if (this._focusTrackingMode == GDesktopEnums.MagnifierFocusTrackingMode.PUSH) + return this._centerFromFocusPush(xFocus, yFocus); this should be (xfocus, yfocus) @@ +1335,3 @@ + return this._centerFromFocusPush(xFocus, yFocus); + else if (this._focusTrackingMode == GDesktopEnums.MagnifierFocusTrackingMode.CENTERED) + this._centerFromCenteredPosition(xFocus, yocus); this should be (xfocus, yfocus), and you also miss the return @@ +1340,3 @@ + _centerFromCaretPosition: function() { + let xcaret = this._xPoint; + let ycaret = this._yPoint; You use this._xPoint / this._yPoint for both caret and focus. You should probably use different instance variables here. @@ +1347,3 @@ + return this._centerFromcaretPush(xcaret, ycaret); + else if (this._caretTrackingMode == GDesktopEnums.MagnifiercaretTrackingMode.CENTERED) + this._centerFromCenteredPosition(xcaret, ycaret); missing the return @@ +1353,3 @@ + _centerFromPointPush: function(xPoint, yPoint) { + let [xRoi, yRoi, widthRoi, heightRoi] = (0,0,0,0); + this.getROI(); This needs to be the same as before: let [xRoi, yRoi, widthRoi, heightRoi] = this.getROI(); why was it changed? @@ +1354,3 @@ + let [xRoi, yRoi, widthRoi, heightRoi] = (0,0,0,0); + this.getROI(); + let [cursorWidth, cursorHeight] = (0,0); This line is unnecessary as the variables are reassigned below. @@ +1359,3 @@ + let yPos = 0 + xPos = xRoi + widthRoi / 2; + yPos = yRoi + heightRoi / 2; Why are these on separate lines than the let?
Review of attachment 253159 [details] [review]: Part I: review of the focusCaretTracker.js code. First a prior question: is this the correct file? We discussed an earlier version of this code, and I thought you were going to make changes. See below for specifics. ::: js/ui/focusCaretTracker.js @@ +34,3 @@ + _init: function() { + Atspi.init(); + this.atspiListener = Atspi.EventListener.new(Lang.bind(this, this._onChanged)); This was one of the things we had discussed: to make atspiListener private (as Jasper suggests in his review). @@ +35,3 @@ + Atspi.init(); + this.atspiListener = Atspi.EventListener.new(Lang.bind(this, this._onChanged)); + this.atspiListener._update = this; This was another point of discussion. This adds an "_update" property to the atspiListener object. The value of that property is the current tracker instance. But, I don't see where that is used. What is the purpose? @@ +37,3 @@ + this.atspiListener._update = this; + + this._caretId = this.Atspi.connect('caret-changed', Lang.bind(this, this._startCaretTracking)); As far as I can tell, 'this' (the tracker instance) does not have an Atspi property. What object is "this.Atspi"? @@ +38,3 @@ + + this._caretId = this.Atspi.connect('caret-changed', Lang.bind(this, this._startCaretTracking)); + this._caretId = this; After assigning this._caretId the connection id returned on line 39, it is immediately overwritten with a reference to the current tracker instance. At least, that is what I see happening here. I don't understand the rationale. @@ +50,3 @@ + else if (event.type == CARETMOVED) + update = 'caret-moved'; + this.emit(update, event); Spacing: "this.emit(update, event);" is indented once too far. Also, in the spirit of "I say tomayto, you say tomahto", I would not use the local 'update' variable. I would do: if (event.type.indexOf(STATECHANGED) == 0) this.emit('focus-changed'); else this.emit('caret-moved'); But, I won't fall on my sword for that; it's just a suggestion. @@ +75,3 @@ + _isTrackingCaret: function() { + return _caretId; + } I don't understand the purpose of these various _startTracking/_stopTracking methods. Can you explain?
Review of attachment 253159 [details] [review]: ::: js/ui/magnifier.js @@ +177,1 @@ + if (xPoint != this.xPoint || yPoint != this.yPoint) { Undeclared xPoint and yPoint. Also, I can't find where this.xPoint and this.yPoint are defined or initialized. They were formerly this.xMouse and this.yMouse. Is this a typo? The old logic was: - get the current position of the mouse. - if the current position is not different from the last recorded position (the mouse did not move): -- don't do anything and return true. - if the current mouse position is different from recorded (the mouse moved), then: -- update the recorded position, -- inform each zoomregion of the new mouse position, -- check to see if the mouse pointer needs to be shown/hidden (hide if over a zoom region, show otherwise), and return true. The new logic is: - immediately update the recorded position of the mouse with its current position. - check ??? I can't tell what is being checked here. @@ +186,1 @@ if (sysMouseOverAny) sysMouseOverAny is initialized to 'true' at the top of the function. There is no place where it is set to false before this 'if (sysMouseOverAny)' is reached. Consequently, this.hideSystemCursor() on the next line will always be called, and users will no longer see their mouse pointer. The old version initialized sysMouseOverAny to 'false', so it could be false when the mouse was not over a zoom region, but set to true when it was. @@ +189,3 @@ this.showSystemCursor(); } + return sysMouseOverAny; scrollToMousePos() is called periodically using Mainloop.timeout() (via the PointerWatcher object -- see line 144 above). The way periodic functions work is that they return 'true' if they need to be called again, and 'false' when they are done. Specifically, if scrollToMousePos() ever returns 'false', it will be removed from these periodic calls, and cease to be called at all. That is why scrollToMousePos() must always return 'true', and should not be dependent on the value of sysMouseOverAny. @@ +234,3 @@ + //let mouse be default + if (!this.isTrackingMouse() && !this.isTrackingFocus() && !this.isTrackingCaret()) + this.startTrackingMouse(); Mouse tracking is independent of focus and caret tracking. Whenever a zoomRegion is added, mouse tracking needs to start, if not already in effect. Otherwise, various things that depend on the position of the mouse won't be updated appropriately. @@ +703,3 @@ + }, + + caretTrackerPos: function(caller, event) { I can't find where caretTrackerPos() is used. Maybe a typo? That is, is it called using another name? @@ +807,3 @@ + + /** + * setFocusTrackingMode Typo: "setCaretTrackingMode". @@ +1290,3 @@ }, + _isOverRegion: function() { The change in name makes this less meaningful: Is what over the region? Also, the places that called the function by the old name, _isMouseOverRegion, have not been updated to the new name. Finally, I don't see what these changes have to do with focus and/or caret tracking. @@ +1292,3 @@ + _isOverRegion: function() { + if (!this.isActive()) + return; Need to return a boolean value. @@ +1321,1 @@ return this._centerFromMouseProportional(xMouse, yMouse); You mean _centerFromPointProportional. Typo? @@ +1323,1 @@ return this._centerFromMousePush(xMouse, yMouse); _centerFromPointPush. Typo? @@ +1323,3 @@ return this._centerFromMousePush(xMouse, yMouse); + else if (this._mouseTrackingMode == GDesktopEnums.MagnifierMouseTrackingMode.CENTERED) + this._centerFromMouseCentered(xMouse, yMouse); __centerFromPointCentered. Typo? @@ +1331,3 @@ + + if (this._focusTrackingMode == GDesktopEnums.MagnifierFocusTrackingMode.PROPORTIONAL) + return this._centerFromFocusProportional(xfocus, yfocus); _centerFromPointProportional @@ +1333,3 @@ + return this._centerFromFocusProportional(xfocus, yfocus); + else if (this._focusTrackingMode == GDesktopEnums.MagnifierFocusTrackingMode.PUSH) + return this._centerFromFocusPush(xFocus, yFocus); _centerFromPointPush @@ +1335,3 @@ + return this._centerFromFocusPush(xFocus, yFocus); + else if (this._focusTrackingMode == GDesktopEnums.MagnifierFocusTrackingMode.CENTERED) + this._centerFromCenteredPosition(xFocus, yocus); _centerFromPointCentered @@ +1343,3 @@ + + if (this._caretTrackingMode == GDesktopEnums.MagnifiercaretTrackingMode.PROPORTIONAL) + return this._centerFromcaretProportional(xcaret, ycaret); _centerFromPointProportional @@ +1345,3 @@ + return this._centerFromcaretProportional(xcaret, ycaret); + else if (this._caretTrackingMode == GDesktopEnums.MagnifiercaretTrackingMode.PUSH) + return this._centerFromcaretPush(xcaret, ycaret); _centerFromPointPush @@ +1347,3 @@ + return this._centerFromcaretPush(xcaret, ycaret); + else if (this._caretTrackingMode == GDesktopEnums.MagnifiercaretTrackingMode.CENTERED) + this._centerFromCenteredPosition(xcaret, ycaret); _centerFromPointCentered @@ +1397,3 @@ }, _screenToViewPort: function(screenX, screenY) { I do not see where the FocusCaretTracker is used in the zoom region code. That is, no instance of a tracker is created, and no connect() calls to that tracker instance is made to handle any 'focus-changed' and/or 'caret-changed' signals.
Created attachment 253419 [details] [review] updated Magnifier+Tracker Changes It has been hard testing due to my ongoing computer troubles Thanks for the comments the main problem is that I am not sure how to call connect in the ZoomRegions because the _onChanged is already done in the tracker and I am not sure what to substitute that with inside the magnifier without blatantly just repeating code from focusCaretTracker.js Please take a look at how it has been done and let me know. Methods Explained: focusId caretId These would get an integer to confirm the 'connected status' of the focus or caret tracking which could be used by isTrackingFocus isTrackingCaret to determine the state of play and return a boolean value. This can be used by the ZoomRegions of the Magnifier to decide whether or not to try and get the x and y coordinates of the caret/focus. Theoretically you could use the register/deregister methods for this but it would mean calling deregister and register from Signals and that would not be ideal (especially if they were already connected, for example) The following methods startFocusTracking startCaretTracking were a bit of an experiment. I wanted to see if calling each of them as a delegate of _atspiListener.connect from inside magnifier would affectively call their respective connect methods that I had inside the focusCaretTracker. I did not upload the magnifier code where they were explicitly called from and there were a few different ways I went about it so, sorry about the confusion! I can only say that I was really tired when I did the patch and as Joseph made sure to point out already, the result was that there were a fair few unrelated typos in the previous code. However the magnifier.js source code from master does have some errors that have to be fixed (or at least that is what gjs seems to think anyway) JS ERROR: !!! WARNING: 'anonymous function does not always return a value' JS ERROR: !!! WARNING: file '/home/magpie/gnome/gnome-shell/js/ui/magnifier.js' line 1328 exception 0 number 157 JS ERROR: !!! WARNING: 'anonymous function does not always return a value' JS ERROR: !!! WARNING: file '/home/magpie/gnome/gnome-shell/js/ui/magnifier.js' line 1354 exception 0 number 157 I am not sure whether to include fixes in the patch or do a separate bug to deal with it I did fix a typo in the getContrast method in this patch though as I thought I might forget that one but let me know if that needs a bug report. I have got rid of the prototyping and these methods altogether for now, at least. Generally I am struggling to see the advantage of focusCaretTracker.js as a separate file if all the 'logic' needs to be handled inside the magnifier anyway which is why I figured it would be worth giving the magnifier as little as is possible to do for the f/c tracking related work but how to connect to the tracker from the magnifier has been the main area of confusion for me. If I cannot get around that, I think the only solution is to make the tracking from inside the magnifier and scrap focusCaretTracker.js altogether so advice welcome. Thanks in advance.
(In reply to comment #27) > Created an attachment (id=253419) [details] [review] > ... > Thanks for the comments the main problem is that I am not sure how to call > connect in the ZoomRegions because the _onChanged is already done in the > tracker and I am not sure what to substitute that with inside the magnifier > without blatantly just repeating code from focusCaretTracker.js I sent you an example in an email sometime near mid- to late- July. I'll try to find that old email. In any case, I think we should take the discussion out of the bugzilla for the time being, and iron out the problems before submitting another patch. > Thanks in advance. You're welcome. Thanks for plugging away on this.
(In reply to comment #27) > Created an attachment (id=253419) [details] [review] > updated Magnifier+Tracker Changes > > It has been hard testing due to my ongoing computer troubles FWIW. Magdalen, are you sure that this is the correct patch? I didn't review it, but went directly to try to test it. Doing that I get this error trying to launch gnome-shell: (gnome-shell:1302): Gjs-WARNING **: JS ERROR: Exception in callback for signal: sessions-loaded: TypeError: this.focusCaretTracker is undefined ZoomRegion<._init@/opt/gnome3/share/gnome-shell/js/ui/magnifier.js:684 wrapper@/opt/gnome3/share/gjs-1.0/lang.js:213 _Base._construct@/opt/gnome3/share/gjs-1.0/lang.js:154 Class._construct/newClass@/opt/gnome3/share/gjs-1.0/lang.js:248 Magnifier<._init@/opt/gnome3/share/gnome-shell/js/ui/magnifier.js:72 wrapper@/opt/gnome3/share/gjs-1.0/lang.js:213 _Base._construct@/opt/gnome3/share/gjs-1.0/lang.js:154 Class._construct/newClass@/opt/gnome3/share/gjs-1.0/lang.js:248 _initializeUI@/opt/gnome3/share/gnome-shell/js/ui/main.js:165 _sessionsLoaded@/opt/gnome3/share/gnome-shell/js/ui/main.js:118 _emit@/opt/gnome3/share/gjs-1.0/signals.js:124 SessionMode<.init/<@/opt/gnome3/share/gnome-shell/js/ui/sessionMode.js:161 done@/opt/gnome3/share/gnome-shell/js/misc/fileUtils.js:33 @/opt/gnome3/share/gnome-shell/js/misc/fileUtils.js:43
(In reply to comment #28) > (In reply to comment #27) > > Created an attachment (id=253419) [details] [review] [details] [review] > > ... > > Thanks for the comments the main problem is that I am not sure how to call > > connect in the ZoomRegions because the _onChanged is already done in the > > tracker and I am not sure what to substitute that with inside the magnifier > > without blatantly just repeating code from focusCaretTracker.js > > I sent you an example in an email sometime near mid- to late- July. I'll try > to find that old email. Thanks but that old email has an example of connection made in the magnifier not the zoomRegions but I seem to have that resolved now. Full patch coming up.
Created attachment 253588 [details] [review] Magnifier Focus/Caret Tracking Fully Tested and Working. I have tested this patch and believe this patch to be fully working. However the patch does depend on Bug 707010 I have been able to resume testing the focus/caret magnifier work since applying patch https://bug707010.bugzilla-attachments.gnome.org/attachment.cgi?id=253500 from the bug so please make sure to apply patch 253500 to at-spi-core if you wish to test this work too. Joseph Scheuhammer, and I have been in discussion and he recommended some specific checks I could make for the next patch (I have paraphrased a bit as the suggestions were made in IRC, but hopefully I have not missed anything out) so I tried the following before uploading this work: Test A: Get an app that supports focus navigation via the TAB key and use the TAB and arrow key presses to move focus around that app. Result A: Using the accessibility settings manager it was possible to demonstrate that the magnified view zoomed onto the focused objects as expected. Test B: Use a terminal to change the tracking gsettings between centered, push, proportional and for each mode, check the mode of focus tracking appropriately changes. Result B: Okay Test C: Find an app that exposes caret motion through the a11y API and type to check that the magnified view tracks the movement of the caret in the document. Result C: Using the text editor gedit it was possible to demonstrate that the magnified view tracked the movement of the caret across the text field of the document, as expected. Test D: Use a terminal to change the tracking gsettings between centered, push, proportional and for each mode, check the mode of focus tracking appropriately changes. Result D: Okay Test E: Do the Cross hairs of the Mouse still work as they did before? Result E: Yes Test F: Does the contrast setting still work as before? Result F: Yes
(In reply to comment #31) > Created an attachment (id=253588) [details] [review] > Magnifier Focus/Caret Tracking Fully Tested and Working. Just made a quick test. Will continue on the afternoon. First comments: > > Test D: Use a terminal to change the tracking gsettings between centered, push, > proportional and for each mode, check the mode of focus tracking appropriately > changes. Include 'none' here please. AFAIU, if you set the tracking to none on both cases, you should get a experience similar to what he had without the patch. In my tests, although I set the value to 'none' the magnifier is tracking both caret and focus changes. > Result D: Okay See my previous comment. Additionally, using this patch I have some problems moving the zoom region with the mouse. This is the case: 0. Zoom activated 1. Open gedit 2. Select gedit 2.1 As focus change, gedit is centered => Good 3. Start to write 3.1 If new text gets out of centered region, it is centered => Good But if I stop to write (so not caret events) or move to a different widget (so not focus changes), and I try to move with the mouse the zoom region, it gets stuck. It seems that it is affecting the mouse tracking somehow. As I said, I will make further review this afternoon. ************************** And a nitpick on a comment: + // FIXME that select events have been included in the logic for focus events + // only because objects will lose focus the moment they are selected. + registerFocusListener: function() { + return this._atspiListener.register(STATECHANGED + ':focused') || this._atspiListener.register( + STATECHANGED + ':selected'); That FIXME is not accurate, imho. The reason to also track selected changes is because in some cases, although the user sees a "focus change", what is happening is a selection change on a container. One example is the alt-tab switcher. The focus is on the switcher (container) but the tab changes the selected object. That is an example, there are another cases. So that FIXME is not in fact a FIXME (as will not be fixed in the future). In any case, explaining it would be ok.
Review of attachment 253588 [details] [review]: ::: js/ui/magnifier.js @@ +684,3 @@ this._uiGroupClone = null; this._mouseSourceActor = mouseSourceActor; + this._actor = null; This is going to sound nitpicky, but it isn't really. When looking at the changes below, I was confused by the new name "_actor" -- which actor? The ZoomRegion manages a number of actors, and the name "_mouseActor" clearly indicates the relevant one. @@ +1040,3 @@ this._updateMousePosition(); + } + if (this._caretTrackingMode != GDesktopEnums.MagnifierCaretTrackingMode.NONE && Formerly scrollToMousePos(), this is a function specifically for updating the view based on mouse movements, and is part of the machinery for updates related to changes in mouse position. Focus and/or caret changes are independent of changes in mouse position. Adding these checks partially explains why API is seeing that "It seems that it is affecting the mouse tracking somehow." Removing the focus/caret tracker checks will help fix that. You are already using scrollContentsTo() to update the view based on focus/caret changes. Also, change the name back to scrollToMousePos() after removing these checks since that is all the function is concerned with. @@ +1340,2 @@ } + return mouseIsOver || pointIsOver; Formerly _isMouseOverRegion(), this is for detecting when the system mouse pointer is over the magnified view. The return statement is an OR, meaning the return value will sometimes be 'true' when the mouse is *not* over the magnified view. Callers who are using _isPointOverRegion() to determine if the mouse pointer is over the magnified view no longer receive reliable information. The callers are: setActive(), scrollToPos(), and _setViewPort(). In each case they are calling _isPointOverRegion() only to determine if the mouse pointer is over the magnified view. Recommend that the new code be removed, and this reverts back to _isMouseOverRegion(). @@ +1408,3 @@ + let yProportion = (yPoint - halfScreenHeight) / halfScreenHeight; // -1 ... 1 + return [this.xPoint - xProportion * (widthRoi / 2 - xPadding), + this.yPoint - yProportion * (heightRoi /2 - yPadding)]; Likely a typo: the arguments to the function are xPoint and yPoint, and are used throughout the calculation except at the last step, where 'this.xPoint' and 'this.yPoint' are used. Change the return statement to: return [xPoint - xProportion * (widthRoi / 2 - xPadding), yPoint - yPorportion * (heightRoi / 2 - yPadding)]; @@ +1475,3 @@ + xMagPoint = Math.round(xMagPoint); + yMagPoint = Math.round(yMagPoint); + this._actor.set_position(xMagPoint, yMagPoint); this._actor is the magnified mouse pointer image. Its position shouldn't be set to the current focus/caret position. The location of the magnified mouse pointer within the magnified view should always be associated with the relative location of the system mouse pointer on the desktop.
Created attachment 253719 [details] [review] Solved issues and made some other improvements Fixes: 1. 'Mouse getting stuck' with creation of centerFromFocusCentered and centerFromCaretCentered functions. 2. Removed pointless 'if' stament this._mouseTrackingMode != GDesktopEnums.MagnifierMouseTrackingMode.NONE. This check is is already done by scrollToPos before it calls _changeROI (where the if statement was made) 3. Got rid of 'FIXME' in focusCaretTracker although focus and select cannot be called simultaneously, this is not going to be fixed. Improvements: 1. Made it possible to change the setting to something other than 'none' afterwards and focus/caret tracking will resume. 2. Removed use of Clutter _actor object for focus and caret. 3. Created simplified updateFocusPosition and updateCaretPosition functions and removed updateCaretFocusPosition. Also, tidied it up a bit by making it more concise. The motivation behind this commit is to get a working tracker for the magnifier pushed to master (hopefully in time for 3.9.91) so that people can use it right away once this has been achieved I will be looking for feedback on how to further improve and optimise the features seen here. Thanks in advance.
Created attachment 253723 [details] [review] Focus and Caret Tracking for the Magnifier Spotted unintentional change so fixed that rechecked the following and got passes for each one cross hairs colour gsettings (none, centered, push and proportional) Thanks in advance
Review of attachment 253723 [details] [review]: ::: js/ui/focusCaretTracker.js @@ +29,3 @@ +const STATECHANGED = 'object:state-changed'; + +const FocusCaretTracker = new Lang.Class({ I think for now, I'd prefer to see this as just a helper class at the top of magnifier.js. ::: js/ui/magnifier.js @@ +716,3 @@ + _updateFocus: function(caller, event) { + let accessibleFocus = event.source; + let extents = this.extents; This won't do what you think it will. This means that it will keep the old extents when scrolling the screen below, and it means that if for some reason we get called with an invalid event, we'll scroll to some old or invalid location. Instead of making this an instance variable, just make it a local. @@ +718,3 @@ + let extents = this.extents; + + if (accessibleFocus && event.type.indexOf('object:state-changed') == 0 && event.detail1 == 1) { Why is this indexOf instead of == ? And won't the focus tracker already check the event type? Do we ever expect accessibleFocus to be false, or detail1 to not be 1? If so, what's supposed to happen? @@ +723,3 @@ + } + this.xFocus = extents.x; + this.yFocus = extents.y; In the constructor, you set extents to an array, but it seems get_extents returns an AtspiRect? Meaning this will throw an error the first time around. You should probably just exit out early if you don't have an accessibleFocus or if the detail is wrong. @@ +727,3 @@ + }, + + _updateCaret: function(caller, event) { ditto for this method @@ +815,3 @@ + * setFocusTrackingMode + * @mode: One of the enum FocusTrackingMode values. + */ I'd say these comments are useless, but I'm fine with them landing. @@ +1048,3 @@ + if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.NONE) { + this.focusCaretTracker.deregisterCaretListener(); scrollToPos seems to me to be not the greatest place to register/unregister the listeners. Maybe setCaretTrackingMode / setFocusTrackingMode might be better? @@ +1050,3 @@ + this.focusCaretTracker.deregisterCaretListener(); + } + else { Bad indentation style, we tend to use: } else { @@ +1927,3 @@ */ getContrast: function() { + let result = {}; This is an unrelated typo fix. But I don't see these methods actually used anywhere. I'd prefer to just have a followup commit that removes it entirely.
(In reply to comment #36) > Review of attachment 253723 [details] [review]: > Do we ever expect accessibleFocus to be false, or detail1 to not be 1? If so, > what's supposed to happen? detail1 is 1 for a focused accessible and 0 if the focus has been lost so that needs to be there but I am making a change with respect to this line that Jasper has suggested in IRC. > ::: js/ui/focusCaretTracker.js > @@ +29,3 @@ > +const STATECHANGED = 'object:state-changed'; > + > +const FocusCaretTracker = new Lang.Class({ > > I think for now, I'd prefer to see this as just a helper class at the top of > magnifier.js. About this: I would like to continue to keep the tracker as it is: A namespace written in a separate file to the magnifier. My reasoning: a) There is nothing else handling focus caret tracking in GNOME Shell right now. b) You requested a use case for the tracking. That is what the magnifier is doing here. c) It difficult to untangle the code from the magnifier later on for when something else wants to use it and could lead to repeated bits of code in GNOME Shell. How fixed is your mind on this? In order to proceed it would help to know one way or another what I need to do about this one. Is it a must? Everything else should be fine to change right away.
Oh and thanks!
Oh and one more thing... indexOf is used for the reason explained in the tracker. I mean this bit: // Select events have been included in the logic for focus events // only because objects will lose focus the moment they are selected. Do you want me to put that in the code where you asked about it too? Thanks
Created attachment 253730 [details] [review] Focus and Caret Tracking for the Magnifier I have repeated checks after making the suggested changes.
Review of attachment 253730 [details] [review]: Very close to landing at this point. The one major thing I'd want you to think about at this point is a better commit message. Something like: magnifier: Implement focus and caret tracking A11y users who use the magnifier may have trouble focusing when they're typing or trying to keynav. Implement a new system so that they can have the magnifier track the caret and focus instead instead of just the mouse. ::: js/ui/magnifier.js @@ +713,3 @@ + _updateFocus: function(caller, event) { + let accessibleFocus = event.source; + let extents = [-1 , -1, -1, -1]; I'd just remove this line and do: let extents = component.get_extents(...); inside the block below. @@ +714,3 @@ + let accessibleFocus = event.source; + let extents = [-1 , -1, -1, -1]; + let component = accessibleFocus.get_component_iface(); You can put this and the "let accessibleFocus" line inside the if statement. @@ +727,3 @@ + let accessibleCaret = event.source; + let extents = [-1 , -1, -1, -1]; + let text = accessibleCaret.get_text_iface(); same @@ +1437,3 @@ + _centerFromPointCentered: function(xPoint, yPoint) { + return [xPoint, yPoint]; I guess I don't understand the point of this method, but OK. @@ +1495,3 @@ + let [xMagCaret, yMagCaret] = this._screenToViewPort(this.xCaret, this.yCaret); + this.xCaret = Math.round(xMagCaret); + this.yCaret = Math.round(yMagCaret); I'm not exactly sure how this is architected (clown would have to weigh in here), but is screenToViewPort supposed to be idempotent? Taking a member variable, applying some sort of transform, and then spitting it back out into the same member variable seems wrong to me.
Created attachment 253732 [details] [review] magnifier: Implement focus and caret tracking made suggested changes
Review of attachment 253732 [details] [review]: After these changes, this should be ready to land, assuming clown or API think it's fine as well. ::: js/ui/magnifier.js @@ +712,3 @@ + + _updateFocus: function(caller, event) { + extra whitespace @@ +715,3 @@ + if (event.detail1 == 1) { + let accessibleFocus = event.source; + let extents = [-1 , -1, -1, -1]; extents gets overwritten below, so just use 'let extents = component.get_extents(Atspi.CoordType.SCREEN);' @@ +725,3 @@ + + _updateCaret: function(caller, event) { + extra whitespace @@ +726,3 @@ + _updateCaret: function(caller, event) { + + if (text.get_caret_offset() >= 0) { 'text' here is undefined. You need to move the 'let text' and 'let accessibleCaret' below back out; sorry. @@ +813,3 @@ + */ + setFocusTrackingMode: function(mode) { + extra whitespace @@ +834,3 @@ + */ + setCaretTrackingMode: function(mode) { + extra whitespace
Created attachment 253733 [details] [review] Magnifier: implement focus and caret tracking (In reply to comment #41) > @@ +1495,3 @@ > + let [xMagCaret, yMagCaret] = this._screenToViewPort(this.xCaret, > this.yCaret); > + this.xCaret = Math.round(xMagCaret); > + this.yCaret = Math.round(yMagCaret); > > I'm not exactly sure how this is architected (clown would have to weigh in > here), but is screenToViewPort supposed to be idempotent? Taking a member > variable, applying some sort of transform, and then spitting it back out into > the same member variable seems wrong to me. Thank you for pointing that out. The _updateCaretPosition and _updateFocusPosition have reduced to + _updateCaretPosition: function() { + this._screenToViewPort(this.xCaret, this.yCaret); + }, + + _updateFocusPosition: function() { + this._screenToViewPort(this.xFocus, this.yFocus); + }, respectively, and has nothing broke: Good.
(In reply to comment #44) > Thank you for pointing that out. > > The _updateCaretPosition and _updateFocusPosition have reduced to > > + _updateCaretPosition: function() { > + this._screenToViewPort(this.xCaret, this.yCaret); > + }, > + > + _updateFocusPosition: function() { > + this._screenToViewPort(this.xFocus, this.yFocus); > + }, > > respectively, and has nothing broke: Good. That doesn't make sense. screenToViewPort doesn't do anything other than return a transformed value. Have you tried with other magnifier settings like half-screen, and are you sure you rebuilt gnome-shell after every single JS change?
Yep.(In reply to comment #45) > (In reply to comment #44) > > Thank you for pointing that out. > > > > The _updateCaretPosition and _updateFocusPosition have reduced to > > > > + _updateCaretPosition: function() { > > + this._screenToViewPort(this.xCaret, this.yCaret); > > + }, > > + > > + _updateFocusPosition: function() { > > + this._screenToViewPort(this.xFocus, this.yFocus); > > + }, > > > > respectively, and has nothing broke: Good. > > That doesn't make sense. screenToViewPort doesn't do anything other than return > a transformed value. Have you tried with other magnifier settings like > half-screen, and are you sure you rebuilt gnome-shell after every single JS > change? Yes. Another good point: I think it is probably safe to remove these two functions but I will test that before uploading, just in case.
Created attachment 253760 [details] [review] Magnifier: Implement focus and caret tracking Those two functions were not needed after all. Checked: Cross hairs, Various views Switching between gsettings Contrast
Review of attachment 253760 [details] [review]: ::: js/ui/magnifier.js @@ +714,3 @@ + if (event.detail1 == 1) { + let accessibleFocus = event.source; + let extents = [-1 , -1, -1, -1]; You still need to fix this one. @@ +729,3 @@ + if (text.get_caret_offset() >= 0) { + let extents = [-1 , -1, -1, -1]; + extents = text.get_character_extents(text.get_caret_offset(), 0); ... and this one. @@ +749,3 @@ this._updateMousePosition(); + } + else if (!activate && this.isActive()) { changed style for no reason @@ +1040,3 @@ * @return: Whether the system mouse pointer is over the magnified view. */ + scrollToPos: function() { Is there any reason you changed this method name? @@ +1043,2 @@ this._followingCursor = true; + ... and introduced this whitespace? @@ -935,3 @@ this._updateMousePosition(); - - // Determine whether the system mouse pointer is over this zoom region. Why was this comment removed? @@ +1364,3 @@ }, + _centerFromCaretPosition: function() { This method doesn't seem to be used? @@ +1376,3 @@ + }, + + _centerFromFocusPosition: function() { This method doesn't seem to be used?
(In reply to comment #48) > Review of attachment 253760 [details] [review]: > > ::: js/ui/magnifier.js > @@ +714,3 @@ > + if (event.detail1 == 1) { > + let accessibleFocus = event.source; > + let extents = [-1 , -1, -1, -1]; > > You still need to fix this one. > > @@ +729,3 @@ > + if (text.get_caret_offset() >= 0) { > + let extents = [-1 , -1, -1, -1]; > + extents = text.get_character_extents(text.get_caret_offset(), 0); > > ... and this one. I removed the newline and now really cannot see what whitespace I should be seeing there at all. Can you double check? Everything else should be fine.
Created attachment 253765 [details] [review] Magnifier: Implement focus and caret tracking
Review of attachment 253765 [details] [review]: ::: js/ui/magnifier.js @@ +715,3 @@ + return; + let accessibleFocus = event.source; + let extents = [-1 , -1, -1, -1]; I mean that you overwrite the extents variable on the next line, so you should just do: let extents = component.get_extents(Atspi.CoordType.SCREEN); @@ +812,3 @@ + else + this.focusCaretTracker.registerFocusListener(); + this._focusTrackingMode = mode; You don't ever use this._focusTrackingMode. @@ +832,3 @@ + else + this.focusCaretTracker.registerCaretListener(); + this._caretTrackingMode = mode; You don't ever use this._caretTrackingMode.
Created attachment 253776 [details] [review] Magnifier: implement focus and caret tracking
Review of attachment 253776 [details] [review]: Just finished a more detailed test of the patch, mostly focused on the functionality/crashing/errors. I let the code style review and similars to Jasper. Most of the stuff that I found are nitpicks or things really easy to solve (like an && instead of an ||), except one question about the tracking modes. See that question on the code review. Setting as needs-work because the && vs || is important, and needs to be fixed for sure. Detailed review below: ::: js/ui/focusCaretTracker.js @@ +48,3 @@ + + // Select events have been included in the logic for focus events + // only because objects will lose focus the moment they are selected. On comment 32 I mentioned to remove the FIXME, but also to update this comment. Selection events are not tracked because of a lose of focus. Selection events are tracked because in several cases, what users see as a focus change, it is in fact a selection change. On the following examples, the event that you get is an state-changed:selected * On gedit, if you press Alt+F to select the File menu, and start to move down * On gnome-shell, if you press alt+tab to show the app switcher, and then tab to move on the elements (the same for ctrl+alt+tab) * On gnome-shell, if you start to write on the overview, so an element of the search is selected, and each time you press tab So, a more correct comment would be something like: //state-changed:selected events have been included in the logic for focus events because in several cases, what the user see as a focus event is in fact a selection change, ie: navigate through menuitems inside a menu But with proper english. @@ +51,3 @@ + registerFocusListener: function() { + return this._atspiListener.register(STATECHANGED + ':focused') || this._atspiListener.register( + STATECHANGED + ':selected'); That needs to be an && instead of ||. Using || provokes that only focused evets are registered, and you need to listen to both events. That seems to be a typo, as on the deregisterFocusListener you use the correct &&. FWIW, this is really needed. Without this change *all* the examples on my previous comment fails. @@ +59,3 @@ + + // Select events have been included in the logic for focus events + // only because objects will lose focus the moment they are selected. See my previous comment. ::: js/ui/magnifier.js @@ +51,2 @@ let magDBusService = null; +let focusCaretTracker = null; The focusCaretTracker that you use is created at ZoomRegion, and used as an element of the tracker (this.focusCaretTracker). This global focusCaretTracker is not used at all, so useless. @@ +668,3 @@ _init: function(magnifier, mouseSourceActor) { this._magnifier = magnifier; + this.focusCaretTracker = new FocusCaretTracker.FocusCaretTracker(); This should be private (so, this._focusCaretTracker) @@ +672,3 @@ this._mouseTrackingMode = GDesktopEnums.MagnifierMouseTrackingMode.NONE; + this._focusTrackingMode = GDesktopEnums.MagnifierFocusTrackingMode.NONE; + this._caretTrackingMode = GDesktopEnums.MagnifierCaretTrackingMode.NONE; As Jasper mentions on comment 51, the value of the modes are not checked at all. 'none' works because when the mode is set, the atspi listeners are registered/deregistered. That means that right now there are two practical modes, tracking and non tracking, and that push/centered/proportional [caret/focus]TrackingModes are all the same. For this reason I don't understand what you say on comment 31: "Test B: Use a terminal to change the tracking gsettings between centered, push, proportional and for each mode, check the mode of focus tracking appropriately changes. Result B: Okay" I don't understand the "Okay". AFAIS, all the non-none modes work the same. Could you ellaborate this? (just in case I missed some of your conversations with Jasper or Joseph on IRC) @@ +717,3 @@ + let extents = [-1 , -1, -1, -1]; + let component = accessibleFocus.get_component_iface(); + extents = component.get_extents(Atspi.CoordType.SCREEN); Sometimes I get the following error: (gnome-shell:9434): Gjs-WARNING **: JS ERROR: Exception in callback for signal: focus-changed: TypeError: component is null ZoomRegion<._updateFocus@/opt/gnome3/share/gnome-shell/js/ui/magnifier.js:719 wrapper@/opt/gnome3/share/gjs-1.0/lang.js:213 _emit@/opt/gnome3/share/gjs-1.0/signals.js:124 FocusCaretTracker<._onChanged@/opt/gnome3/share/gnome-shell/js/ui/focusCaretTracker.js:46 wrapper@/opt/gnome3/share/gjs-1.0/lang.js:213 So that means that sometimes the object focused/selected doesn't implement component. Probably that are invalid objects (like object just to be deceased), and it would be good to avoid to focus on them. But in any case, means that there are a possibility of receiving an object that doesn't implement that interface. So I will add a null check before calling extents. Something like the null check that you do on _updateCaret (that case in relation with the text interface).
(In reply to comment #54) > Review of attachment 253776 [details] [review]: > > Just finished a more detailed test of the patch, mostly focused on the > functionality/crashing/errors. I let the code style review and similars to > Jasper. > > Most of the stuff that I found are nitpicks or things really easy to solve > (like an && instead of an ||), except one question about the tracking modes. > See that question on the code review. > > Setting as needs-work because the && vs || is important, and needs to be fixed > for sure. > > Detailed review below: > > ::: js/ui/focusCaretTracker.js > @@ +48,3 @@ > + > + // Select events have been included in the logic for focus events > + // only because objects will lose focus the moment they are selected. > > On comment 32 I mentioned to remove the FIXME, but also to update this comment. > Selection events are not tracked because of a lose of focus. Selection events > are tracked because in several cases, what users see as a focus change, it is > in fact a selection change. On the following examples, the event that you get > is an state-changed:selected > * On gedit, if you press Alt+F to select the File menu, and start to move > down > * On gnome-shell, if you press alt+tab to show the app switcher, and then tab > to move on the elements (the same for ctrl+alt+tab) > * On gnome-shell, if you start to write on the overview, so an element of the > search is selected, and each time you press tab > > So, a more correct comment would be something like: > //state-changed:selected events have been included in the logic for focus > events because in several cases, what the user see as a focus event is in fact > a selection change, ie: navigate through menuitems inside a menu > > But with proper english. That is what I said and it was based on what is in the X documentation > @@ +51,3 @@ > + registerFocusListener: function() { > + return this._atspiListener.register(STATECHANGED + ':focused') || > this._atspiListener.register( > + STATECHANGED + ':selected'); > > That needs to be an && instead of ||. Using || provokes that only focused evets No I don't think so. It needs to be true if either events are recognised it is not possible to have both a focus and select event so that change would mean the result was always false. > are registered, and you need to listen to both events. That seems to be a typo, > as on the deregisterFocusListener you use the correct &&. > > FWIW, this is really needed. Without this change *all* the examples on my > previous comment fails. > > @@ +59,3 @@ > + > + // Select events have been included in the logic for focus events > + // only because objects will lose focus the moment they are selected. > > See my previous comment. > > ::: js/ui/magnifier.js > @@ +51,2 @@ > let magDBusService = null; > +let focusCaretTracker = null; > > The focusCaretTracker that you use is created at ZoomRegion, and used as an > element of the tracker (this.focusCaretTracker). This global focusCaretTracker > is not used at all, so useless. > > @@ +668,3 @@ > _init: function(magnifier, mouseSourceActor) { > this._magnifier = magnifier; > + this.focusCaretTracker = new FocusCaretTracker.FocusCaretTracker(); > > This should be private (so, this._focusCaretTracker) > > @@ +672,3 @@ > this._mouseTrackingMode = > GDesktopEnums.MagnifierMouseTrackingMode.NONE; > + this._focusTrackingMode = > GDesktopEnums.MagnifierFocusTrackingMode.NONE; > + this._caretTrackingMode = > GDesktopEnums.MagnifierCaretTrackingMode.NONE; > > As Jasper mentions on comment 51, the value of the modes are not checked at > all. 'none' works because when the mode is set, the atspi listeners are > registered/deregistered. > > That means that right now there are two practical modes, tracking and non > tracking, and that push/centered/proportional [caret/focus]TrackingModes are > all the same. > > For this reason I don't understand what you say on comment 31: > "Test B: Use a terminal to change the tracking gsettings between centered, > push, > proportional and for each mode, check the mode of focus tracking appropriately > changes. > > Result B: Okay" > > I don't understand the "Okay". AFAIS, all the non-none modes work the same. > Could you ellaborate this? (just in case I missed some of your conversations > with Jasper or Joseph on IRC) > > @@ +717,3 @@ > + let extents = [-1 , -1, -1, -1]; > + let component = accessibleFocus.get_component_iface(); > + extents = component.get_extents(Atspi.CoordType.SCREEN); > > Sometimes I get the following error: > (gnome-shell:9434): Gjs-WARNING **: JS ERROR: Exception in callback for signal: > focus-changed: TypeError: component is null > ZoomRegion<._updateFocus@/opt/gnome3/share/gnome-shell/js/ui/magnifier.js:719 > wrapper@/opt/gnome3/share/gjs-1.0/lang.js:213 > _emit@/opt/gnome3/share/gjs-1.0/signals.js:124 > FocusCaretTracker<._onChanged@/opt/gnome3/share/gnome-shell/js/ui/focusCaretTracker.js:46 > wrapper@/opt/gnome3/share/gjs-1.0/lang.js:213 > > So that means that sometimes the object focused/selected doesn't implement > component. Probably that are invalid objects (like object just to be deceased), > and it would be good to avoid to focus on them. But in any case, means that > there are a possibility of receiving an object that doesn't implement that > interface. So I will add a null check before calling extents. Something like > the null check that you do on _updateCaret (that case in relation with the text > interface). I can add an extra check for this like i did for the 'text' amd that will solve the problem
(In reply to comment #55) > (In reply to comment #54) > > So, a more correct comment would be something like: > > //state-changed:selected events have been included in the logic for focus > > events because in several cases, what the user see as a focus event is in fact > > a selection change, ie: navigate through menuitems inside a menu > > > > But with proper english. > > That is what I said and it was based on what is in the X documentation No. Current comment says: "Select events have been included in the logic for focus events only because objects will lose focus the moment they are selected." This seems to suggest that when an element is selected it will lose the focus. And that is false at clutter/gnome-shell. For example, as I explained, when the focus moves to the alt+tab switcher, the focus remains on the alt-tab switcher container, no matter how many times you change the selection on one of his children. What X documentation says is irrelevant here, this is not a patch for X. My proposed comment and current comment are not equivalent. In any case, this is just a detail, and doesn't affect the functionality like in other cases. > > @@ +51,3 @@ > > + registerFocusListener: function() { > > + return this._atspiListener.register(STATECHANGED + ':focused') || > > this._atspiListener.register( > > + STATECHANGED + ':selected'); > > > > That needs to be an && instead of ||. Using || provokes that only focused evets > > No I don't think so. It needs to be true if either events are recognised it is > not possible to have both a focus and select event so that change would mean > the result was always false. I think that you misunderstood the meaning of that || or && in that context. Taking into account this comment, it seems that you think that that line would make an atomic registration to both events at the same time, but that is not true, you are calling register twice. What you are doing is registering the same listener (so callback) to two different events: So the code that I'm proposing: { return atspiListener.register(event1) && atspiListener.register(event2) } is equivalent to: { result1 = atspiListener.register(event1); result2 = atspiListener.register(event2); return result1&&result2; } About the current code. As || uses a short-circuit evaluation [1], if the first one is true, the second one is not checked. So your current code: { return atspiListener.register(event1) || atspiListener.register(event2) } is equivalent to: { result = atspiListener.register(event1); if (result) return; return atspiListener.register(event2); } So, that means that if state-changed:focused is registered (something we want), it is not checked state-changed:selected, so is not registered. As I said, you need to register to both events. If not, the focus tracking would fail. So if you need both, registerFocusListener should return and && of the two registers, not one or the other. > > For this reason I don't understand what you say on comment 31: > > "Test B: Use a terminal to change the tracking gsettings between centered, > > push, > > proportional and for each mode, check the mode of focus tracking appropriately > > changes. > > > > Result B: Okay" > > > > I don't understand the "Okay". AFAIS, all the non-none modes work the same. > > Could you ellaborate this? (just in case I missed some of your conversations > > with Jasper or Joseph on IRC) Again, could you ellaborate this? > > So that means that sometimes the object focused/selected doesn't implement > > component. Probably that are invalid objects (like object just to be deceased), > > and it would be good to avoid to focus on them. But in any case, means that > > there are a possibility of receiving an object that doesn't implement that > > interface. So I will add a null check before calling extents. Something like > > the null check that you do on _updateCaret (that case in relation with the text > > interface). > > I can add an extra check for this like i did for the 'text' amd that will solve > the problem That is exactly what I proposed. [1] http://en.wikipedia.org/wiki/Short-circuit_evaluation
Review of attachment 253776 [details] [review]: I haven't looked over everything yet. But, regarding where API noted that changing the tracking mode does not have an effect, see below. ::: js/ui/magnifier.js @@ +732,3 @@ + this.xCaret = extents.x; + this.yCaret = extents.y; + this.scrollContentsTo(this.xCaret, this.yCaret); The point (this.xCaret, this.yCaret) needs to pass through one of the _centerFromPointPush(), _centerFromPointProportional(), or _centerFromPointCentered() methods in order to get the center based on the tracking mode, before calling scrollContentsTo(). Something along these lines: if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.PUSH) [this.xCaret, this.yCaret] = this._centerFromPointPush(extents.x, extents.y); else if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.PROPORTIONAL) [this.xCaret, this.yCaret] this._centerFromPointProportional(extents.x, extents.y); else if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.CENTERED) [this.xCaret, this.yCaret] = this._centerFromPointCentered(extents.x, extents.y); this.scrollContentsTo(this.xCaret, this.yCaret); It would be easier to read if the above logic were wrapped in a method "_centerFromCaretPosition()": [this.xCaret, this.yCaret] = this._centerFromCaretPosition(extents.x, extents.y); this.scrollContentsTo(this.xCaret, this.yCaret); And, similarly for _updateFocus(). This speaks to the issue that API raised where changing the focus or caret tracking modes has no effect.
(In reply to comment #57) > Review of attachment 253776 [details] [review]: > > I haven't looked over everything yet. But, regarding where API noted that > changing the tracking mode does not have an effect, see below. > > ::: js/ui/magnifier.js > @@ +732,3 @@ > + this.xCaret = extents.x; > + this.yCaret = extents.y; > + this.scrollContentsTo(this.xCaret, this.yCaret); > > The point (this.xCaret, this.yCaret) needs to pass through one of the > _centerFromPointPush(), _centerFromPointProportional(), or > _centerFromPointCentered() methods in order to get the center based on the > tracking mode, before calling scrollContentsTo(). Something along these lines: > > if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.PUSH) > [this.xCaret, this.yCaret] = this._centerFromPointPush(extents.x, > extents.y); > > else if (this._caretTrackingMode == > GDesktopEnums.MagnifierCaretTrackingMode.PROPORTIONAL) > [this.xCaret, this.yCaret] this._centerFromPointProportional(extents.x, > extents.y); > > else if (this._caretTrackingMode == > GDesktopEnums.MagnifierCaretTrackingMode.CENTERED) > [this.xCaret, this.yCaret] = this._centerFromPointCentered(extents.x, > extents.y); > > this.scrollContentsTo(this.xCaret, this.yCaret); > > It would be easier to read if the above logic were wrapped in a method > "_centerFromCaretPosition()": > > [this.xCaret, this.yCaret] = this._centerFromCaretPosition(extents.x, > extents.y); > this.scrollContentsTo(this.xCaret, this.yCaret); > > And, similarly for _updateFocus(). > > This speaks to the issue that API raised where changing the focus or caret > tracking modes has no effect. That seems to work well
(In reply to comment #56) > (In reply to comment #55) > > (In reply to comment #54) > > > > So, a more correct comment would be something like: > > > //state-changed:selected events have been included in the logic for focus > > > events because in several cases, what the user see as a focus event is in fact > > > a selection change, ie: navigate through menuitems inside a menu > > > > > > But with proper english. > > > > That is what I said and it was based on what is in the X documentation > > No. Current comment says: > "Select events have been included in the logic for focus events only because > objects will lose focus the moment they are selected." > > This seems to suggest that when an element is selected it will lose the focus. > And that is false at clutter/gnome-shell. For example, as I explained, when the > focus moves to the alt+tab switcher, the focus remains on the alt-tab switcher > container, no matter how many times you change the selection on one of his > children. What X documentation says is irrelevant here, this is not a patch for > X. > > My proposed comment and current comment are not equivalent. In any case, this > is just a detail, and doesn't affect the functionality like in other cases. > > > > @@ +51,3 @@ > > > + registerFocusListener: function() { > > > + return this._atspiListener.register(STATECHANGED + ':focused') || > > > this._atspiListener.register( > > > + STATECHANGED + ':selected'); > > > > > > That needs to be an && instead of ||. Using || provokes that only focused evets > > > > No I don't think so. It needs to be true if either events are recognised it is > > not possible to have both a focus and select event so that change would mean > > the result was always false. > > I think that you misunderstood the meaning of that || or && in that context. > Taking into account this comment, it seems that you think that that line would > make an atomic registration to both events at the same time, but that is not > true, you are calling register twice. What you are doing is registering the > same listener (so callback) to two different events: > > So the code that I'm proposing: > { > return atspiListener.register(event1) && atspiListener.register(event2) > } > > is equivalent to: > { > result1 = atspiListener.register(event1); > result2 = atspiListener.register(event2); > > return result1&&result2; > } > > About the current code. As || uses a short-circuit evaluation [1], if the first > one is true, the second one is not checked. So your current code: > > { > return atspiListener.register(event1) || atspiListener.register(event2) > } > > is equivalent to: > { > result = atspiListener.register(event1); > if (result) return; > return atspiListener.register(event2); > } > > So, that means that if state-changed:focused is registered (something we want), > it is not checked state-changed:selected, so is not registered. > > As I said, you need to register to both events. If not, the focus tracking > would fail. So if you need both, registerFocusListener should return and && of > the two registers, not one or the other. > What you are suggesting will cause the focus tracking to stop working. Moreover, I cannot see what problem you found that you expect this change should solve > > > For this reason I don't understand what you say on comment 31: > > > "Test B: Use a terminal to change the tracking gsettings between centered, > > > push, > > > proportional and for each mode, check the mode of focus tracking appropriately > > > changes. > > > > > > Result B: Okay" > > > > > > I don't understand the "Okay". AFAIS, all the non-none modes work the same. > > > Could you ellaborate this? (just in case I missed some of your conversations > > > with Jasper or Joseph on IRC) > > Again, could you ellaborate this? > To me, this was a non-issue because I really cannot imagine a scenario when you wouldn't want to use 'centered' for caret tracking or push for focus tracking and in my view the modes were never needed in the first place. The most important thing to my mind is that the feature (i.e focus caret tracking) is available to users who may depend on it and also that the choice to be able to turn it off is available. That is why prioritised testing those options before uploading the work. But in any case I was clearly wrong about that since this detail has caused the tracking to miss 3.9.91. I have added it in the way Joseph suggested I try so it is there in my follow-up patch.
Created attachment 254050 [details] [review] Magnifier: Implement focus and caret Tracking
(In reply to comment #59) > > As I said, you need to register to both events. If not, the focus tracking > > would fail. So if you need both, registerFocusListener should return and && of > > the two registers, not one or the other. > > > > What you are suggesting will cause the focus tracking to stop working. No. It will not. I have tested it. FWIW, Have you tested the small change I proposed before claiming for sure that will fail? > Moreover, I cannot see what problem you found that you expect this change > should solve All the scenarios I explained on comment 54 (navigate on a menu of gedit, using the alt+tab switcher, etc) were failing with patch uploaded at comment 52. > To me, this was a non-issue because I really cannot imagine a scenario when you > wouldn't want to use 'centered' for caret tracking or push for focus tracking > and in my view the modes were never needed in the first place. This comment is somewhat disturbing. Why you didn't raise that concern when you opened and provided a patch for bug 705652? > The most important thing to my mind is that the feature (i.e focus caret > tracking) is available to users who may depend on it and also that the choice > to be able to turn it off is available. Assuming that all the users will be happy with 'centered' for caret tracking and 'push' for focus tracking is at least debatable. > That is why prioritised testing those options before uploading the work. But in > any case I was clearly wrong about that since this detail has caused the > tracking to miss 3.9.91. This was not the only "detail" that caused it to miss. As I explained on my review, the patch was failing in several places. Again, something as commonly used as the alt+tab switcher was failing. > I have added it in the way Joseph suggested I try so > it is there in my follow-up patch. Ok.
Created attachment 254052 [details] [review] Magnifier: Implement focus and caret tracking Alejandro's suggestion did improve the focus tracking on certain applications that were not registering previously, after all. So, I have included this.
Created attachment 254054 [details] [review] Magnifier: Implement focus and caret Tracking
(In reply to comment #59) > To me, this was a non-issue because I really cannot imagine a scenario when you > wouldn't want to use 'centered' for caret tracking or push for focus tracking > and in my view the modes were never needed in the first place. Allow me to provide such a scenario then: People with advanced Retinitis Pigmentosa. And in the spirit of a picture being worth a thousand words, here's a picture from Wikipedia [1]. What you should note is: 1. Visual field is constricted to central vision (and can get way more constricted in the picture. Think "straw vision" rather than "tunnel vision") 2. Visual acuity is pretty clear. So these are users who tend to prefer a lower level of magnification (decent acuity) but who wind up having to do a lot of visual scanning to find what they are looking for (constricted field). The less visual scanning required, the less eye fatigue after prolonged use. For users with RP, centered focus tracking lessens the amount of visual scanning required because they can just keep looking at the center and only have to visually scan when the magnifier is unable to center the focused object (because, for instance, it is towards the unmagnified screen's edge). It would be a shame, therefore, to not have this option available -- especially when it was already in progress. [1] http://en.wikipedia.org/wiki/File:Human_eyesight_two_children_and_ball_with_retinitis_pigmentosa_or_tunnel_vision.png
Review of attachment 254054 [details] [review]: Just to avoid any hypothetical review (so waste of time) from Jasper. Magdalen detected some issues and she is working. Mentioned that on IRC.
Created attachment 254067 [details] [review] Magnifier: Implement focus and caret tracking It was necessary to tweak some additional things to get the GSettings modes to update properly without thowing an exception so I have fixed that now.
Created attachment 254069 [details] [review] Magnifier: Implement focus and caret tracking Sorry, that an old patch. Here is the latest.
Created attachment 254078 [details] [review] Magnifier: Implement focus and caret tracking Spotted a style typo so have corrected that here.
(In reply to comment #64) > (In reply to comment #59) > > > To me, this was a non-issue because I really cannot imagine a scenario when you > > wouldn't want to use 'centered' for caret tracking or push for focus tracking > > and in my view the modes were never needed in the first place. > > Allow me to provide such a scenario then: People with advanced Retinitis > Pigmentosa. And in the spirit of a picture being worth a thousand words, here's > a picture from Wikipedia [1]. What you should note is: > > 1. Visual field is constricted to central vision (and can get > way more constricted in the picture. Think "straw vision" > rather than "tunnel vision") > > 2. Visual acuity is pretty clear. > > So these are users who tend to prefer a lower level of magnification (decent > acuity) but who wind up having to do a lot of visual scanning to find what they > are looking for (constricted field). The less visual scanning required, the > less eye fatigue after prolonged use. > > For users with RP, centered focus tracking lessens the amount of visual > scanning required because they can just keep looking at the center and only > have to visually scan when the magnifier is unable to center the focused object > (because, for instance, it is towards the unmagnified screen's edge). It would > be a shame, therefore, to not have this option available -- especially when it > was already in progress. > > [1] > http://en.wikipedia.org/wiki/File:Human_eyesight_two_children_and_ball_with_retinitis_pigmentosa_or_tunnel_vision.png Ok thanks for clarifying. You should see that switching between modes has been fully included to functionality of the magnifiers caret and focus now. Let me know if you find something different from what you expect there.
Review of attachment 254078 [details] [review]: Not a full review, just some comments that came up when skimming over the code: ::: js/ui/focusCaretTracker.js @@ +44,3 @@ + else if (event.type == CARETMOVED) + update = 'caret-moved'; + this.emit(update, event); So if event.type neither contains 'object:state-changed' or is not 'object:text-caret-moved', we call this.emit(null, event)? @@ +49,3 @@ + registerFocusListener: function() { + return this._atspiListener.register(STATECHANGED + ':focused') && this._atspiListener.register( + STATECHANGED + ':selected'); Odd indentation, I'd prefer return this._atspiListener.register(...) && this._atspiListener.register(...); @@ +58,3 @@ + deregisterFocusListener: function() { + return this._atspiListener.deregister(STATECHANGED + ':focused') && this._atspiListener.deregister( + STATECHANGED + ':selected'); Dto. ::: js/ui/magnifier.js @@ +51,2 @@ let magDBusService = null; +let focusCaretTracker = null; Unused. Did you intend to use this instead of ZoomRegion.focusCaretTracker? @@ +668,3 @@ _init: function(magnifier, mouseSourceActor) { this._magnifier = magnifier; + this.focusCaretTracker = new FocusCaretTracker.FocusCaretTracker(); This could be private, no? @@ +701,3 @@ + this.yFocus = 0; + this.xCaret = 0; + this.yCaret = 0; Dto. @@ +714,3 @@ + let accessibleFocus = event.source; + let component = accessibleFocus.get_component_iface(); + if (!component || event.detail1 != 1) Is it common knowledge what "detail1 == 1" means? Otherwise it would be a good idea to define a constant for it at the top. Also as suggested by Jasper, the check could already be made in FocusCaretTracker ... @@ +1348,3 @@ + this._centerFromPointProportional(xCaret, yCaret); + } + else if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.PUSH) { I was going to complain about the odd braces style, but I see you are being consistent with the code above, so OK.
(In reply to comment #70) > Review of attachment 254078 [details] [review]: > > Not a full review, just some comments that came up when skimming over the code: > > ::: js/ui/focusCaretTracker.js > @@ +44,3 @@ > + else if (event.type == CARETMOVED) > + update = 'caret-moved'; > + this.emit(update, event); > > So if event.type neither contains 'object:state-changed' or is not > 'object:text-caret-moved', we call this.emit(null, event)? > > @@ +49,3 @@ > + registerFocusListener: function() { > + return this._atspiListener.register(STATECHANGED + ':focused') && > this._atspiListener.register( > + STATECHANGED + ':selected'); > > Odd indentation, I'd prefer > > return this._atspiListener.register(...) && > this._atspiListener.register(...); > > @@ +58,3 @@ > + deregisterFocusListener: function() { > + return this._atspiListener.deregister(STATECHANGED + ':focused') && > this._atspiListener.deregister( > + STATECHANGED + ':selected'); > > Dto. > > ::: js/ui/magnifier.js > @@ +51,2 @@ > let magDBusService = null; > +let focusCaretTracker = null; > > Unused. Did you intend to use this instead of ZoomRegion.focusCaretTracker? > > @@ +668,3 @@ > _init: function(magnifier, mouseSourceActor) { > this._magnifier = magnifier; > + this.focusCaretTracker = new FocusCaretTracker.FocusCaretTracker(); > > This could be private, no? > > @@ +701,3 @@ > + this.yFocus = 0; > + this.xCaret = 0; > + this.yCaret = 0; > > Dto. > > @@ +714,3 @@ > + let accessibleFocus = event.source; > + let component = accessibleFocus.get_component_iface(); > + if (!component || event.detail1 != 1) > > Is it common knowledge what "detail1 == 1" means? Otherwise it would be a good > idea to define a constant for it at the top. Also as suggested by Jasper, the > check could already be made in FocusCaretTracker ... > The idea was to only optimise when we know what we want and leave the focusCaretTracker to pick up on focus lost events but I am happy to go with the majority on this one. > @@ +1348,3 @@ > + this._centerFromPointProportional(xCaret, yCaret); > + } > + else if (this._caretTrackingMode == > GDesktopEnums.MagnifierCaretTrackingMode.PUSH) { > > I was going to complain about the odd braces style, but I see you are being > consistent with the code above, so OK. The rest I can change. I think. However, since uploading this commit I noticed the call to scrollContentsTo was getting a bit circular so I fixed that by taking out the call from the _updateFocus/Caret methods and putting it at the end of _centerFromFocus/Caret Position for each, instead. I will commit those changes once I have added these to them
Created attachment 254092 [details] [review] Magnifier: Implement focus and caret tracking
(In reply to comment #70) > Review of attachment 254078 [details] [review]: > > ::: js/ui/focusCaretTracker.js > @@ +44,3 @@ > + else if (event.type == CARETMOVED) > + update = 'caret-moved'; > + this.emit(update, event); > > So if event.type neither contains 'object:state-changed' or is not > 'object:text-caret-moved', we call this.emit(null, event)? If that happens, your system Atspi is broken and we have a lot more things to worry about.
Review of attachment 254092 [details] [review]: Almost there! ::: js/ui/magnifier.js @@ +715,3 @@ + return; + let extents = component.get_extents(Atspi.CoordType.SCREEN); + [this._xFocus, this._yFocus] = [extents.x, extents.y] Missing semicolon. @@ -935,3 @@ this._updateMousePosition(); - - // Determine whether the system mouse pointer is over this zoom region. Why was this changed? @@ -1177,3 @@ - if (params.redoCursorTracking && - this._mouseTrackingMode != GDesktopEnums.MagnifierMouseTrackingMode.NONE) { - // This depends on this.xMagFactor/yMagFactor already being updated Mind explaining this change as well?
(In reply to comment #74) > Review of attachment 254092 [details] [review]: > > Almost there! > > ::: js/ui/magnifier.js > @@ +715,3 @@ > + return; > + let extents = component.get_extents(Atspi.CoordType.SCREEN); > + [this._xFocus, this._yFocus] = [extents.x, extents.y] > > Missing semicolon. > > @@ -935,3 @@ > this._updateMousePosition(); > - > - // Determine whether the system mouse pointer is over this zoom > region. > > Why was this changed? > > @@ -1177,3 @@ > - if (params.redoCursorTracking && > - this._mouseTrackingMode != > GDesktopEnums.MagnifierMouseTrackingMode.NONE) { > - // This depends on this.xMagFactor/yMagFactor already being > updated > > Mind explaining this change as well? Hmm. I am not sure. Probably just civilian casualties. I will take a look! (In reply to comment #73) > (In reply to comment #70) > > Review of attachment 254078 [details] [review] [details]: > > > > ::: js/ui/focusCaretTracker.js > > @@ +44,3 @@ > > + else if (event.type == CARETMOVED) > > + update = 'caret-moved'; > > + this.emit(update, event); > > > > So if event.type neither contains 'object:state-changed' or is not > > 'object:text-caret-moved', we call this.emit(null, event)? > > If that happens, your system Atspi is broken and we have a lot more things to > worry about. It's not my system Atspi it's everybody's ;-). In any case, can you elaborate on what you mean here. Since I have added the following to account for any potential issue that not having it could cause: + if(update != null) + this.emit(update, event);
(In reply to comment #70) > Review of attachment 254078 [details] [review]: > > Not a full review, just some comments that came up when skimming over the code: > ... > ::: js/ui/focusCaretTracker.js > @@ +44,3 @@ > + else if (event.type == CARETMOVED) > + update = 'caret-moved'; > + this.emit(update, event); > > So if event.type neither contains 'object:state-changed' or is not > 'object:text-caret-moved', we call this.emit(null, event)? Given the tracker's registerX() methods, the only events registered for are focus-changed, select-changed, and caret-moved. Atspi should not report (i.e. filter out) all other event types. null can't happen in a well-behaved scenario. > Is it common knowledge what "detail1 == 1" means? Otherwise it would be a good > idea to define a constant for it at the top. Also as suggested by Jasper, the > check could already be made in FocusCaretTracker ... > There should be a constant for it in Atspi, but there isn't. In this case, it means that this is a focus-gained event, as opposed to a focus-lost event. The tracker should not make this check, since it is tracking all focus events. It's up to the client to determine whether this particular focus event is of interest. The magnifier is interested only when something gains focus.
> > @@ -1177,3 @@ > > - if (params.redoCursorTracking && > > - this._mouseTrackingMode != > > GDesktopEnums.MagnifierMouseTrackingMode.NONE) { > > - // This depends on this.xMagFactor/yMagFactor already being > > updated > > > > Mind explaining this change as well? > > Hmm. I am not sure. Probably just civilian casualties. I will take a look! The comment was an accidental erase but I felt that this._mouseTrackingMode != > > GDesktopEnums.MagnifierMouseTrackingMode.NONE needed to be removed since the function that calls the function this check happens from already makes this check so it was not necessary and was also confusing to have there.
(In reply to comment #76) > There should be a constant for it in Atspi, but there isn't. In this case, it > means that this is a focus-gained event, as opposed to a focus-lost event. The > tracker should not make this check, since it is tracking all focus events. It's > up to the client to determine whether this particular focus event is of > interest. The magnifier is interested only when something gains focus. can you have: const FOCUS_LOST = 0; const FOCUS_GAINED = 1; or whatever the correct values are at the top of the file, then?
Review of attachment 254092 [details] [review]: ::: js/ui/magnifier.js @@ +1347,3 @@ + } + else if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.CENTERED) { + this._centerFromPointCentered(xCaret, yCaret); The various _centerFrom...() methods take in an x and y coordinate, and return an array of coordinates. These should read: [xCaret, yCaret] = this._centerFromPointCentered(xCaret, yCaret). Otherwise, the changed value is not recorded, and the outcome is always centered tracking. @@ +1363,3 @@ + } + else if (this._focusTrackingMode == GDesktopEnums.MagnifierFocusTrackingMode.CENTERED) { + this._centerFromPointCentered(xFocus, yFocus); The various _centerFrom...() methods take in an x and y coordinate, and return an array of coordinates. These should read: [xFocus, yFocus] = this._centerFromPointCentered(xCaret, yCaret). Otherwise, the changed value is not recorded, and the outcome is always centered tracking.
Review of attachment 254092 [details] [review]: This patch still doesn't distinguish between modes, although most of the logic was added. Comments below. ::: js/ui/magnifier.js @@ +805,3 @@ + else + this._focusCaretTracker.registerFocusListener(); + }, As I told you on IRC, on this method you miss set the new focusTrackingMode: this._focusTrackingMode = mode; Note that setMouseTrackingMode only purpose is doing that. @@ +816,3 @@ + else + this._focusCaretTracker.registerCaretListener(); + }, As I told you on IRC, on this method you miss set the new caretTrackingMode: this._focusTrackingMode = mode; Note that setMouseTrackingMode only purpose is doing that. @@ +1338,3 @@ + _centerFromCaretPosition: function() { + let xCaret = this._xCaret; + let yCaret = this._yCaret; This should not be needed. See next comment. @@ +1349,3 @@ + this._centerFromPointCentered(xCaret, yCaret); + } + this.scrollContentsTo(xCaret, yCaret); Nor _centerFromPointProportional, _centerFromPointPush or _centerFromPointCentered updates xCaret or yCaret. They return the new value. So this call to this.scrollContentsTo(xCaret, yCaret) is using the values of this._xCaret or this._yCaret before calling any of those methods. In the same way, Im not sure why was the purpose of moving scrollContents from _updateCaret to here. The purpose of _centerFromCaretPosition is compute the centerPosition, not doing the scroll. I really think that it would be easier something like I suggested to you on IRC: at updateCaret: [this._xCaret, this._yCaret] = this._centerFromCaretPosition(this._xCaret, this._yCaret]; this.scrollContentsTo(this._xCaret, this._yCaret); Change _centerFromCaretPosition to (note that I re-added the return): _centerFromCaretPosition: function (xCaret, yCaret) { if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.PROPORTIONAL) { return this._centerFromPointProportional(xCaret, yCaret); } else if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.PUSH) { return this._centerFromPointPush(xCaret, yCaret); } else if (this._caretTrackingMode == GDesktopEnums.MagnifierCaretTrackingMode.CENTERED) { return this._centerFromPointCentered(xCaret, yCaret); } @@ +1366,3 @@ + } + this.scrollContentsTo(xFocus, yFocus); + }, Ditto.
(In reply to comment #75) > > > > @@ -1177,3 @@ > > - if (params.redoCursorTracking && > > - this._mouseTrackingMode != > > GDesktopEnums.MagnifierMouseTrackingMode.NONE) { > > - // This depends on this.xMagFactor/yMagFactor already being > > updated > > > > Mind explaining this change as well? > > Hmm. I am not sure. Probably just civilian casualties. I will take a look! Taking into account this kind of things, and the fact that each time you say that the patch works for you, but when I test it, it doesn't work, I really think that you are uploading wrong versions of your patch. Please, before uploading any new patch, test it with care. Thanks.
Created attachment 254110 [details] [review] Magnifier: Implement focus and caret tracking
Review of attachment 254110 [details] [review]: FWIW: we are talking about this bug on IRC. Magdalen will upload an updated patch soon.
Created attachment 254118 [details] [review] Magnifier: Implement focus and caret tracking Alejandro suggested I make some improvements to the setters for tracking modes. I have included that work here.
(In reply to comment #84) > Created an attachment (id=254118) [details] [review] > Magnifier: Implement focus and caret tracking > > Alejandro suggested I make some improvements to the setters for tracking modes. > I have included that work here. FWIW, I have just tested the patch. Now the needed functionality is there, and switching modes (using gsettings set on a terminal) change the behaviour properly. I will let the code review for Joseph and/or Jasper.
Review of attachment 254118 [details] [review]: I'm happy as well.
Thanks everyone.
Review of attachment 254118 [details] [review]: Fine with me.
Asked Feature Freeze Break request: https://mail.gnome.org/archives/release-team/2013-September/msg00045.html And we already have the 2 needed votes.
*** Bug 699366 has been marked as a duplicate of this bug. ***