After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 629884 - magnifier defaults
magnifier defaults
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: magnifier
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Joseph Scheuhammer
gnome-shell-maint
: 633572 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2010-09-16 23:24 UTC by William Jon McCann
Modified: 2013-12-04 18:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
magnifier: Change default values (1.39 KB, patch)
2010-09-16 23:58 UTC, Florian Müllner
committed Details | Review
magnifier: Sync MouseTrackingMode values with the gsettings enum (879 bytes, patch)
2010-09-16 23:58 UTC, Florian Müllner
needs-work Details | Review
magnifier: Sync MouseTrackingMode values with the gsettings enum (1.75 KB, patch)
2010-09-18 09:54 UTC, Florian Müllner
none Details | Review
magnifier: Sync MouseTrackingMode values with the gsettings enum (2.13 KB, patch)
2010-09-18 11:43 UTC, Florian Müllner
none Details | Review
magnifier: Sync MouseTrackingMode values with the gsettings enum (2.23 KB, patch)
2010-09-18 16:06 UTC, Florian Müllner
none Details | Review
magnifier: Sync MouseTrackingMode values with the gsettings enum (1.18 KB, patch)
2010-10-30 20:22 UTC, Florian Müllner
committed Details | Review

Description William Jon McCann 2010-09-16 23:24:59 UTC
Our screen magnifier should default to fullscreen with "proportional" mouse tracking.  We should also have global hotkeys for enabling/disabling, zooming in, and zooming out.

The proportional mouse tracking can be seen in orca or in OS X.
Comment 1 William Jon McCann 2010-09-16 23:47:51 UTC
So partly:

gsettings set org.gnome.accessibility.magnifier screen-position \"full-screen\"
gsettings set org.gnome.accessibility.magnifier mouse-tracking \"proportional\"

And fix:
diff --git a/js/ui/magnifier.js b/js/ui/magnifier.js
index e572961..d68ab9c 100644
--- a/js/ui/magnifier.js
+++ b/js/ui/magnifier.js
@@ -15,8 +15,8 @@ const MagnifierDBus = imports.ui.magnifierDBus;
 const MouseTrackingMode = {
     NONE: 0,
     CENTERED: 1,
-    PUSH: 2,
-    PROPORTIONAL: 3
+    PROPORTIONAL: 2,
+    PUSH: 3
 };

We also have a bug in the proportional mode where it uses the full screen width and height to calculate the viewable area.  This makes it hard to use the edges of the screen because there is insufficient periphery.  There should be a pretty decent margin all the way around the edges of the screen.  So for example, in order to activate the Activities hot corner you should only have to move the cursor to (say) 10% of the screen width from the edge of the monitor.  We can experiment for the right value.
Comment 2 Florian Müllner 2010-09-16 23:58:13 UTC
Created attachment 170447 [details] [review]
magnifier: Change default values

mccann: we should default to full screen with mouse position as
"proportional"
Comment 3 Florian Müllner 2010-09-16 23:58:17 UTC
Created attachment 170448 [details] [review]
magnifier: Sync MouseTrackingMode values with the gsettings enum

The tracking modes "push" and "proportional" are swapped - fix the
code so that the setting behaves as expected.
Comment 4 Owen Taylor 2010-09-17 12:03:50 UTC
Assigning to Joseph for review
Comment 5 Joseph Scheuhammer 2010-09-17 15:58:19 UTC
Review of attachment 170447 [details] [review]:

mccann: we should default to full screen with mouse position as
"proportional"

It should have defaulted to full screen for a while now.  My bad.

