> On Jan. 30, 2012, 3:40 p.m., Raphael Kubo da Costa wrote: > > Isn't it better to move FindKDeclarative.cmake to the top-level > > cmake/modules directory with the other find-files?
No if the intention is to prevent the file from being installed, which is the case here. > On Jan. 30, 2012, 3:40 p.m., Raphael Kubo da Costa wrote: > > ksmserver/FindKDeclarative.cmake, line 14 > > <http://git.reviewboard.kde.org/r/103621/diff/7/?file=48371#file48371line14> > > > > Why is this needed? I do not know, I just used FindJPEG.cmake as example. > On Jan. 30, 2012, 3:40 p.m., Raphael Kubo da Costa wrote: > > ksmserver/FindKDeclarative.cmake, lines 22-24 > > <http://git.reviewboard.kde.org/r/103621/diff/7/?file=48371#file48371line22> > > > > Why not pass KDECLARATIVE_LIBRARIES directly to find_library() to get > > rid of this block? I think that FindJJPEG.cmake is like a template and using ${KDECLARATIVE_NAMES} is usefull in the case where one library can have more than one name or use a non-standard name (name.so instead of libname.so for example). For kdeclarative that is not necessary. > On Jan. 30, 2012, 3:40 p.m., Raphael Kubo da Costa wrote: > > ksmserver/FindKDeclarative.cmake, lines 26-30 > > <http://git.reviewboard.kde.org/r/103621/diff/7/?file=48371#file48371line26> > > > > I don't see these being used anywhere in the patch, does it make sense > > to keep deprecated declarations in a new file? I guess no. I will remove them. > On Jan. 30, 2012, 3:40 p.m., Raphael Kubo da Costa wrote: > > ksmserver/FindKDeclarative.cmake, line 32 > > <http://git.reviewboard.kde.org/r/103621/diff/7/?file=48371#file48371line32> > > > > Missing KDECLARATIVE_LIBRARIES? Probably. I do not know if this line is really required. > On Jan. 30, 2012, 3:40 p.m., Raphael Kubo da Costa wrote: > > ksmserver/FindKDeclarative.cmake, line 1 > > <http://git.reviewboard.kde.org/r/103621/diff/7/?file=48371#file48371line1> > > > > As picky as it may seem, doesn't this file need a license? I will add one. - Lamarque Vieira ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/103621/#review10232 ----------------------------------------------------------- On Jan. 30, 2012, 5:08 p.m., Lamarque Vieira Souza wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/103621/ > ----------------------------------------------------------- > > (Updated Jan. 30, 2012, 5:08 p.m.) > > > Review request for KDE Base Apps and KDE Runtime. > > > Description > ------- > > Port shutdown dialog to QML. Two QML themes are included: default, which > mimics the current shutdown dialog look & feel, and contour, which is used in > Plasma Active. > > > This addresses bugs 216853 and 216853. > http://bugs.kde.org/show_bug.cgi?id=216853 > http://bugs.kde.org/show_bug.cgi?id=216853 > > > Diffs > ----- > > ksmserver/CMakeLists.txt 295b96e > ksmserver/Copyright.txt PRE-CREATION > ksmserver/FindKDeclarative.cmake PRE-CREATION > ksmserver/Messages.sh 0aa8bab > ksmserver/shutdown.cpp 7fd1e11 > ksmserver/shutdowndlg.h e5f0942 > ksmserver/shutdowndlg.cpp a09a1a7 > ksmserver/themes/contour/ContourButton.qml PRE-CREATION > ksmserver/themes/contour/main.qml PRE-CREATION > ksmserver/themes/contour/metadata.desktop PRE-CREATION > ksmserver/themes/contour/screenshot.png PRE-CREATION > ksmserver/themes/default/ContextMenu.qml PRE-CREATION > ksmserver/themes/default/KSMButton.qml PRE-CREATION > ksmserver/themes/default/MenuItem.qml PRE-CREATION > ksmserver/themes/default/helper.js PRE-CREATION > ksmserver/themes/default/main.qml PRE-CREATION > ksmserver/themes/default/metadata.desktop PRE-CREATION > ksmserver/themes/default/screenshot.png PRE-CREATION > > Diff: http://git.reviewboard.kde.org/r/103621/diff/diff > > > Testing > ------- > > Works in Plasma Active Two using MeeGo image and KDE SC 4.8. It does not work > in 4.7.x because the default theme requires kde-runtime 4.8's declarative > imports. > > TODO: > > . test right to left language support. > > > Screenshots > ----------- > > > http://git.reviewboard.kde.org/r/103621/s/400/ > New version with label accelerator working > http://git.reviewboard.kde.org/r/103621/s/407/ > > > Thanks, > > Lamarque Vieira Souza > >