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

[Stefan Becker]

> On May 9, 2015, 7:42 p.m., David Faure wrote:
> > 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?

I understood that KConfigGui is responsible for the applications' session configuration. I.e. there are the following situations:

1. application started without -session command line argumen:t KConfigGui::sessionConfig() must never be called. That's what canBeRestored() is all about.
2. application started with -session command line argument: KConfig::sessionConfig() needs to create KConfig object based on that name so that the application can restore its state.
3. session manager emits saveStateRequest signal. At this point the old session config file has become obsolete and a new one needs to be generated -> KConfigGui::sessionConfig(&sm).  The application must file that KConfig object with the new status. If code calls KConfigGui::sessionConfig() it must return the new object.

I agree that if kxmlgui KMainWindow is the only user of KConfigGui then the code could be refactored to squash KConfigGui into KMainWindow. But IMHO that is beyond the scope of this bug/review.


> On May 9, 2015, 7:42 p.m., David Faure wrote:
> > src/gui/kconfiggui.cpp, line 58
> > <https://git.reviewboard.kde.org/r/123705/diff/1/?file=367967#file367967line58>
> >
> >     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?

Moved to the change in review /r/123706 instead


> On May 9, 2015, 7:42 p.m., David Faure wrote:
> > src/gui/kconfiggui.cpp, line 78
> > <https://git.reviewboard.kde.org/r/123705/diff/1/?file=367967#file367967line78>
> >
> >     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).

Thanks, my brain was still stuck in the old code structure, which of course now is obsolete.


- Stefan


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


On May 10, 2015, 10:30 a.m., Stefan Becker wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123705/
> -----------------------------------------------------------
> 
> (Updated May 10, 2015, 10:30 a.m.)
> 
> 
> Review request for KDE Frameworks and Rex Dieter.
> 
> 
> Bugs: 346768
>     https://bugs.kde.org/show_bug.cgi?id=346768
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> When the application receives a saveState signal it needs to replace the current KConfig object with a new one 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/20150510/39a16953/attachment.html>