From kfm-devel Fri Oct 25 12:07:26 2013 From: "Emmanuel Pescosta" Date: Fri, 25 Oct 2013 12:07:26 +0000 To: kfm-devel Subject: Re: Review Request 113293: Restore the tree state in Details View if a top-level folder is collapsed Message-Id: <20131025120726.7497.37038 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kfm-devel&m=138270285908984 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============2939041297966830562==" --===============2939041297966830562== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113293/#review42352 ----------------------------------------------------------- Ship it! Code looks good! I can't test your patch because of the lack of time, but I'm sure it works as expected ;) - Emmanuel Pescosta On Oct. 16, 2013, 10:58 p.m., Frank Reininghaus wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/113293/ > ----------------------------------------------------------- > > (Updated Oct. 16, 2013, 10:58 p.m.) > > > Review request for Dolphin. > > > Bugs: 304363 > http://bugs.kde.org/show_bug.cgi?id=304363 > > > Repository: kde-baseapps > > > Description > ------- > > This patch actually does two things when collapsing an expanded folder with expanded children: > > (a) It removes all expanded children (which are removed from the model) from m_expandedDirs. This fixes some very subtle issues (to reproduce, e.g., expand a/ and a/a/, collapse and re-expand a/, note that a/a/ is not expanded, go to another folder and then go back, and see that a/a/ is now expanded, i.e., that the view state is actually not restored correctly). > > (b) It remembers the expanded children and restores them if the top-level folder is re-expanded. > > Storing the information in the "values" hash is sort of a hack, because this is information that is, unlike the other data stored in this hash, not supposed to be shown in the view. However, I still think that this approach is probably cleaner than all alternatives because it requires changes only in one function. If we stored this information in a new member in KFileItemModel (like a QHash >, where the key is the previously expanded folder, and the value the set of expanded children), we would have to update this member whenever the parent folder is removed from the model (because it is deleted, its own parent gets collapsed, the model is cleared, etc.), so we would need new code in many different places, and remember to update it whenever there is a change in the affected functions. > > > Diffs > ----- > > dolphin/src/kitemviews/kfileitemmodel.cpp ea7ac2f > > Diff: http://git.reviewboard.kde.org/r/113293/diff/ > > > Testing > ------- > > If I expand a top-level folder and then some of its children, then collapse the top-level folder and re-expand it, the previous state is restored correctly. > > > Thanks, > > Frank Reininghaus > > --===============2939041297966830562== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit
This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/113293/

Ship it!

Code looks good!

I can't test your patch because of the lack of time, but I'm sure it works as expected ;)

- Emmanuel Pescosta


On October 16th, 2013, 10:58 p.m. CEST, Frank Reininghaus wrote:

Review request for Dolphin.
By Frank Reininghaus.

Updated Oct. 16, 2013, 10:58 p.m.

Bugs: 304363
Repository: kde-baseapps

Description 

This patch actually does two things when collapsing an expanded folder with expanded children:

(a) It removes all expanded children (which are removed from the model) from m_expandedDirs. This fixes some very subtle issues (to reproduce, e.g., expand a/ and a/a/, collapse and re-expand a/, note that a/a/ is not expanded, go to another folder and then go back, and see that a/a/ is now expanded, i.e., that the view state is actually not restored correctly).

(b) It remembers the expanded children and restores them if the top-level folder is re-expanded.

Storing the information in the "values" hash is sort of a hack, because this is information that is, unlike the other data stored in this hash, not supposed to be shown in the view. However, I still think that this approach is probably cleaner than all alternatives because it requires changes only in one function. If we stored this information in a new member in KFileItemModel (like a QHash<KUrl, QSet<KUrl> >, where the key is the previously expanded folder, and the value the set of expanded children), we would have to update this member whenever the parent folder is removed from the model (because it is deleted, its own parent gets collapsed, the model is cleared, etc.), so we would need new code in many different places, and remember to update it whenever there is a change in the affected functions.

Testing 

If I expand a top-level folder and then some of its children, then collapse the top-level folder and re-expand it, the previous state is restored correctly.

Diffs

  • dolphin/src/kitemviews/kfileitemmodel.cpp (ea7ac2f)

View Diff
--===============2939041297966830562==--