I have no opinion regarding the default mouse tracking mode.  I asked our occupational therapist for her input.  She advised that all things being equal, proportional cues location better than the other modes.  However, I *think* Orca's preferences default to centred.  I'll touch base with Joanie on this.
Comment 6 Joseph Scheuhammer 2010-09-17 18:17:07 UTC
(In reply to comment #5)
> I *think*
> Orca's preferences default to centred.  I'll touch base with Joanie on this.

Joanie confirmed.  But, she went on to suggest that the default should be "push".
Comment 7 Owen Taylor 2010-09-17 18:21:08 UTC
I was actually asking for code review (though these patches are pretty simple). So, if you think the patches are good to commit, mark accepted-commit-now, if you think they need changes, describe what the changes are and mark needs-work.
Comment 8 William Jon McCann 2010-09-17 18:28:05 UTC
Yeah we want proportional.  I'll open a separate bug for fixing the proportional behavior.
Comment 9 Joseph Scheuhammer 2010-09-17 18:33:42 UTC
Review of attachment 170448 [details] [review]:

Orca uses centered=0, proportional=1, push=2, none=3.  This would be a good time to synchronize with Orca.
Comment 10 Joseph Scheuhammer 2010-09-17 18:37:11 UTC
Review of attachment 170447 [details] [review]:

Code looks right.
Comment 11 Florian Müllner 2010-09-18 09:54:59 UTC
Created attachment 170533 [details] [review]
magnifier: Sync MouseTrackingMode values with the gsettings enum

The tracking modes "push" and "proportional" are swapped - while
at it, synchronize the values with Orca.
Comment 12 Milan Bouchet-Valat 2010-09-18 10:02:35 UTC
Isn't it a little strange to have:
> NONE: 3
instead of
> NONE: 0

Maybe we can't fix Orca though, but if we can, now is the time to fix this weird value...
Comment 13 Florian Müllner 2010-09-18 11:42:17 UTC
Comment on attachment 170448 [details] [review]
magnifier: Sync MouseTrackingMode values with the gsettings enum

(In reply to comment #12)
> Isn't it a little strange to have:
> > NONE: 3
> instead of
> > NONE: 0
> 
> Maybe we can't fix Orca though, but if we can, now is the time to fix this
> weird value...

Yeah, NONE:0 makes more sense to me. On the other hand, changing Orca likely is going to mess with users' settings, while the code change in gnome-shell is completely undisruptive (except maybe for some raised eyebrows).
Comment 14 Florian Müllner 2010-09-18 11:43:50 UTC
Created attachment 170535 [details] [review]
magnifier: Sync MouseTrackingMode values with the gsettings enum

Adjust bounds check to use the changed values.
Comment 15 Milan Bouchet-Valat 2010-09-18 15:18:33 UTC
(In reply to comment #13)
> Yeah, NONE:0 makes more sense to me. On the other hand, changing Orca likely is
> going to mess with users' settings, while the code change in gnome-shell is
> completely undisruptive (except maybe for some raised eyebrows).
Am I correct that enums are stored as strings in GConf and GSettings, and that integer values are only internal representations? In this case, fixing the Shell and Orca wouldn't be a problem.
Comment 16 Florian Müllner 2010-09-18 16:06:18 UTC
Created attachment 170553 [details] [review]
magnifier: Sync MouseTrackingMode values with the gsettings enum

Fix a syntax error.
Comment 17 Joanmarie Diggs (IRC: joanie) 2010-09-24 14:22:34 UTC
Hey guys. FWIW, Orca generally does:

  DEFAULT_SETTING: 0
  OTHER_SETTING1: 1
  OTHER_SETTING2: 2
  ...

And in the case of mouse tracking, normally you want the mouse tracked *somehow* -- to not track the mouse at all would be "weird." Thus None is not default, and as a result, None is not 0. 

Having said that, no one should be accessing these settings by literal value. (And if they are, they get what they deserve. ;-) )Therefore, if it would make y'all happy to have Orca modified to fit your world view, go for it.
Comment 18 Florian Müllner 2010-09-24 14:49:30 UTC
(In reply to comment #17)
> Hey guys. FWIW, Orca generally does:
> 
>   DEFAULT_SETTING: 0
>   OTHER_SETTING1: 1
>   OTHER_SETTING2: 2
>   ...
> 
> And in the case of mouse tracking, normally you want the mouse tracked
> *somehow* -- to not track the mouse at all would be "weird." Thus None is not
> default, and as a result, None is not 0.

Ah, makes sense then. Now the obvious question is whether it makes sense to change *both* the patch and Orca to account for the new default value ...
Comment 19 Milan Bouchet-Valat 2010-09-24 15:08:53 UTC
If you already have a consistent policy, don't worry about my remark - it would add inconsistency in Orca. Anyway, the value stored in GSettings is what matters, and it's a string...
Comment 20 William Jon McCann 2010-10-02 18:48:47 UTC
What's the story here folks?  Can we push this?
Comment 21 Florian Müllner 2010-10-03 12:34:23 UTC
(In reply to comment #20)
> What's the story here folks?  Can we push this?

Second patch is still waiting for review. Also, my question about whether it makes sense to change the enum values to match the "0 == default value" policy still applies ...
Comment 22 Joseph Scheuhammer 2010-10-25 17:39:06 UTC
(In reply to comment #21)
> (In reply to comment #20)
> > What's the story here folks?  Can we push this?
> 
> Second patch is still waiting for review. Also, my question about whether it
> makes sense to change the enum values to match the "0 == default value" policy
> still applies ...

Some thoughts.

1.  Most of the uses of enumerated types in GNOME Shell use 0 for the "none" value, so switching to "0 = the default value" would introduce an inconsistency in style.

2.  Joanie reports that Orca does not actually persist the numerical value, so, in that sense, there is no question about matching Orca's use of numeric constants for its preferences.  We could simply ignore numeric values in the magnifier code -- that is, use GSettings' get/set_string() to acquire the 'nick names' of the enumeration.  Admittedly, the ability to do range checks easily is lost, e.g.: 'if (setting >= NONE && setting <= MAX_ENUM)'.  But, on the plus side, the whole issue of what to do with NONE, and how to be consistent with Orca simply goes away.

The implication is to re-write parts of magnifier.js so as to use just the nick-names.  I'll do that if others think that's a viable way to go.
Comment 23 Florian Müllner 2010-10-30 20:07:04 UTC
*** Bug 633572 has been marked as a duplicate of this bug. ***
Comment 24 Florian Müllner 2010-10-30 20:22:11 UTC
Created attachment 173567 [details] [review]
magnifier: Sync MouseTrackingMode values with the gsettings enum

(In reply to comment #22)
> 1.  Most of the uses of enumerated types in GNOME Shell use 0 for the "none"
> value, so switching to "0 = the default value" would introduce an inconsistency
> in style.

OK, going back to 0 == NONE.


> 2.  Joanie reports that Orca does not actually persist the numerical value, so,
> [...]
> The implication is to re-write parts of magnifier.js so as to use just the
> nick-names.  I'll do that if others think that's a viable way to go.

It probably is, but I think we should get the bug fix in nonetheless - having the settings behave as expected certainly wouldn't hurt while working on the code.
Comment 25 Joseph Scheuhammer 2010-11-01 14:26:51 UTC
Review of attachment 173567 [details] [review]:

Yes, this has been lingering a while.  Let's commit this.
Comment 26 Florian Müllner 2010-11-01 15:08:37 UTC
Attachment 170447 [details] pushed as 576376a - magnifier: Change default values
Attachment 173567 [details] pushed as 0c2aa14 - magnifier: Sync MouseTrackingMode values with the gsettings enum

I think the change of settings enums => nick-names should get its own bug, so closing this one ...