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

[Michael Pyne]

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



I think I disagree with the idea of overwriting KAboutData properties if they are already set by the user. Alex, any thoughts?

In the event the KAboutData doesn't already exist I think automatically setting it up makes sense, and QCoreApplication is a good source. But I would rather flag property conflicts than to break ties when developer selects two different values for same property, as that's change in behavior might break other parts of code that rely on KAboutData not changing values.

Would this partial solution be OK for the problem you're running into?


src/lib/kaboutdata.h (line 319)
<https://git.reviewboard.kde.org/r/127655/#comment64489>

    I'd rather this say something along lines of:
    
    "If setApplicationData has not been called, the returned KAboutData will be initialized from equivalent information in the QCoreApplication instance, if present. See setApplicationData for the list of properties."



src/lib/kaboutdata.h (line 332)
<https://git.reviewboard.kde.org/r/127655/#comment64488>

    And desktopFileName



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

    I think an else block should be added here that verifies whether the QCoreApplication properties are consistent with KAboutData's and if not, displays a big huge warning for each conflicting property.
    
    I'd rather not initialize KAboutData from QCoreApplication if doing so would overwrite a non-empty KAboutData property, at least at this point. Seems to risk a very spooky 'action-at-a-distance' in  code whose behavior would then depend on when you ran KAboutData::setApplicationData in relation to creating a QCoreApplication.



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

    On other hand, initializing a KAboutData that hasn't already been set from QCoreApplication makes sense. But I would move this into the if block above so that it only runs when a KAboutData wasn't already available.


- Michael Pyne


On April 28, 2016, 1:04 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 April 28, 2016, 1:04 a.m.)
> 
> 
> Review request for KDE Frameworks, Alex Richardson and Michael Pyne.
> 
> 
> Repository: kcoreaddons
> 
> 
> Description
> -------
> 
> KAboutData is passed as values on getting and setting the "applicationData",
> and it only makes sense to have its properties be a transparent access
> to the actual mirrored Q*Application metadata.
>     
> Even more as there is code in KF5 (e.g. KXMLGUI) which relies on KAboutData::applicationData(),
> without requiring the user to use KAboutData::setApplicationData().
> 
> 
> 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/20160429/fa56711d/attachment-0001.html>