Review LibKdeAccessibilityClient

[Alexander Neundorf]

On Thursday 16 August 2012, Frederik Gladhorn wrote:
> Hi,
> a few of us trying to improve accessibility of KDE worked on a little
> library, LibKdeAccessibilityClient.
> 
> What it does is making the AT-SPI over DBus protocol easy to use for KDE
> apps. This is the accessibility framework that we share with Gnome. It
> enables two way communication between apps and assistive tools. The first
> users for it would be the magnifiers in KWin and KMag - they can follow
> the focus with it. Further users will be screen readers and Simon with the
> AT-SPI plugin to dynamically create vocabulary for the running
> application's actions.
> 
> I'd like to ask for initial review in order to move this tiny lib to
> kdesupport/frameworks. It's Qt-only and actually only consists of two

LibKdeAccessibilityClientConfig.cmake.in:

Line 9: why is there the 
if(NOT LIBKDEACCESSIBILITYCLIENT_INSTALL_DIR)
   set(LIBKDEACCESSIBILITYCLIENT_INSTALL_DIR "@CMAKE_INSTALL_PREFIX@")
endif()

condition ?
This will be evaluated at usage-time. At cmake-time you already should know 
whether LIBKDEACCESSIBILITYCLIENT_INSTALL_DIR is set or not and set it to 
CMAKE_INSTALL_PREFIX if it is not set, but at cmake-time of the library, not 
at cmake-time of the using project.

Line 13: 
set(LIBKDEACCESSIBILITYCLIENT_INCLUDE_DIR         
"@LIBKDEACCESSIBILITYCLIENT_INCLUDE_DIR@")

This hardcodes the include dir, which will be problematic under Windows, but 
ok.

Line 15: this one should be changed, it hardcodes a lot of stuff:

et(LIBKDEACCESSIBILITYCLIENT_LIBS                
"@LIBKDEACCESSIBILITYCLIENT_LIB_DIR@/libkdeaccessibilityclient.so")

Please use the target-exporting feature of cmake

install(TARGETS kdeaccessibilityclient ... EXPORT MyExports ....)
install(EXPORTS MyExports ... )

and then install this export into the same directory as the Config.cmake file 
and include() it in the Config.cmake file.

CMakeLists.txt:

Line 29:
use 
find_poackage(PkgConfig)
instead of 
include(FindPkgConfig)

Line 30:
you rely completely on pkg_config to find atspi-2.
I would prefer if you could add a simple FindATSPI2.cmake, which first uses 
pkg_config and then uses its results as HINTS for the calls to
find_path() and find_library() (as e.g. done in FindLibXml2.cmake).

Alex
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20120816/db6c3e52/attachment.html>