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

[Frank Reininghaus]

> On Aug. 15, 2014, 11:08 a.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.
> 
> Emmanuel Pescosta wrote:
>     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?

Hi Emmanuel, yes, this looks like a possible solution. For DolphinView, we should definitely add a version int (which would be 1 at the moment) to the beginning of the stream though. Up to now, we have never written the DolphinView state to disk, so we can change the format now without risking trouble.

For DolphinTabPage/DolphinTabWidget, your solution would work, of course, and it would not require many code changes, but having the version information and the actual data saving/restoring functions in different classes might lead to confusion and trouble in the future. We should really aim to have the version information in the same function where the data is stored/retrieved, i.e., in DolphinTabPage::saveState() and restoreState().

How about this: we add an int for the versioning at the beginning of the data stream in DolphinTabPage, which is 2 initially, and at the same time, we rename the config group entry from, e.g., "Tab 1" to "Tab data 1" or something like that. DolphinTabWidget::readProperties(const KConfigGroup& group) would then first check if it finds a "Tab data 1" entry in the group. If that's not the case, it would try to read a "Tab 1" entry and prepend the data with the int "1", which would encode the (then obsolete) data format which is currently used by Dolphin 4.14.0.

The advantage of this approach is, from my point of view, that the functions in DolpinTabPage look exactly like they should have been written from the beginning (i.e., if we had not overlooked the possible versioning problems), and the hackish compatility code is limited to just one function, namely, DolphinTabWidget::readProperties(const KConfigGroup& group). What do you think?


- Frank


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


On Aug. 14, 2014, 6: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, 6: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/20140817/29c97e28/attachment.htm>