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

List:       kfm-devel
Subject:    Re: Review Request 123253: dolphin: Navigate to parent folder selects child folder
From:       Frank Reininghaus <frank78ac () googlemail ! com>
Date:       2016-02-11 20:47:35
Message-ID: 20160211204735.7653.94628 () mimi ! kde ! org
[Download RAW message or body]

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


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




src/tests/urlutiltest.cpp (line 45)
<https://git.reviewboard.kde.org/r/123253/#comment62940>

    Passing a path to the QUrl constructor is wrong. This could be done with KUrl, \
but here you should use QUrl::fromLocalPath("/home/test/"), to avoid getting invalid \
QUrl objects.  
    Moreover, if do replace the manual slash search with KIO::upUrl, as I suggested \
in the other comment, then you might have to add trailing slashes to all paths in the \
test.



src/urlutil.h (line 60)
<https://git.reviewboard.kde.org/r/123253/#comment62939>

    Instead of messing around manually with the slashes, you could use KIO::upUrl(), \
something like  
        QUrl firstChildUrl(const QUrl& lastUrl, const QUrl& currentUrl)
        {
            if (!currentUrl.isParentOf(lastUrl)) {
                return QUrl();
            }
    
            QUrl childUrl = lastUrl;
            QUrl parentUrl = KIO::upUrl(childUrl);
            while (parentUrl != currentUrl) {
                // childUrl is not the first child URL yet.
                childUrl = parentUrl;
                parentUrl = KIO::upUrl(childUrl);
    
                if (!currentUrl.isParentOf(childUrl)) {
                    // Not sure if this can happen, but better prevent an infinite \
loop if there is  // a bug in KIO::upUrl().
                    return QUrl();
                }
            }
    
            return childUrl;
        }


- Frank Reininghaus


On Jan. 24, 2016, 4:14 p.m., Gregor Mi wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123253/
> -----------------------------------------------------------
> 
> (Updated Jan. 24, 2016, 4:14 p.m.)
> 
> 
> Review request for Dolphin and Emmanuel Pescosta.
> 
> 
> Bugs: 335616
> https://bugs.kde.org/show_bug.cgi?id=335616
> 
> 
> Repository: dolphin
> 
> 
> Description
> -------
> 
> This is a first working implementation of the feature suggestion filed in the \
> ticket https://bugs.kde.org/show_bug.cgi?id=335616: "Dolphin: Navigate to parent \
> folder selects child folder". 
> In short, this is what is does: Whenever the dolphin view is initialized to show \
> the contents of a new URL (e.g. "/home/x/test") it will be checked if the new URL \
> is a parent of the previous displayed URL (e.g. "/home/x/test/documents/aaa"). If \
> the check is successful, then the common child (in this example: \
> "/home/x/test/documents/") folder item will be selected and scrolled into view. 
> 
> Diffs
> -----
> 
> src/dolphinviewcontainer.h 62f91100e9e5d457edd6f4d927c87610335834d7 
> src/dolphinviewcontainer.cpp 8fea3ba9d0bb8389d89724b9f0cd74605c0286fe 
> src/tests/CMakeLists.txt 22a8b48491fa7ac88ce1b29aecb20192837dd7ea 
> src/tests/urlutiltest.cpp PRE-CREATION 
> src/urlutil.h PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/123253/diff/
> 
> 
> Testing
> -------
> 
> - unit test passes
> - Played around with dolphin: enter URL manually, navigate via click in the item \
> view, navigate via click in kurlnavigator, navigate with Alt+Left, Alt+Right, \
> Alt+up, Backspace 
> 
> Thanks,
> 
> Gregor Mi
> 
> 


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











<div>



<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/123253/diff/3/?file=436493#file436493line45" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/tests/urlutiltest.cpp</a>  <span style="font-weight: normal;">

     (Diff revision 3)

    </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">45</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="n">QCOMPARE</span><span class="p">(</span><span class="n">UrlUtil</span><span \
class="o">::</span><span class="n">firstChildUrl</span><span class="p">(</span><span \
class="n">QUrl</span><span class="p">(</span><span \
class="s">&quot;/home/test/data/documents/muh/&quot;</span><span class="p">),</span> \
<span class="n">QUrl</span><span class="p">(</span><span \
class="s">&quot;/home/test/&quot;</span><span class="p">)),</span> <span \
class="n">QUrl</span><span class="p">(</span><span \
class="s">&quot;/home/test/data&quot;</span><span class="p">));</span></pre></td>  \
</tr>

 </tbody>

</table>

 <div style="margin-left: 2em;">

  <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;">Passing a path to the QUrl constructor is wrong. This could be done with \
KUrl, but here you should use QUrl::fromLocalPath("/home/test/"), to avoid getting \
invalid QUrl objects.</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;">Moreover, if do replace the manual \
slash search with KIO::upUrl, as I suggested in the other comment, then you might \
have to add trailing slashes to all paths in the test.</p></pre>  </div>
</div>
<br />

<div>



<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/123253/diff/3/?file=436494#file436494line60" \
style="color: black; font-weight: bold; text-decoration: \
underline;">src/urlutil.h</a>  <span style="font-weight: normal;">

     (Diff revision 3)

    </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">60</font></th>  <td bgcolor="#c5ffc4" \
width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">    <span \
class="k">static</span> <span class="n">QUrl</span> <span \
class="n">firstChildUrl</span><span class="p">(</span><span class="k">const</span> \
<span class="n">QUrl</span><span class="o">&amp;</span> <span \
class="n">lastUrl</span><span class="p">,</span> <span class="k">const</span> <span \
class="n">QUrl</span><span class="o">&amp;</span> <span \
class="n">currentUrl</span><span class="p">)</span></pre></td>  </tr>

 </tbody>

