[prev in list] [next in list] [prev in thread] [next in thread]
List: kfm-devel
Subject: Re: Review Request 119792: Save the view states in addition to the view urls and splitter state in D
From: "Emmanuel Pescosta" <emmanuelpescosta099 () gmail ! com>
Date: 2014-08-16 12:24:20
Message-ID: 20140816122420.14667.85137 () probe ! kde ! org
[Download RAW message or body]
--===============7367640186581058872==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> 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
>
>
--===============7367640186581058872==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 \
solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;"> \
<tr> <td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/119792/">https://git.reviewboard.kde.org/r/119792/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;"> <p style="margin-top: 0;">On August 15th, 2014, 1:08 p.m. CEST, <b>Frank \
Reininghaus</b> wrote:</p> <blockquote style="margin-left: 1em; border-left: 2px \
solid #d0d0d0; padding-left: 10px;"> <pre style="white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Thanks Emmanuel - this is indeed a very nice change, \
and I can confirm that it works nicely!</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">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.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">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.</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.</p></pre> </blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: \
-pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">Thanks for your detailed feedback!</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Hmm the session data \
versioning is a real problem, I should have added this in my first QByteArray session \
management patch :/</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">But I think we can easily fix it now, \
by adding an additional property to the config group (in \
DolphinTabWidget::saveProperties and restoreProperties).<br style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /> \
group.writeEntry("Session Data Version", version);</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">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.<br style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;" /> 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) ;)</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">What \
do you think?</p></pre> <br />
<p>- Emmanuel</p>
<br />
<p>On August 14th, 2014, 8:40 p.m. CEST, Emmanuel Pescosta wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: \
1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; \
-webkit-border-radius: 6px;"> <tr>
<td>
<div>Review request for Dolphin.</div>
<div>By Emmanuel Pescosta.</div>
<p style="color: grey;"><i>Updated Aug. 14, 2014, 8:40 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kde-baseapps
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" \
style="border: 1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Save the view states in addition to the view urls and \
splitter state in DolphinTabPage.</p> <p style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: inherit;">Previously closed tabs \
can now be exactly restored. (same as in e.g. Firefox)</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This \
is also great for session management, because you can continue your work exactly \
where you left it. :)</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: \
1px solid #b8b5a0"> <tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: \
-moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: \
break-word;"><ol style="padding: 0;text-rendering: inherit;margin: 0 0 0 \
2em;line-height: inherit;white-space: normal;"> <li style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">Expand some folders, set \
a item somewhere in the list as current item and scroll to a random position (also in \
split view)</li> <li style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">Close this tab</li> <li style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Reopen \
the previously closed tab</li> <li style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: normal;">Check if all previously expaned folders \
are expanded now (same with current item and scroll position)</li> </ol>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">Works for me!</p></pre> </td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>dolphin/src/dolphintabpage.cpp <span style="color: grey">(3d1ba5a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/119792/diff/" style="margin-left: \
3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>
--===============7367640186581058872==--
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic