Review Request 123253: dolphin: Navigate to parent folder selects child folder

Mon Feb 15 19:07:15 GMT 2016

> On Jan. 29, 2016, 3:24 p.m., Frank Reininghaus wrote:
> > What I still do not understand is why you want to make this change in Dolphin, and not in KUrlNavigator, as I have suggested in another comment here some time ago.
> > 
> > Your patch will make the "go up" behavior inconsistent in Dolphin and the file dialog, which is rather bad for the user experience IMHO. If you change KUrlNavigator, then everything will be consistent, and code maintenance will be easier. Moreover, the risk of subtle bugs (by the two layers of history handling which your patch introduces) will be lower.
> 
> Gregor Mi wrote:
>     Hi, in some earlier comments I noted that I had difficulties implementing this in KUrlNavigator (can't remember why excactly, but I could look it up later).
>     
>     I share your concern with the inconsistency issue between dolphin and file dialog. Right now, I see this as a lower priority because...
>     
>     1) it was not sure if the feature was accepted at all and I wanted to present a working implementation.
>     2) I think most non-power users won't notice the feature at all.
>     3) In my personal experience "heavy-duty" navigating takes place in dolphin but not in the file dialog. So, the lack of the feature there would not be that bad. 
>     
>     That said, I think it should indeed be moved to KUrlNavigator or some other shared location to make the behaviour consistent, if time proves that the new behaviour is good to keep.
> 
> Frank Reininghaus wrote:
>     > Hi, in some earlier comments I noted that I had difficulties implementing this in KUrlNavigator (can't remember why excactly, but I could look it up later).
>     
>     I looked it up. This was because there are different ways to change the behavior concerning "Navigating to parent folder":
>     
>     1. Assume that /home/user is shown in the view, then the user clicks /home/user/subfolder to navigate to "subfolder", then clicks "up" to go to the home folder again. Currently, the view will then show /home/user scrolled to the top, but it seems that some users expect that it should be scrolled to "subfolder", and "subfolder" should be the active item. Essentially, this could be achieved by interpreting "go up" as "go back" if the "up" URL is that same URL that would be reached by the "back" action. This would be easy to implement in KUrlNavigator.
>     
>     2. What you want is something different: If /home/user is shown in the view, and then "/" is entered in the location bar, then you want that "home" is selected. This is difficult to implement in KUrlNavigator.
>     
>     Now the question is if solution 2 is really so much better than solution 1 that inconsistencies and more complex code (the two different kinds of history handling in KUrlNavigator and in DolphinViewConainer's m_lastUrl still look wrong to me) are justified. I think I would actually find behavior 2 confusing, to be honest. For example, if "/" is shown in the view, /usr is the current item, and I click "Home" in the Places, and then click "Back": Currently, the original state of the "/" view would be restored, i.e., /usr would still be the current item, and I think that this is what would be expected from the "Back" action: go back, and show the view just like it was. This is what the history is for IMHO. With your patch, however, /home would be the current item in this situation. This
  looks wrong to me.
>     
>     > 1) it was not sure if the feature was accepted at all and I wanted to present a working implementation.
>     > 2) I think most non-power users won't notice the feature at all.
>     > 3) In my personal experience "heavy-duty" navigating takes place in dolphin but not in the file dialog. So, the lack of the feature there would not be that bad.
>     
>     I don't see how any of this justifies inconsistencies between the behavior of Dolphin and the file dialog.
>     
>     > That said, I think it should indeed be moved to KUrlNavigator or some other shared location to make the behaviour consistent, if time proves that the new behaviour is good to keep.
>     
>     This might not be possible at all. Once Dolphin is released with a modified behavior that adds some custom history handling on top of the one in KUrlNavigator, then any future change in KUrlNavigator's behavior might interact badly with the modifications in Dolphin. Note that both have different release schedules, so it would not be possible to fix this situation by reverting the modifications in Dolphin.
> 
> Frank Reininghaus wrote:
>     I think that I understand now what our (maybe mostly my) confusion is about. I tested the behavior of the Windows 7 Explorer in the mean time, and I found that there is a crucial difference between what it does and what you propose here.
>     
>     As Heiko said in http://marc.info/?l=kde-usability&m=140273827601672&w=2  navigating from c:\windows\system32\drivers to c:\ will select 'windows' because it is an indirect parent of the previous folder, but I found that this happens **only if the navigation did not involve the history, i.e., it was not initiated by a 'back' or 'forward' action**. Then the conflicts between the existing history handling (managed by KUrlNavigator) and your proposed feature (which is also a sort of history handling) that I'm worried about cannot occur.
>     
>     BTW, just to show you that my worries are justified, here is a simple example that causes a rather odd behavior with your patch:
>     
>     1. Go /usr/include (or another folder with many subfolders, make sure that the subfolders fill multiple pages).
>     2. Add a subfolder to the Places (e.g., by dragging it to the Places Panel). I'll take the 'c++' subfolder.
>     3. Press the down arrow key and hold it until you have scrolled down the view a few pages. I'll keep the key pressed until the 'X11' subfolder is the current item.
>     4. Click the created 'Place' in the panel (in my case, 'c++').
>     5. Click the 'Back' button in the toolbar.
>     6. Now you will be in /usr/include, and the view is scrolled to the old position, i.e., X11 is visible, but c++ is the current item - if you press 'up', then the view will scroll up suddenly.
>     
>     This is due to a conflict between the history handling and your feature: the scoll position is restored from the history, but not the current item, which is set by your 'firstChildUrl' handling.
>     
>     If you modify your feature such that it really does what Windows 7 does, then I will withdraw my objections :-)
>     
>     I think that this should really be solved in KUrlNavigator. If it detects that a URL change was not a history action, such that it cannot provide view data that could be restored, AND the new URL is a parent of the previous URL, then it could emit a signal like selectParentUrl(QUrl), which Dolphin and other users of KUrlNavigator (such as the file dialog) could connect to.

+1 to everything :)

> here is a simple example that causes a rather odd behavior with your patch:

Thanks for this. I didn't try this before. This a good way to verify if the changes work correctly.

> the scoll position is restored from the history, but not the current item, which is set by your 'firstChildUrl' handling.

I can reproduce it. This is definitely not how it should be.

> I think that this should really be solved in KUrlNavigator. If it detects that a URL change was not a history action, such that it cannot provide view data that could be restored, AND the new URL is a parent of the previous URL, then it could emit a signal like selectParentUrl(QUrl), which Dolphin and other users of KUrlNavigator (such as the file dialog) could connect to.

I'll give it another try.

- Gregor

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

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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20160215/306aeaaf/attachment.htm>