GNOME Bugzilla – Bug 669192
D-Bus: org.gnome.Magnifier.setActive incomplete.
Last modified: 2021-07-05 14:32:35 UTC
Using DBus to enable the magnifier is only partly functional. It DOES work, but not as expected. When magnification is enabled using DBus alone, the GSettings: screen-magnifier-enabled key of schema org.gnome.desktop.a11y.applications. is not updated. Certain parts of gnome-shell rely on the gsettings key to be updated, while others will respond to the DBus command. Most visibly, the little slider that is visible when you click on the man in the circle at the top of the screen that allows you to enable magnification only updates when the gsettings value is changed, and not when the DBus value is changed. Steps to reproduce: 1.Gsettings works: $ gsettings set org.gnome.desktop.a11y.applications screen-magnifier-enabled false and $ gsettings set org.gnome.desktop.a11y.applications screen-magnifier-enabled true You will see that when the gsettings value is set to false, the slider in the accessibility menue is also set to false. You will also see that when the value is set to true the slider is also set to true. 2. Use the following python script, to do the same thing, but changing the DBus setting instead: Create the file: ---dbus_bug.py--------- #!/usr/bin/env python2 import dbus from dbus import DBusException session_bus = dbus.SessionBus() mag = session_bus.get_object( 'org.gnome.Magnifier', '/org/gnome/Magnifier') while True: mag.setActive(input("mag.setActive(True or False)")) print mag.isActive() ----------------------- $ python2 bug.py mag.setActive(True or False)False 0 mag.setActive(True or False)True 1 mag.setActive(True or False)False 0 mag.setActive(True or False)True 1 You will note that the slider does not move. Having a DBus setting which is different from the gsettins setting can lead to weird out of sync behavior. An app to play with can be pulled from git here: https://github.com/timthelion/gnome-shell-zoom-control-window It will run in place with the command $ python2 zoomcontrolwindow.py there is no need to install it perminently to your system. http://live.gnome.org/GnomeShell/Magnification
Most specifically, the weird and out of sync behavior relates to controlling the Magnifier using the hotkeys. If you set hotkeys for changing the zoom level by going to system settings, keyboard, shortcuts, universal access(I'm guessing on the names of the menues as mine are in Czech) then these hotkeys will only be active when they think that the magnifier is enabled. I'm not sure how this is determined, but if the dbus and gsettings settings are different, the behavior of the hotkeys becomes chaotic. I believe that the hotkeys are actually using their own third internal setting which is wrong...
(In reply to comment #1) If the goal is to change a user preference, use GSettings, not DBus. Here's a code fragment: from gi.repository.Gio import Settings # Get the a11y application preferences, and turn on the magnifier. a11yAppPrefs = Settings('org.gnome.desktop.a11y.applications') a11yAppPrefs.set_boolean('screen-magnifier-enabled', True) # Get magnifier preferences and change the magnification factor. magPrefs = Settings('org.gnome.desktop.a11y.magnifier') magPrefs.set_double('mag-factor', 3.5) Better yet, if there is a one-to-one relationship between a UI widget and a setting, use GSettings' bind() function. For example, if using a slider for magnification factor, then write something like: magPrefs.bind('mag-factor', slider.get_adjustment(), 'value', 0) Binding entails that any change to the slider value updates the GSetting value *and* any change to the GSetting immediately updates the slider. (Note: the '0' for the last parameter is the value for the bind constant G_SETTINGS_BIND_DEFAULT. I don't remember how to reference the constant from python). As noted in your description, if GSettings are used to change magnifier preferences, then the "Zoom" toggle button in the shell's universal access menu is appropriately updated. Also, the new zoom options panel will reflect the new preference values (http://live.gnome.org/ThreePointThree/Features/ZoomOptionsDialog). The hot keys in the universal access keyboard shortcuts panel should also use GSettings. If they don't, then that's a bug in the gnome-control-center project (https://bugzilla.gnome.org/browse.cgi?product=gnome-control-center). I did a smoke test -- the increase/decrease hotkeys work by changing the appropriate GSetting. But, I could not get the enable/disable hot key to work at all.
Thank you for your reply and your instructions. Why is there a DBus setting at all? What kind of application would ever want to set the DBus setting if it means a lack of synchronization with the zoom levels set by the hotkeys? In this case, if the DBus setting is different from the gsettings setting then we get random/unpredictable behavior. When is that ever wanted? I really truly don't want to set the gsettings setting. Take the code from which my example was forked: https://github.com/tobiasquinn/gnome-shell-mousewheel-zoom this code also has the desynchronization bug regarding DBus and gsettings, given that we use the same code to set the zoom level. Given this code as an example, setting the gsettings setting with each change of zoom level, we would be writting to disk many times a seccond. That is not "good practice" though I *think* that at this point the thrashing will be minimalized by disk buffering. I still believe this is a bug. Possible fixes: 1. Make the keycodes interact with the DBus so that there won't be random unpredictable jumping if DBus is used to set the zoom level. 2. Get rid of the DBus setting, since it's existance makes no sense. 3. Bind the DBus setting to the gsettings setting. *worse*
(In reply to comment #3) > Thank you for your reply and your instructions. > > Why is there a DBus setting at all? The rationale is (was) that GSettings is for persistent user preferences, and DBus for temporary, dynamic, non-persistent changes. This distinction is briefly outlined on the magnifier page (http://live.gnome.org/GnomeShell/Magnification#GSettings_vs._D-Bus:_Common_Magnifier_Framework). More detail is found in the "Common Magnifier Framework" proposal (http://live.gnome.org/Accessibility/MagnificationFramework). When GSettings vs. DBus were first discussed, I argued that toggling the magnifier should be handled solely by GSettings, but I didn't convince others. (Notice that in the "Common Magnifier Framework" proposal, there is no DBus method for changing the magnification factor). > > I still believe this is a bug. > > Possible fixes: > > ... > > 2. Get rid of the DBus setting, since it's existance makes no sense. > ... That's my preference. But, I think it's worth discussing on the a11y list to see if anyone objects to its removal. I'll post something soon.
After discussing on the a11y list and at a couple of irc meetings, it turns out the problem goes deeper than DBus. Since GS-mag is an object within GNOME Shell, it's possible for any other GNOME Shell object to directly invoke a magnifier setX() method and bypass GSettings. Hence, it's possible for a magnifier state to be out of sync with the corresponding GSettings value. The way to fix this is for the magnifier to insure that the appropriate setting is updated when setX() is invoked. If that's done, then this DBus issue evaporates, since GS-mag's DBus implementation is simply a wrapper around the actual GS-mag objects. It also means no change to the DBus interface/code. Patch forthcoming...
Created attachment 212313 [details] [review] Patch to sync magnifier states with corresponding settings, Modifies the internal GNOME Shell magnifier objects to update relevant GSettings when a magnifier state is directly manipulated.
Thank you for taking the time to fix this. I'll be looking forward to seeing this improved functionality soon.
(In reply to comment #5) > After discussing on the a11y list and at a couple of irc meetings, it turns out > the problem goes deeper than DBus. > > Since GS-mag is an object within GNOME Shell, it's possible for any other GNOME > Shell object to directly invoke a magnifier setX() method and bypass GSettings. > Hence, it's possible for a magnifier state to be out of sync with the > corresponding GSettings value. The way to fix this is for the magnifier to > insure that the appropriate setting is updated when setX() is invoked. Why not connect to the appropriate changed signals on the gsettings object, and then remove the public API? Does anything need the public API?
Review of attachment 212313 [details] [review]: This isn't how gsettings is supposed to be used. You aren't supposed to add another API layer on top of it, DBus or not. The set_* APIs are pretty much supposed to be used from some form of UI (in-process or not), and then you listen for changes in the actual program. ::: js/ui/magnifier.js @@ +315,3 @@ + let setting = this._settings.get_string(CROSS_HAIRS_COLOR_KEY); + if (setting != color) + this._settings.set_string(CROSS_HAIRS_COLOR_KEY, color); GSettings will fizzle this out for you if it actually hasn't changed. @@ +344,3 @@ + // Keep in sync with settings. + let setting = this._settings.get_int(CROSS_HAIRS_THICKNESS_KEY); + thickness = this.getCrosshairsThickness(); ??? @@ +400,3 @@ + // Keep in sync with settings. + let setting = this._settings.get_int(CROSS_HAIRS_LENGTH_KEY); + length = this.getCrosshairsLength(); ??? @@ +596,3 @@ + }); + zoom.connect('clampmode-changed', function(zoomRegion, clampMode) { + gsettings.set_boolean(CLAMP_MODE_KEY, !clampMode); Indentation is off here.
Happy 2014. This bug seems to suggest that the very existance of '/org/gnome/Magnifier' service is questionable and from what is said, I get the sense the bug is relevant to recent locking up issues as a result of synchronous dbus calls and atspi in gnome-shell. Accordingly I had a look at MagDBus.js: Note the comments reference https://git.gnome.org/browse/gnome-mag/tree/xml which doesn't exist (what happened to it? I hope there is an archive somewhere at least ?) not a biggie but it does suggest this file is in need of some love. There are a number of functions in the magDBus.js which already exist (by name at least) in the magnifier. I cannot see where the magDBus members have been called anywhere in the magnifier.js (because none of them have). Is there really a need to create an instance of magDBus in magnifier js at all?. If the dbus service should exist I am inclined to think that it might be handled more efficiently if it were in at-spi2-core since at-spi2-core's purpose to be the accessibility api's and hence magnifiers bridge to dbus. (will cc mgorse and hopefully get some of his advice on this). If the whole dbus service is conflicting with gsettings in some way then I cannot get a sense of how that comes about from reading this thread. Can anyone clarify this a bit more or point me to the right mailing list threads so I can have a look into what's been said about this, by myself?
I'd rather we got rid of D-Bus interface. I don't see the point of keeping it, personally. In which case would somebody want to control the magnifier in such a way that the settings would be lost by a restart?
(In reply to comment #11) > I'd rather we got rid of D-Bus interface. I don't see the point of keeping it, > personally. Echo this. Nobody is maintaining it and it's pretty buggy. It's probably better to let at-spi2-core carry the load. > In which case would somebody want to control the magnifier in such a way that > the settings would be lost by a restart? Which settings in particular?
(In reply to comment #12) > (In reply to comment #11) > > In which case would somebody want to control the magnifier in such a way that > > the settings would be lost by a restart? > > Which settings in particular? Any of the ones that are currently accessible via the D-Bus API (or that are supposed to be accessible via that API).
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #11) > > > In which case would somebody want to control the magnifier in such a way that > > > the settings would be lost by a restart? > > > > Which settings in particular? > > Any of the ones that are currently accessible via the D-Bus API (or that are > supposed to be accessible via that API). At-spi2-core is the accessibility bridge to d-bus so I am not sure why the activation needs to be done in MagnifierDBus, to be honest. With that said I actually need to rewrite a patch in at-spi2-core to activate and deactivate the magnifier there properly though so I would imagine for now, at least there is still some other relevant stuff that is needed in there. I suggest letting me remove all the outdated stuff, unused functions etc for now anyway. They confuse things. There is this accessibility toolkit bug which activates and deactivates the magnifier and the accessibility toolkit which is is just as much a bug in MagnifierDBus as in at-spi2-core (in the sense they could be fixed in either) C is not my strong point, so I have been humming and hawing about whether to fix that bug in MagnifierDBus instead or not for a while though I have to admit, I personally the idea of this dbus stuff being done in C. Ultimately, the creation of MagnifierDBus had some rationale once so although I have never really got to the bottom of what that rationale was, I would like to. At the moment it just seems like MagnifierDBus and at-spi2-core are not working harmoniously, at all and like neither of them completely serve the magnifier because each thinks the other is doing "something". I don't know. It's probably a good idea to cc Mike and Alejandro to this bug since they might be able to confirm what the big idea is and explain whether it is a good idea to remove this from gnome-shell and have all this settings handled in at-spi2-core instead.
(In reply to comment #14) > It's probably a good idea to cc Mike and Alejandro to this bug since they might > be able to confirm what the big idea is and explain whether it is a good idea > to remove this from gnome-shell and have all this settings handled in > at-spi2-core instead. This doesn't need to be handled at at-spi2 either. And since the beginning (see comment 5) the idea was about removing the DBUS interface. The only issue is that the patch needs works, as have some errors. So this is just about going on with the agreed (removing DBUS interface, handling everything with the settings), but with a patch solving the issues mentioned on the review at comment 9.
(In reply to comment #15) > (In reply to comment #14) > > > It's probably a good idea to cc Mike and Alejandro to this bug since they might > > be able to confirm what the big idea is and explain whether it is a good idea > > to remove this from gnome-shell and have all this settings handled in > > at-spi2-core instead. > > This doesn't need to be handled at at-spi2 either. And since the beginning (see > comment 5) the idea was about removing the DBUS interface. The only issue is > that the patch needs works, as have some errors. > > So this is just about going on with the agreed (removing DBUS interface, > handling everything with the settings), but with a patch solving the issues > mentioned on the review at comment 9. So essentially you think that Joseph's patch was correct and the issue holding that Jasper did not agree with that way forward? In that case perhaps it is worth opening some dialogue with him (or whoever is in GNOME Shell now) to learn exactly why he seems feel so strongly against the idea, so we can figure out a way forward? This is a problem that could do with being solved.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > > > It's probably a good idea to cc Mike and Alejandro to this bug since they might > > > be able to confirm what the big idea is and explain whether it is a good idea > > > to remove this from gnome-shell and have all this settings handled in > > > at-spi2-core instead. > > > > This doesn't need to be handled at at-spi2 either. And since the beginning (see > > comment 5) the idea was about removing the DBUS interface. The only issue is > > that the patch needs works, as have some errors. > > > > So this is just about going on with the agreed (removing DBUS interface, > > handling everything with the settings), but with a patch solving the issues > > mentioned on the review at comment 9. > > So essentially you think that Joseph's patch was correct and the issue holding > that Jasper did not agree with that way forward? No. I didn't say that. I said that what Joseph tried to do was the way to go, and Jasper just think that the patch had some issues to solve (so the needs-work status).
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > > > > It's probably a good idea to cc Mike and Alejandro to this bug since they might > > > > be able to confirm what the big idea is and explain whether it is a good idea > > > > to remove this from gnome-shell and have all this settings handled in > > > > at-spi2-core instead. > > > > > > This doesn't need to be handled at at-spi2 either. And since the beginning (see > > > comment 5) the idea was about removing the DBUS interface. The only issue is > > > that the patch needs works, as have some errors. > > > > > > So this is just about going on with the agreed (removing DBUS interface, > > > handling everything with the settings), but with a patch solving the issues > > > mentioned on the review at comment 9. > > > > So essentially you think that Joseph's patch was correct and the issue holding > > that Jasper did not agree with that way forward? > > No. I didn't say that. I said that what Joseph tried to do was the way to go, > and Jasper just think that the patch had some issues to solve (so the > needs-work status). In that case, what do you say to his question in #c8
(In reply to comment #18) > > No. I didn't say that. I said that what Joseph tried to do was the way to go, > > and Jasper just think that the patch had some issues to solve (so the > > needs-work status). > > In that case, what do you say to his question in #c8 Comment 8 is aligned with review on comment 9.
(In reply to comment #19) > (In reply to comment #18) > > > > No. I didn't say that. I said that what Joseph tried to do was the way to go, > > > and Jasper just think that the patch had some issues to solve (so the > > > needs-work status). > > > > In that case, what do you say to his question in #c8 > > Comment 8 is aligned with review on comment 9. Is the whole point of that patch not to change the way we are using gsettings? i.e. #8 and #9 seem to contradict #5. I guess what I am asking is what is it that is "right" about this patch.
(In reply to comment #15) > So this is just about going on with the agreed (removing DBUS interface, > handling everything with the settings), but with a patch solving the issues > mentioned on the review at comment 9. Is that what Bastien is saying in comment 11? > I'd rather we got rid of D-Bus interface. That sounds like he wants to remove the magnifier's DBus interface entirely. My patch does not do that. What it did was keep the magnifier gsettings in sync with the states of the magnifier *if* something called a magnifier setX() method. In a way, it did what gsettings bind() does (and I'd use bind() if I could). As for removing all of the magnifier D-Bus interface: Definitely remove the parts that duplicate gsettings. Otherwise, I have no strong opinion. The point of D-Bus is interprocess communication. The reason to keep a magnifier D-Bus interface is to provide a way for some other process (e.g., LibreOffice, or a voice command system) to drive the magnifier. But, aside from the idea, I have no concrete API for such interprocess communication.
(In reply to comment #21) > (In reply to comment #15) > > > So this is just about going on with the agreed (removing DBUS interface, > > handling everything with the settings), but with a patch solving the issues > > mentioned on the review at comment 9. > > Is that what Bastien is saying in comment 11? > > > I'd rather we got rid of D-Bus interface. > > That sounds like he wants to remove the magnifier's DBus interface entirely. > > My patch does not do that. What it did was keep the magnifier gsettings in > sync with the states of the magnifier *if* something called a magnifier setX() > method. If that was needed, and I can't see why, couldn't the problem use GSettings directly? > In a way, it did what gsettings bind() does (and I'd use bind() if I > could). > > As for removing all of the magnifier D-Bus interface: Definitely remove the > parts that duplicate gsettings. Otherwise, I have no strong opinion. The > point of D-Bus is interprocess communication. The reason to keep a magnifier > D-Bus interface is to provide a way for some other process (e.g., LibreOffice, > or a voice command system) to drive the magnifier. But, aside from the idea, I > have no concrete API for such interprocess communication. You should have D-Bus methods for stateless items, not for stateful ones. You'd use settings for stateful items. I think that the whole API of the magnifier probably needs review and splitting into those 2 categories.
GNOME is going to shut down bugzilla.gnome.org in favor of gitlab.gnome.org. As part of that, we are mass-closing older open tickets in bugzilla.gnome.org which have not seen updates for a longer time (resources are unfortunately quite limited so not every ticket can get handled). If you can still reproduce the situation described in this ticket in a recent and supported software version, then please follow https://wiki.gnome.org/GettingInTouch/BugReportingGuidelines and create a new ticket at https://gitlab.gnome.org/GNOME/gnome-shell/-/issues/ Thank you for your understanding and your help.