Review Request 119792: Save the view states in addition to the view urls and splitter state in DolphinTabPage.

[Emmanuel Pescosta]

> On Aug. 15, 2014, 1:08 p.m., Frank Reininghaus wrote:
> > Thanks Emmanuel - this is indeed a very nice change, and I can confirm that it works nicely!
> > 
> > The only possible problem that I see is the versioning of the data format that is used by DolphinTabPage::saveState() and DolphinTabPage::restoreState(const QByteArray&). Since the QByteArray is written to disk when saving the session and then read again when the next session is started, we cannot be sure that saving and loading the data is done with the same Dolphin version.
> > 
> > The session management issue has already caused https://bugs.kde.org/show_bug.cgi?id=338187 - that one was easy to solve because the QByteArray simply wasn't there in older versions, so checking in the function if the QByteArray is empty was enough to fix the problem and prevent crashes.
> > 
> > The versioning issue gets more tricky when new items are added to the QByteArray though, in particular if a new entry is inserted between existing entries, like the view state of the first view, which is now in front of the information for the second view.
> > 
> > Moreover, we also have to care about the versioning of the DolphinView state now. Up to now, the QByteArray that keeps the DolphinView state was never written to disk, so we always new what format the data is stored in.
> > 
> > I'm not entirely sure what the best solution is. One could store an int that identifies the stream format version as the first item in the QByteArray. But then we would need special compatibility hacks because Dolphin 4.14.0 does not store such an int in the stream. I'll try to think about it some more, but if you or anyone else has an idea, that would be very welcome as well, of course.
> > 
> > In any case, I think that we might want to add a DolphinMainWindow unit test that calls DolphinMainWindow::readProperties(const KConfigGroup& group) with KConfigGroups that have been created with different versions of DolphinMainWindow::saveProperties(KConfigGroup& group). Maybe even with KConfigGroups that have been modified in some way, because one could argue that we should not get a crash even if the user or a malicious program messed up the session data on disk.

Thanks for your detailed feedback!

Hmm the session data versioning is a real problem, I should have added this in my first QByteArray session management patch :/

But I think we can easily fix it now, by adding an additional property to the config group (in DolphinTabWidget::saveProperties and restoreProperties).
group.writeEntry("Session Data Version", version);

Everytime we change something in our session management code we can increase the "Session Data Version". Also the reading and checking of the session data version is quite straightforward.
The patch should only need a few lines of additional code. (1 line in saveProperties + ~4 lines in restoreProperties + 1 line for the version number) ;)

What do you think?


- Emmanuel


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


On Aug. 14, 2014, 8:40 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119792/
> -----------------------------------------------------------
> 
> (Updated Aug. 14, 2014, 8:40 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Repository: kde-baseapps
> 
> 
> Description
> -------
> 
> Save the view states in addition to the view urls and splitter state in DolphinTabPage.
> 
> Previously closed tabs can now be exactly restored. (same as in e.g. Firefox)
> 
> This is also great for session management, because you can continue your work exactly where you left it. :)
> 
> 
> Diffs
> -----
> 
>   dolphin/src/dolphintabpage.cpp 3d1ba5a 
> 
> Diff: https://git.reviewboard.kde.org/r/119792/diff/
> 
> 
> Testing
> -------
> 
> 1. Expand some folders, set a item somewhere in the list as current item and scroll to a random position (also in split view)
> 2. Close this tab
> 3. Reopen the previously closed tab
> 4. Check if all previously expaned folders are expanded now (same with current item and scroll position)
> 
> Works for me!
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20140816/f1505f51/attachment.htm>