'Re: Review Request 129533: Dolphin: expand all one level, expand all, and collapse all in details v'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-usability
Subject:    Re: Review Request 129533: Dolphin: expand all one level, expand all,  and collapse all in details v
From:       Don Nguyen <don78colorado () gmail ! com>
Date:       2016-12-13 16:16:52
Message-ID: 20161213161652.16852.63895 () mimi ! kde ! org
[Download RAW message or body]

--===============0420518011088471135==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit



> On Dec. 4, 2016, 8:40 p.m., Emmanuel Pescosta wrote:
> > src/kitemviews/kfileitemmodel.cpp, lines 592-600
> > <https://git.reviewboard.kde.org/r/129533/diff/8/?file=486990#file486990line592>
> > 
> > Add a new enum parameter to `setExpanded` to pass on some flags. Add an enum \
> > which has a `RememberPreviouslyExpandedChildren` (or a better name) item. Use \
> > this item as default for the new parameter and only insert the previously \
> > expanded children in `KFileItemModel::setExpanded` if it is requested by the \
> > client. 
> > When you iterate through the list of items, you have to start from the beginning \
> > to avoid the mentioned performance problems. `KFileItemModel::setExpanded` will \
> > automatically remove all child items of the currently collapsed item (the list in \
> > KFileItemModel looks like A, B, B-1, B-2, B-3, C and when you collapse B B-1 to \
> > B-3 will automatically be removed from the list). 
> > In the end the code for collapse-all should look like:
> > 
> > for i = 0 .. count()-1 {
> > setExpanded(i, false, YourFlags::None);
> > }

I implemented your idea but using a bool instead.  Let me know if you still prefer an \
enum.  I had the setExpanded() override call the new setExpanded() with true as it's \
third arg inline.  Let me know if you want me to change this as well.


- Don


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


On Dec. 7, 2016, 9:53 p.m., Don Nguyen wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/129533/
> -----------------------------------------------------------
> 
> (Updated Dec. 7, 2016, 9:53 p.m.)
> 
> 
> Review request for Dolphin and KDE Usability.
> 
> 
> Bugs: 196772
> https://bugs.kde.org/show_bug.cgi?id=196772
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> Implemented functions:  Expand, Expand Full, and Collapse All.  They are available \
> under the View and Control menus as well as custom toolbar buttons as Thomas \
> explained and are disabled when not in details view mode.  They are also available \
> via the context menu only in details view mode.  See screenshots.   Expand - If \
> there is a selection, expand the selection once.  Otherwise expand everything once. \
>  Expand Full - If there is a selection, expand the selected items completely.  \
> Otherwise expand everything completely. Collapse All - Collapse everything.
> 
> 
> Diffs
> -----
> 
> src/dolphincontextmenu.h f67300d 
> src/dolphincontextmenu.cpp ed3f643 
> src/dolphinmainwindow.cpp e5103fd 
> src/dolphinui.rc f197af4 
> src/kitemviews/kfileitemmodel.h 8a0df72 
> src/kitemviews/kfileitemmodel.cpp 05f85a6 
> src/tests/kfileitemmodeltest.cpp 6c45e11 
> src/views/dolphinview.h 0b0d819 
> src/views/dolphinview.cpp 4105628 
> src/views/dolphinviewactionhandler.h eb375b5 
> src/views/dolphinviewactionhandler.cpp 858f929 
> 
> Diff: https://git.reviewboard.kde.org/r/129533/diff/
> 
> 
> Testing
> -------
> 
> make test passes
> manual testing including:
> expand all on an Android SDK folder which yielded in 3180 folders, and 30993 files
> collapse all on above situation
> expand all on a directory that contains a symbolic link to a directory containing \
> itself - operation will stop with error when expansion reaches a path that \
> KCoreDirLister doesn't accept(see looptest screenshot)  changing directories in the \
> middle of an expand all operation changing directories in the middle of a restore \
> expanded directories operation  canceling in the middle of an expand all operation
> canceling in the middle of a restore expanded directories operation
> canceling in the middle of an expand all operation or restore expand directory \
> operation, and then expanding an item manually with mouse click collapse all in the \
> middle of an expand all operation collapse all in the middle of a restore expanded \
> directories operation switched view modes and verified items are enabled only in \
> details view mode 
> 
> File Attachments
> ----------------
> 
> looptest
> https://git.reviewboard.kde.org/media/uploaded/files/2016/11/25/f02b7e85-d0b3-4153-ae0d-e86a4ddfc916__looptest.png
>  context-menu-disabled
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/1fcc4412-e5f2-4c31-92bf-b828eea938f7__contextdisabled.png
>  context-menu-with-selection
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/8a4709e5-d674-4f74-8cbc-a71841db7c36__selectioncontextenabled.png
>  view-menu-enabled
> https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/1ddaa88d-e0aa-441a-a840-5e2cdc06fbf2__viewmenuenabled.png
>  
> 
> Thanks,
> 
> Don Nguyen
> 
> 


