GNOME Bugzilla – Bug 723709
Crosshairs Are Broken
Last modified: 2014-02-08 23:57:00 UTC
Florian noticed this the other day at FOSDEM and having checked I now I see that my patch for https://bugzilla.gnome.org/show_bug.cgi?id=719370 fixes this problem as well as the one already described in that bug. I will amend the commit message and upload that here.
Created attachment 268210 [details] [review] restore crosshairs This patch inits atspi only when tracking of focus or caret is active instead of indirectly via the magnifier which breaks cross-hairs (and is hard to work around).
Review of attachment 268210 [details] [review]: Sorry, just realised a change I added should actually be in there now I think. Give me two sec I will add that to this mix
Review of attachment 268210 [details] [review]: Ok having double checked the code. It was not affected so I do not need to change anything. Sorry, it's been a long day!!
Review of attachment 268210 [details] [review]: One thing that bugs me is that sometimes ZoomRegions get the stored settings, and sometimes they don't. But it has always been the case, so... Anyway, I would prefer to see one patch with the settingsInit() changes, and one patch with focus/caret tracking disabled when not needed. ::: js/ui/magnifier.js @@ +805,3 @@ + } else { + this._focusCaretTracker.connect('focus-changed', + Lang.bind(this, this._updateFocus)); You're connecting this signal multiple times, if this function is called more than once (and same for setCaretTrackingMode)
(In reply to comment #5) > Review of attachment 268210 [details] [review]: > > One thing that bugs me is that sometimes ZoomRegions get the stored settings, > and sometimes they don't. But it has always been the case, so... > > Anyway, I would prefer to see one patch with the settingsInit() changes, and > one patch with focus/caret tracking disabled when not needed. > > ::: js/ui/magnifier.js > @@ +805,3 @@ > + } else { > + this._focusCaretTracker.connect('focus-changed', > + Lang.bind(this, this._updateFocus)); > > You're connecting this signal multiple times, if this function is called more > than once (and same for setCaretTrackingMode) AH mgorse advised in the past that this was ok to do since apparently it does not add any expense, but I might be imagining this. I will ask him to comment to be sure.
The problem is not the performance penalty of connecting multiple times, the problem is the callback will be called more than once, and side-effects will be multiplied. You should keep the signal handler ID around, and disconnect() it as needed
Created attachment 268213 [details] [review] restore crosshairs Ok thanks Giovanni, you were right. This restores the crosshairs and the magnifier still works fine when I try it out without the initialisation changes. I can raise a new bug for the init if that seems needed, I guess.
(In reply to comment #8) > Created an attachment (id=268213) [details] [review] > restore crosshairs > > Ok thanks Giovanni, you were right. This restores the crosshairs and the > magnifier still works fine when I try it out without the initialisation > changes. I can raise a new bug for the init if that seems needed, I guess. I didn't review the patch (will let gnome-shell reviewers doing that), but Magdalen asked to test the patch. FWIW, the patch seems to work fine. Solve the missing crosshairs, and I didn't detect any regression with the magnifier.
(In reply to comment #9) > (In reply to comment #8) > > Created an attachment (id=268213) [details] [review] [details] [review] > > restore crosshairs > > > > Ok thanks Giovanni, you were right. This restores the crosshairs and the > > magnifier still works fine when I try it out without the initialisation > > changes. I can raise a new bug for the init if that seems needed, I guess. > > I didn't review the patch (will let gnome-shell reviewers doing that), but > Magdalen asked to test the patch. FWIW, the patch seems to work fine. Solve the > missing crosshairs, and I didn't detect any regression with the magnifier. Thanks Alejandro.
Review of attachment 268213 [details] [review]: Looks good to me
(In reply to comment #11) > Review of attachment 268213 [details] [review]: > > Looks good to me Thanks!
Review of attachment 268213 [details] [review]: Committed as accepted following review.