[Kde-accessibility] Review Request: Focus Tracking in KWin's Zoom Plugin

[Thomas Lübking]

> On Aug. 15, 2012, 5:35 p.m., Martin Gräßlin wrote:
> > First of all: we cannot depend on anything in playground. This review has a push embargo till the library has gone through review.
> > 
> > Overall I dislike the extensive usage of ifdefs. I would prefer if the accessibility feature becomes a mandatory requirement for this effect to build. That is if LibKdeAccessibilityClient is not found the effect does not get build. I think this would make the code much cleaner to read and also to maintain.
> 
> Thomas Lübking wrote:
>     I wonder whether the compile time check
>     a) is required at all
>     b) a good idea
>     
>     It looks to me as if the effect would through that lib just hook to some dbus signal - and one does not really have to link some lib for that purpose (that may draw in what not else, notably ati-spi) while and overmore this feature will then likely depend on the presence of some loaded kded module or so, shipped with kaccessibility, rather then the lib presence at compile time, so it actually has to check whether that is installed / loaded - yesno?
> 
> Sebastian Sauer wrote:
>     > I would prefer if the accessibility feature becomes a mandatory requirement for this effect to build.
>     
>     Just some background on that: libkdea11yclient is a Qt-only lib with dependency on dbus. There is a compile-time dependency on a header-file on atspi but we could easily remove that dependency by e.g. copying the header into our lib.
>     
>     Its a thin library and fullfits the requirements for kdeframeworks tear1. So, in the ideal case it would be there (means in kdeframeworks) once frameworks is ready. For now (means with kdelibs+kdesupport) it would match into both imho but since it does not depend on anything else kdesupport would maybe match better. In any case our plan is to bring that discussion and/or a reviewboard request for moving it there soon. The plan is for sure not to merge this or the mag patches before the kdeay11yclient library is at its new place but neverless (so was the idea) it cannot harm to do the review-request itself now and merge things later :)
>     
>     Long explanation short result:
>     Yes, would make sense to add a hard dependency and it would NOT drag anything bigger in but only that one lib.
>     
>     Note that compare the previous solution (where we only listened direct to the KAccessible dbus signal without dependency to a another lib) this is not so easy with atspi. We need some more magic to proper determinate the focus-point and decided not to do with a kded or another additional daemon but have a library for that that communicates direct with the atspi daemon and does all the hard client-side magic to finally earn that simple focusChanged signal.
>     
>     There may another reason that would speak for making it a hard dependency:
>     * Focus tracking is just the very first a11y-specific functionality that is implemented in KWin but I can imagine there may more cases in the future. One thing I could imagine is bridghtness/contrast/highlightning related to certain areas of interest ( https://live.gnome.org/GnomeShell/Magnification#Features_in_Development ). libkdea11yclient provides us a deep view on all of the applications (GTK and Qt) and not only to the "outer window vs content" region what makes it rather interesting for effects/features/
>     etc. related to things happening inside the window / the widgets / actions done by the user on them / context-actions provided by the applications (yes, you can for example introspect the app, fetch/trigger menu-items, buttons and stuff using atspi).
>     * Beside the (fullscreen) zoom-plugin there is also those magnifier plugin which does not offer such a feature yet. Maybe some day there is a demand to allow some kind of mixed mode where e.g. half of the screen is the magnifier+focustracking and the other half is just a regular view on the desktop (some kind of overview). That is similar to what https://live.gnome.org/GnomeShell/Magnification/#org.gnome.Magnifier.ZoomRegion enables.
>     
>     So, all in all I think there are lot more use-cases then only focus-tracking that can be implemented using libkdea11y and yes, it would make sense to not add everytime ifdefs' all over especially since the dependency is rather thin/small.
>
> 
> Martin Gräßlin wrote:
>     Thanks for the long explanation. Given that I think we should focus on getting it a hard dependency either for just the effect or maybe even for kde-workspace in general (personal opinion: accessibility is not a nice-to-have feature, but something which is a mandatory requirement).
>     
>     Only requirement from my side for the lib: no sync D-Bus calls, we are allergic to that in the window manager ;-) But I will have a look on that once the review for the lib starts.

Thanks for addressing the dependency concern.

So since the ultimate runtime dependency seems the at-spi-registryd, I'd suggest to reflect that in the config GUI (rather than the compile time presence) and simply disable the entries hinting that the at-spi daemon is required for this feature of the zoom effect but not running.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/106041/#review17466
-----------------------------------------------------------


On Aug. 15, 2012, 3:07 p.m., Amandeep Singh wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/106041/
> -----------------------------------------------------------
> 
> (Updated Aug. 15, 2012, 3:07 p.m.)
> 
> 
> Review request for KDE Accessibility, kwin, Frederik Gladhorn, Sebastian Sauer, and Luboš Luňák.
> 
> 
> Description
> -------
> 
> This patch is for KWin, makes the focus-tracking feature of KWin work with applications. This makes KDE more accessible.
> A new accessibility client library "libkdeaccessibilityclient" (which can be found here: https://projects.kde.org/projects/playground/accessibility/libkdeaccessibilityclient) is also to be added in kde-support.
> 
> The focus-tracking feature will use the new library to get the accessibility information from at-spi.
> 
> Adjustments in CMakeLists have been made to add the library as an optional dependency, which if found enables the focus-tracking option in Zoom Plugin.
> And in absence of library, the option is removed.
> 
> 
> Diffs
> -----
> 
>   kwin/effects/CMakeLists.txt 29accb1 
>   kwin/effects/zoom/CMakeLists.txt 36e11ac 
>   kwin/effects/zoom/focustrackconfig.h.in PRE-CREATION 
>   kwin/effects/zoom/zoom.h d809b21 
>   kwin/effects/zoom/zoom.cpp b275b1e 
>   kwin/effects/zoom/zoom_config.cpp 3edd10c 
> 
> Diff: http://git.reviewboard.kde.org/r/106041/diff/
> 
> 
> Testing
> -------
> 
> I tested KWin's new feature working with KWrite and Konsole.
> 
> 
> Thanks,
> 
> Amandeep Singh
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-accessibility/attachments/20120816/12b2c594/attachment-0001.html>