Fix regression in "Open in new tab" behaviour
ClosedPublic

Authored by rade on Mar 23 2018, 7:45 PM.

Details

Summary

In krusader 2.4-beta3 if you was in folder '/home/klaus' and right clicked the folder 'haus' (/home/klaus/haus) and choose "open in new tab" then a new tab would be created with the url /home/klaus/hause open.

In the latest krusader if you do the same then a new tab will be opened with the url /home/klaus and the folder haus in focus.

This patch recreates the krusader 2.4-beta3 behaviour.

Test Plan

Is there a better way too accomplish what I have done? E.g without having to add a new function to listpanel?

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
rade requested review of this revision.Mar 23 2018, 7:45 PM
rade created this revision.
rade updated this revision to Diff 30437.Mar 24 2018, 8:34 PM

I changed the local path behaviour, even though I don't think this code could be called with a remote path, there is no reason why it shouldn't be able to handle it.

rade added a reviewer: nmel.Mar 24 2018, 8:34 PM
rade added a reviewer: martinkostolny.
rade edited the test plan for this revision. (Show Details)Mar 24 2018, 8:38 PM
rade removed reviewers: nmel, martinkostolny.

Is there a better way too accomplish what I have done? E.g without having to add a new function to listpanel?

I think there is a better way, please see my code comment.

And thanks for your effort! :)

krusader/panelmanager.cpp
270

I think, the fix can be done like this with no additional code:

if (nextTo->virtualPath() != url) {
    p->start(url);
}

...instead of line 270.

Please correct me if I've missed something.

rade updated this revision to Diff 30556.Mar 25 2018, 8:23 PM

Updated the code so that start calls openUrl.

rade added inline comments.Mar 25 2018, 8:30 PM
krusader/panelmanager.cpp
270

The reason I choose too not just call start is that start changes the jumpback point. Another issue that is less important is that if start is called with an invalid url, then it navigates to the homefolder. This makes sense for a new tab, because it has to show something. If openUrl is called from an existing tab with an invalid url it does nothing, because it makes more sense to continue showing the folder we are already in.

rade added inline comments.Mar 25 2018, 8:39 PM
krusader/panelmanager.cpp
270

I'm not quite sure I understand the if statement. Is the purpose of 'nextTo->virtualPath() != url' just checking that the url we are trying to open is not the url we are already at? If it is why would it be bad to open it again? Screwing up the back button or something?

martinkostolny added inline comments.Mar 25 2018, 9:24 PM
krusader/panelmanager.cpp
270

Please correct me if I'm wrong but it seems that jumpback point is reset for every new tab that is created (duplicated or not). Even with your diff the jumpback point of the parent tab is not transferred to the new one. I have to be missing something. Anyway I find that as an expected behaviour but I'm not explicitly against changing that :).

As for the invalid url argument, fair enough :).

ad purpose of the if statment: It was there simple in order to not call code that is not necessary to call again. Right I think there probably aren't any bad consequences, I just thought it is logically cleaner this way.

nmel requested changes to this revision.Mar 29 2018, 7:26 AM
nmel added a subscriber: nmel.

I support Martin here. Code seems to be cleaner, jump back is needed for the new tab, url.isValid() will be true in your case. There is no strong reason to create another function.

Martin, could you explain one more time in which scenario the nextTo->virtualPath() == url will happen? Thanks.

This revision now requires changes to proceed.Mar 29 2018, 7:26 AM
rade updated this revision to Diff 30880.EditedMar 29 2018, 8:51 PM

Seems you were right about the jump back being reset martin. I feel into the trap of since I 'knew' it wasn't being reset since my code wasn't calling setjumpback and my cursory (and faulty) test passed and seemed to confirm it wasn't being reset then all was fine and dandy.

I agree nmel, since we are not avoiding resetting jump back and my code more and more started to resemble the full start function code it lost its value and I have reverted to just using start.

I haven't included the virtualPatch check for now because I feel it merits further discussion first. Like could it ever occur and is virtualPath always the same as an that will lead too it, e.g if it's an sftp link leading to the same remote url we are already at?
More importantly I'm not sure I agree that that check belongs in this function. I would argue that if it should be checked it might be better to check it inside the start function, that is if there is no scenario were we would ever want to open an url that is already open.

One thing the virtualPath check did _not_ help against was

ln -s /tmp/a /tmp/a/b

martinkostolny accepted this revision.EditedApr 2 2018, 5:58 PM

Sorry it took me so long.

Martin, could you explain one more time in which scenario the nextTo->virtualPath() == url will happen?

This scenario happen when tab is duplicated (Ctrl+Alt+Shift+N).

It is duplicated in a way that the whole tab settings (of nextTo instance) including url is copied and set to the newly created tab. Inside our function PanelManager::slotNewTab there is a p->restoreSettings(cfg) call which restores all settings to the new tab and also calls func->refresh(). This method is asynchronous (called by a timer in event loop, to be exact) therefore if we also call p->start() and ask if virtualPath or currentUrl of the new tab is already set -> we get empty QUrl as if it is not set. But actually it will get set right away.

So there is basically nothing wrong with calling p->start() every time, but it is not necessary in this case. And checking new url against virtualPath of current panel will not work in start() as well as openUrl() for newly created panels.

I hope I didn't misguide you in any way.

I'm accepting since this patch fixes the issue. I'll gladly discuss any suggestions to improve the new-tab flow.

nmel accepted this revision.Apr 3 2018, 6:25 AM

Thanks Martin for the explanation and Rade for the fix!

This revision is now accepted and ready to land.Apr 3 2018, 6:25 AM
This revision was automatically updated to reflect the committed changes.