</table>

 <div style="margin-left: 2em;">

  <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;">Instead of messing around manually with the slashes, you could use \
KIO::upUrl(), something like</p> <p style="padding: 0;text-rendering: inherit;margin: \
0;line-height: inherit;white-space: inherit;"><div class="codehilite" \
style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: \
#008000; font-weight: bold">QUrl</span> <span style="color: #008000; font-weight: \
bold">firstChildUrl</span><span style="color: #666666">(</span><span style="color: \
#008000; font-weight: bold">const</span> <span style="color: #008000; font-weight: \
bold">QUrl</span><span style="color: #666666">&amp;</span> <span style="color: \
#008000; font-weight: bold">lastUrl</span><span style="color: #666666">,</span> <span \
style="color: #008000; font-weight: bold">const</span> <span style="color: #008000; \
font-weight: bold">QUrl</span><span style="color: #666666">&amp;</span> <span \
style="color: #008000; font-weight: bold">currentUrl</span><span style="color: \
#666666">)</span> {
    if (<span style="color: #666666">!</span>currentUrl<span style="color: \
#666666">.</span>isParentOf(lastUrl)) <span style="border: 1px solid \
#FF0000">{</span>  return QUrl();
    }

    <span style="color: #008000; font-weight: bold">QUrl</span> <span style="color: \
#008000; font-weight: bold">childUrl</span> <span style="color: #666666">=</span> \
<span style="color: #008000; font-weight: bold">lastUrl</span><span style="color: \
#666666">;</span>  <span style="color: #008000; font-weight: bold">QUrl</span> <span \
style="color: #008000; font-weight: bold">parentUrl</span> <span style="color: \
#666666">=</span> <span style="color: #008000; font-weight: bold">KIO</span><span \
style="color: #666666">:</span><span style="color: #AA22FF">:upUrl</span><span \
style="color: #666666">(</span><span style="color: #008000; font-weight: \
bold">childUrl</span><span style="color: #666666">);</span>  <span style="color: \
#008000; font-weight: bold">while</span> <span style="color: #666666">(</span><span \
style="color: #008000; font-weight: bold">parentUrl</span> <span style="color: \
#666666">!=</span> <span style="color: #008000; font-weight: \
                bold">currentUrl</span><span style="color: #666666">)</span> {
        <span style="color: #666666">//</span> childUrl is not the first child URL \
yet<span style="color: #666666">.</span>  childUrl <span style="color: \
                #666666">=</span> parentUrl;
        parentUrl <span style="color: #666666">=</span> KIO<span style="color: \
#666666">::</span>upUrl(childUrl);

        if (<span style="color: #666666">!</span>currentUrl<span style="color: \
#666666">.</span>isParentOf(childUrl)) <span style="border: 1px solid \
                #FF0000">{</span>
            <span style="color: #666666">//</span> Not sure if this can happen<span \
                style="color: #666666">,</span> but better prevent an infinite loop \
                if there is
            <span style="color: #666666">//</span> a bug in KIO<span style="color: \
#666666">::</span>upUrl()<span style="color: #666666">.</span>  return QUrl();
        }
    <span style="border: 1px solid #FF0000">}</span>

    <span style="color: #008000; font-weight: bold">return</span> <span style="color: \
#008000; font-weight: bold">childUrl</span><span style="color: #666666">;</span> \
<span style="border: 1px solid #FF0000">}</span> </pre></div>
</p></pre>
 </div>
</div>
<br />



<p>- Frank Reininghaus</p>


<br />
<p>On January 24th, 2016, 4:14 p.m. UTC, Gregor Mi 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 Emmanuel Pescosta.</div>
<div>By Gregor Mi.</div>


<p style="color: grey;"><i>Updated Jan. 24, 2016, 4:14 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=335616">335616</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: inherit;">This is a first working implementation of the feature \
suggestion filed in the ticket https://bugs.kde.org/show_bug.cgi?id=335616: "Dolphin: \
Navigate to parent folder selects child folder".</p> <p style="padding: \
0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In \
short, this is what is does: Whenever the dolphin view is initialized to show the \
contents of a new URL (e.g. "/home/x/test") it will be checked if the new URL is a \
parent of the previous displayed URL (e.g. "/home/x/test/documents/aaa"). If the \
check is successful, then the common child (in this example: \
"/home/x/test/documents/") folder item will be selected and scrolled into \
view.</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;"><ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 \
1em;line-height: inherit;white-space: normal;"> <li style="padding: 0;text-rendering: \
inherit;margin: 0;line-height: inherit;white-space: normal;">unit test passes</li> \
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: \
inherit;white-space: normal;">Played around with dolphin: enter URL manually, \
navigate via click in the item view, navigate via click in kurlnavigator, navigate \
with Alt+Left, Alt+Right, Alt+up, Backspace</li> </ul></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/dolphinviewcontainer.h <span style="color: \
grey">(62f91100e9e5d457edd6f4d927c87610335834d7)</span></li>

 <li>src/dolphinviewcontainer.cpp <span style="color: \
grey">(8fea3ba9d0bb8389d89724b9f0cd74605c0286fe)</span></li>

 <li>src/tests/CMakeLists.txt <span style="color: \
grey">(22a8b48491fa7ac88ce1b29aecb20192837dd7ea)</span></li>

 <li>src/tests/urlutiltest.cpp <span style="color: grey">(PRE-CREATION)</span></li>

 <li>src/urlutil.h <span style="color: grey">(PRE-CREATION)</span></li>

</ul>

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






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







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


--===============6790434185704302857==--


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

Configure | About | News | Add a list | Sponsored by KoreLogic