Review Request 123705: Add 2nd variant for KConfigGui::sessionConfig()

[David Faure]

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


With this API change, I wonder if it makes sense for kconfiggui.cpp to retain ownership of the KConfig object. Wouldn't it be safer if the caller got ownership of it, saved into it, and then discarded the KConfig instance? Then we wouldn't have this "lose unsaved changes" code path. Or is the idea that multiple parts of the code could connect to the qApp signal, so they should share the same KConfig instance?


src/gui/kconfiggui.h (line 44)
<https://git.reviewboard.kde.org/r/123705/#comment54978>

    I'm confused by the "Updates" in the docu. To the caller, this returns a new object, no? Actually the implementation also does that, destroying the old one and creating a new one.
    
    Also: missing @since 5.11



src/gui/kconfiggui.cpp (line 29)
<https://git.reviewboard.kde.org/r/123705/#comment54976>

    #define is a bit ugly, how about a file-static function which takes two QStrings and returns a QString?



src/gui/kconfiggui.cpp (line 58)
<https://git.reviewboard.kde.org/r/123705/#comment54979>

    This looks dangerous. Are we 100% sure that this is safe?
    
    To ask this otherwise: how can this config object actually have any uncommitted changes? It's only used during session saving, no?



src/gui/kconfiggui.cpp (line 78)
<https://git.reviewboard.kde.org/r/123705/#comment54977>

    why not just return sessionConfig()->name() ?
    Then there's no need for the separate ensureSessionConfig()  (which always has the risk that someone forgets to call it, after a refactoring).


- David Faure


On May 9, 2015, 3:27 p.m., Stefan Becker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123705/
> -----------------------------------------------------------
> 
> (Updated May 9, 2015, 3:27 p.m.)
> 
> 
> Review request for KDE Frameworks and Rex Dieter.
> 
> 
> Bugs: 346768
>     https://bugs.kde.org/show_bug.cgi?id=346768
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> When application receives a saveState signal it needs to get a KConfig object based on the QSessionManager information. Add a new variant that accepts the session manager object.
> 
> 
> Diffs
> -----
> 
>   src/gui/kconfiggui.h 173400f 
>   src/gui/kconfiggui.cpp 0048c60 
> 
> Diff: https://git.reviewboard.kde.org/r/123705/diff/
> 
> 
> Testing
> -------
> 
> On F22 with kwrite & konsole
> 
> 
> Thanks,
> 
> Stefan Becker
> 
>

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