Review Request 112785: Add ki18n_wrap_ui macro to ki18nMacros

Sun Nov 10 15:43:22 UTC 2013

> On Sept. 23, 2013, 10:37 a.m., Kevin Ottens wrote:
> > I'm surprised it doesn't use qt5_wrap_ui. It seems to reinvent it at least partly.
> 
> Jeremy Whiting wrote:
>     well, qt5_wrap_ui wasn't around when this was created (as kde4_add_ui_files iirc). All I did was copy it and rename it. didn't look into making it use qt5_wrap_ui.
> 
> Kevin Ottens wrote:
>     Could you please look into it?
> 
> Chusslove Illich wrote:
>     This is why I asked Jeremy in the other review, that motivated this one, to
>     commit using the plain qt5_wrap_ui, until I get to doing the proper thing
>     for k18n_wrap_ui.
>     
>     Yes, in spirit k18n_wrap_ui should be a wrapper for qt5_wrap_ui, and thus I
>     would call it k18n_qt5_wrap_ui. If uic would have some additional command
>     line options, k18n_wrap_ui would internally really be a wrapper for
>     qt5_wrap_ui; but without these options, it may end up just copying the code
>     (little as there is) from qt5_wrap_ui and adding its own stuff atop.
>     k18n_qt5_wrap_ui must also have its own macro options to support the new/
>     modified ki18n functionality (namely setting the translation domain and
>     activating the KUIT markup processing).
>
> 
> Jeremy Whiting wrote:
>     Ok, I'll leave this alone for now, and just #include <klocalizedstring.h> where we were depending on that being added to the ui_*.h files before.
> 
> Kevin Ottens wrote:
>     Is this patch abandoned or it'll see another revision soon?
> 
> Chusslove Illich wrote:
>     It should see another revision soon, from me. Or maybe that should be
>     another review (different submitter)? The topic is the same...
> 
> Stephen Kelly wrote:
>     Actually, uic should get a command line option to add an include.
>     
>     Then no macro will be needed at all (with the next CMake release - CMake 3.0)
>     
>      http://cmake.org/gitweb?p=cmake.git;a=commitdiff;h=6368bd717e1cee3b947667ea0eaee78f187a2b3b
>     
>     All that would be needed is
>     
>      set(CMAKE_AUTOUIC_OPTIONS -tr i18n -include klocalizedstring.h) 
>     
>     in KI18nConfig.cmake.
> 
> Aleix Pol Gonzalez wrote:
>     Well, we certainly don't want on /all/ calls to uic. When uic is used with projects that don't use ki18n, this shouldn't be applied.
> 
> Stephen Kelly wrote:
>     Projects which use KI18nConfig.cmake do though, yesno?
>     
>     Maybe we can encode it into the KF5::KI18n target instead though. That would be a much better solution.
>     
>
> 
> Chusslove Illich wrote:
>     Another needed option to uic is to define a macro, for setting
>     TRANSLATION_DOMAIN in library code. Then, it must be possible to set whether
>     ordinary or KUIT markup calls are used, i.e. whether -tr tr2i18n or
>     -tr tr2xi18n. So I intended that k18n_qt5_wrap_ui looks something like:
>     
>       ki18n_qt5_wrap_ui(outfilevar [DOMAIN <domain>] [KUIT] uifiles...)
>     
>     and internally call uic with proper options. For example:
>     
>       ki18n_qt5_wrap_ui(foolib_SRCS DOMAIN "foolib" KUIT
>           fooconfig.ui
>           foo.ui
>           ...
>       )
>     
>     Before uic gets these options, ki18n_qt5_wrap_ui would internally do what
>     qt5_wrap_ui does plus the makeshift stuff to do the rest (as in KDE 4).
>
> 
> Stephen Kelly wrote:
>     
>     > Another needed option to uic is to define a macro, for setting
>     > TRANSLATION_DOMAIN in library code. 
>     
>     Assuming you mean a preprocessor macro, that can be set from CMake as a -D, right? 
>     
>     I think it would be easier to get the -include in than the -domain, so that should be aimed for separately and first.
> 
> Chusslove Illich wrote:
>     Right, a preprocessor macro. And I meant setting it by implementing the
>     -define option in uic, rather than the more specific -domain.
>     
>     But how to set all this information is just a matter of convenience. If
>     add_definitions plus qt5_wrap_ui with explicit -tr option (and -include
>     unless CMAKE_AUTOUIC_OPTIONS is used) is deemed good enough, then
>     ki18n_qt5_wrap_ui is not needed indeed.
>
> 
> Stephen Kelly wrote:
>     
>     > But how to set all this information is just a matter of convenience. If
>     > add_definitions plus qt5_wrap_ui 
>     
>     That would also not be needed. The required options to uic would be used simply by linking to KI18n.
>
> 
> Chusslove Illich wrote:
>     So, one must be able to set two things. Add the TRANSLATION_DOMAIN macro;
>     this can be done by add_definitions. Choose whether -tr tr2i18n or
>     -tr tr2xi18n is issued to uic; how can this be done?
>
> 
> Stephen Kelly wrote:
>     I think there's no need for the add_definitions.
>     
>     We would add something like this to ki18n:
>     
>      set_property(TARGET KI18n PROPERTY INTERFACE_AUTOUIC_OPTIONS -include klocalizedstring -tr tr2i18n -define TRANSLATION_DOMAIN=$<TARGET_PROPERTY:NAME>)
>     
>     Additionally, if the TRANSLATION_DOMAIN is needed in c++ code that I write as a developer, we should this to ki18n:
>     
>      set_property(TARGET KI18n PROPERTY INTERFACE_COMPILE_DEFINITIONS TRANSLATION_DOMAIN=$<TARGET_PROPERTY:NAME>)
>     
>     That assumes that -DTRANSLATION_DOMAIN=foo should be used when compiling the foo target. Is that the case?
>     
>     Because are INTERFACE_ properties, when I use 
>     
>      add_library(user mywidget.cpp)
>      target_link_libraries(user KF5::KI18n)
>     
>     CMake will use those options when running uic on mywidget.ui.
>
> 
> Chusslove Illich wrote:
>     I didn't look up yet how this set_property mechanism works, but in your
>     example I still don't see where the translation domain name is given, and
>     how it can be selected whether -tr tr2i18n or -tr tr2xi18n is issued to uic.
>     It appears to me you want to link the translation domain name to the build
>     target name; while sometimes these would be same, sometimes they wouldn't
>     be. The translation domain name should be explicitly given.
>     
>     Right, we would normally want -DTRANSLATION_DOMAIN= for all the code in a
>     target, including *.cpp files.
>

Any chance to see an update or it is abandonned?

- Kevin

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

On Sept. 17, 2013, 7:56 p.m., Jeremy Whiting wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112785/
> -----------------------------------------------------------
> 
> (Updated Sept. 17, 2013, 7:56 p.m.)
> 
> 
> Review request for KDE Frameworks and Alexander Neundorf.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> It builds and installs, but doesn't seem to be usable from within kdelibs, i.e. ki18n_wrap_ui in knewstuff/src/CMakeLists.txt fails with this. I suspect one more thing is needed to make it work from within kdelibs builds.
> 
> 
> Diffs
> -----
> 
>   tier2/ki18n/CMakeLists.txt d0ed448 
>   tier2/ki18n/KI18nConfig.cmake.in 18b6e2f 
>   tier2/ki18n/cmake/KI18NMacros.cmake PRE-CREATION 
>   tier2/ki18n/cmake/ki18nuic.cmake PRE-CREATION 
> 
> Diff: http://git.reviewboard.kde.org/r/112785/diff/
> 
> 
> Testing
> -------
> 
> Builds and installs into PREFIX/lib64/cmake/KI18N next to KI18nConfig.cmake
> 
> 
> Thanks,
> 
> Jeremy Whiting
> 
>

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