Review Request 127655: Fix KAboutData::applicationData() to init from current Q*Application metadata

[Michael Pyne]

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


Fix it, then Ship it!




A couple of minor things but this will work nicely. Good idea on leaving the comment for things to do in KF6 as well... it's probably better to defer entirely to QCoreApplication, especially since plugins provide their own metadata separately anyways. But that's a discussion for later.


autotests/kaboutdatatest.cpp (line 317)
<https://git.reviewboard.kde.org/r/127655/#comment64572>

    Doesn't this call make the KAboutData::applicationData() call that happens later return *this* "aboutData" object (contrary to its comment)?
    
    Might be better to combine these two test cases into one that sets the Qt data, verifies it's picked up into a KAboutData, and then with a new KAboutData object, calls setApplicationData and verifies that the QCoreApplication data settings are updated.



src/lib/kaboutdata.cpp (line 925)
<https://git.reviewboard.kde.org/r/127655/#comment64571>

    Please either make this a static function or place it in an anonymous namespace, no need to make this symbol visible in the compiled object file or library.


- Michael Pyne


On May 3, 2016, 12:39 a.m., Friedrich W. H. Kossebau wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127655/
> -----------------------------------------------------------
> 
> (Updated May 3, 2016, 12:39 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and Michael Pyne.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> There is code in e.g. KXMLGUI which relies on KAboutData::applicationData(), without requiring the user to use KAboutData::setApplicationData(). So better be complete when initializing the data from the Q*Application metadata.
> 
> Also
> - warn if there is no Q*App instance yet to set properties in KAboutData::setApplicationData
> - check and warn if KAboutData::applicationData is out-of-sync with qapp data
> - remove bogus check for empty display name, as the method defaults to componentname
> - unit tests KAboutDataTest::testSetOfQApplicationData/testPickupOfQApplicationData
> 
> 
> Diffs
> -----
> 
>   autotests/kaboutdatatest.cpp f31e7f3 
>   src/lib/kaboutdata.h 97c0f2b 
>   src/lib/kaboutdata.cpp ceb0c06 
> 
> Diff: https://git.reviewboard.kde.org/r/127655/diff/
> 
> 
> Testing
> -------
> 
> Added autotests pass.
> 
> 
> Thanks,
> 
> Friedrich W. H. Kossebau
> 
>

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