Review Request 122680: kglobalaccel: Remove the runtime's KAboutData

[Martin Gräßlin]

> On Feb. 23, 2015, 12:23 p.m., Martin Klapetek wrote:
> > From the docs "Currently, the values set here are shown by the "About" box (see KAboutDialog), used by the bug report dialog (see KBugReport), and by the help shown on command line (see KAboutData::setupCommandLine())."
> > 
> > So daemon has no About box but might need it for the KBugReport. And it doesn't implement --help anyway.
> > 
> > I think this might be fine for a daemon however, but for sure should be replaced by the QApplication bits:
> > 
> > app.setApplicationVersion(aboutData.version());
> > app.setApplicationName(aboutData.componentName());
> > app.setApplicationDisplayName(aboutData.displayName());
> > app.setOrganizationDomain(aboutData.organizationDomain());
> 
> Martin Gräßlin wrote:
>     I agree that the application data needs to be set, so that bug reports in case of e.g. crashes work.
> 
> Jerome Leclanche wrote:
>     So would something like this be acceptable?
>     
>     ```
>     app.setApplicationVersion("0.2");
>     app.setApplicationName("kglobalaccel");
>     app.setApplicationDisplayName(tr("KDE Global Shortcuts Service"));
>     app.setOrganizationDomain("kde.org");
>     ```
>     
>     I guess the tr() would not be accepted, but bringing in k18n because of the *one* string which will almost never appear is a huge shame.
> 
> Martin Klapetek wrote:
>     I'm not sure actually if the display name would be used anywhere in the case of daemon. Very likely in the crash dialog but I can't think of anything else.
>     
>     I can't say if tr() would get rejected though. So I'd suggest to try and update the diff, I'll see if we can get some i18n people to comment.
> 
> Martin Gräßlin wrote:
>     yes, that would be fine. But without creating KAboutData the feature will not work for KCrash:
>     // only for KCrash (no memory allocation allowed)
>     const KAboutData *KAboutData::applicationDataPointer()
>     {
>         if (s_registry.exists()) {
>           return s_registry->m_appData;
>         }
>       return 0;
>     }
>     
>     Without creating a KAboutData instance s_registry will not exist. This means the change would break bug reporting for KDE :-(
> 
> Jerome Leclanche wrote:
>     Is there a way to fix this at a higher level and make KCrash not implicitly depend on KAboutData?

Hmm, from KCrash point of view it's not really implicitly, but rather explicitly. KCrash uses KAboutData and KAboutData has rather many hooks specificly for KCrash as KCrash may not allocate new memory.

Anyway: the usage of Ki18n can be removed as far as I see. KCoreAddons and KCrash is more difficult. I think we agree that KCrash provides required functionality :-)


- Martin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/122680/#review76462
-----------------------------------------------------------


On Feb. 23, 2015, 11:34 a.m., Jerome Leclanche wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/122680/
> -----------------------------------------------------------
> 
> (Updated Feb. 23, 2015, 11:34 a.m.)
> 
> 
> Review request for KDE Frameworks, Martin Gräßlin and Martin Klapetek.
> 
> 
> Repository: kglobalaccel
> 
> 
> Description
> -------
> 
> Remove the runtime's KAboutData
>     
> The about data was unexposed, but created a dependency on KCoreAddons (for
> KAboutData) and in turn on KI18n for the translations of the aboutData.
>     
> This removes both dependencies as well as the string extraction scripts.
> 
> --
> 
> Author notes: This is a RFC. We don't use kglobalaccel in LXQt but we would
> like to, however it currently has too many dependencies. See
> https://github.com/lxde/lxqt/issues/507 for related discussion.
> I'm unsure myself if the about data is actually exposed somewhere I completely
> missed, but it doesn't look that way.
> 
> 
> Diffs
> -----
> 
>   CMakeLists.txt 68ad795 
>   src/Messages.sh 8eae937 
>   src/runtime/CMakeLists.txt e639fa5 
>   src/runtime/Messages.sh 8a5e4a9 
>   src/runtime/globalshortcutsregistry.cpp 3e4d720 
>   src/runtime/kglobalacceld.cpp 4e7cb9d 
>   src/runtime/main.cpp fdf4d62 
> 
> Diff: https://git.reviewboard.kde.org/r/122680/diff/
> 
> 
> Testing
> -------
> 
> Compiles and runs. No further testing done.
> 
> 
> Thanks,
> 
> Jerome Leclanche
> 
>

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