--===============0420518011088471135==
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/129533/">https://git.reviewboard.kde.org/r/129533/</a>
  </td>
    </tr>
   </table>
   <br />










<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: \
10px;">  <p style="margin-top: 0;">On December 4th, 2016, 8:40 p.m. UTC, <b>Emmanuel \
Pescosta</b> wrote:</p>  <blockquote style="margin-left: 1em; border-left: 2px solid \
#d0d0d0; padding-left: 10px;">  


<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; \
border-collapse: collapse; margin: 2px padding: 2px;">  <thead>
  <tr>
   <th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; \
font-size: 9pt; padding: 4px 8px; text-align: left;">  <a \
href="https://git.reviewboard.kde.org/r/129533/diff/8/?file=486990#file486990line592" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/kitemviews/kfileitemmodel.cpp</a>  <span style="font-weight: \
normal;">

     (Diff revision 8)

    </span>
   </th>
  </tr>
 </thead>



 
 

 <tbody>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">592</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">    for(int i = count()-1; i &gt;= 0; --i) {</span></pre></td>  </tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">593</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">        if (isExpandable(i) &amp;&amp; isExpanded(i)) {</span></pre></td>  \
</tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">594</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">#ifdef KFILEITEMMODEL_EXPAND_DEBUG</span></pre></td>  </tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">595</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">    //qCDebug(DolphinDebug) &lt;&lt; &quot;collapsing index: &quot; \
&lt;&lt; i;</span></pre></td>  </tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">596</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">#endif</span></pre></td>  </tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">597</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">            expandedItemFound = true;</span></pre></td>  </tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">598</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">            setExpanded(i, false);</span></pre></td>  </tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">599</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">        }</span></pre></td>  </tr>

  <tr>
    <th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" \
align="right"><font size="2"></font></th>  <td bgcolor="#c5ffc4" width="50%"><pre \
style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>  <th \
bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid \
#C0C0C0;" align="right"><font size="2">600</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span \
class="cm">    }</span></pre></td>  </tr>

 </tbody>

</table>

  <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;">Add a \
new enum parameter to <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: inherit;">setExpanded</code> to pass on \
some flags. Add an enum which has a <code style="text-rendering: inherit;color: \
#4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">RememberPreviouslyExpandedChildren</code> (or a better name) item. Use this \
item as default for the new parameter and only insert the previously expanded \
children in <code style="text-rendering: inherit;color: #4444cc;padding: \
0;white-space: normal;margin: 0;line-height: \
inherit;">KFileItemModel::setExpanded</code> if it is requested by the client.</p> <p \
style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: \
inherit;">When you iterate through the list of items, you have to start from the \
beginning to avoid the mentioned performance problems. <code style="text-rendering: \
inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: \
inherit;">KFileItemModel::setExpanded</code> will automatically remove all child \
items of the currently collapsed item (the list in KFileItemModel looks like A, B, \
B-1, B-2, B-3, C and when you collapse B B-1 to B-3 will automatically be removed \
from the list).</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">In the end the code for collapse-all \
should look like:</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">for i = 0 .. count()-1 {  \
setExpanded(i, false, YourFlags::None); }</p></pre>
 </blockquote>





</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; \
white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I \
implemented your idea but using a bool instead.  Let me know if you still prefer an \
enum.  I had the setExpanded() override call the new setExpanded() with true as \
it&#39;s third arg inline.  Let me know if you want me to change this as well.</pre> \
<br />




<p>- Don</p>


<br />
<p>On December 7th, 2016, 9:53 p.m. UTC, Don Nguyen 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 and KDE Usability.</div>
<div>By Don Nguyen.</div>


<p style="color: grey;"><i>Updated Dec. 7, 2016, 9:53 p.m.</i></p>







<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Bugs: </b>


 <a href="https://bugs.kde.org/show_bug.cgi?id=196772">196772</a>


</div>



<div style="margin-top: 1.5em;">
 <b style="color: #575012; font-size: 10pt;">Repository: </b>
dolphin
</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;">Implemented functions:  Expand, Expand Full, and Collapse All.  They are \
available under the View and Control menus as well as custom toolbar buttons as \
Thomas explained and are disabled when not in details view mode.  They are also \
available via the context menu only in details view mode.  See screenshots.   Expand \
- If there is a selection, expand the selection once.  Otherwise expand everything \
once.   Expand Full - If there is a selection, expand the selected items completely.  \
Otherwise expand everything completely. Collapse All - Collapse everything.</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;">make test passes manual testing including:
expand all on an Android SDK folder which yielded in 3180 folders, and 30993 files
collapse all on above situation
expand all on a directory that contains a symbolic link to a directory containing \
itself - operation will stop with error when expansion reaches a path that \
KCoreDirLister doesn&#39;t accept(see looptest screenshot)  changing directories in \
the middle of an expand all operation changing directories in the middle of a restore \
expanded directories operation  canceling in the middle of an expand all operation
canceling in the middle of a restore expanded directories operation
canceling in the middle of an expand all operation or restore expand directory \
operation, and then expanding an item manually with mouse click collapse all in the \
middle of an expand all operation collapse all in the middle of a restore expanded \
directories operation switched view modes and verified items are enabled only in \
details view mode</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>src/dolphincontextmenu.h <span style="color: grey">(f67300d)</span></li>

 <li>src/dolphincontextmenu.cpp <span style="color: grey">(ed3f643)</span></li>

 <li>src/dolphinmainwindow.cpp <span style="color: grey">(e5103fd)</span></li>

 <li>src/dolphinui.rc <span style="color: grey">(f197af4)</span></li>

 <li>src/kitemviews/kfileitemmodel.h <span style="color: grey">(8a0df72)</span></li>

 <li>src/kitemviews/kfileitemmodel.cpp <span style="color: \
grey">(05f85a6)</span></li>

 <li>src/tests/kfileitemmodeltest.cpp <span style="color: grey">(6c45e11)</span></li>

 <li>src/views/dolphinview.h <span style="color: grey">(0b0d819)</span></li>

 <li>src/views/dolphinview.cpp <span style="color: grey">(4105628)</span></li>

 <li>src/views/dolphinviewactionhandler.h <span style="color: \
grey">(eb375b5)</span></li>

 <li>src/views/dolphinviewactionhandler.cpp <span style="color: \
grey">(858f929)</span></li>

</ul>

<p><a href="https://git.reviewboard.kde.org/r/129533/diff/" style="margin-left: \
3em;">View Diff</a></p>



<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">File Attachments \
</h1>


 <li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/11/25/f02b7e85-d0b3-4153-ae0d-e86a4ddfc916__looptest.png">looptest</a></li>


 <li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/1fcc4412 \
-e5f2-4c31-92bf-b828eea938f7__contextdisabled.png">context-menu-disabled</a></li>

 <li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/8a4709e5 \
-d674-4f74-8cbc-a71841db7c36__selectioncontextenabled.png">context-menu-with-selection</a></li>


 <li><a href="https://git.reviewboard.kde.org/media/uploaded/files/2016/12/04/1ddaa88d-e0aa-441a-a840-5e2cdc06fbf2__viewmenuenabled.png">view-menu-enabled</a></li>


</ul>




  </td>
 </tr>
</table>







  </div>
 </body>
</html>


--===============0420518011088471135==--


[prev in list] [next in list] [prev in thread] [next in thread]
Configure | About | News | Add a list | Sponsored by KoreLogic