This is indeed the proper fix, since m_primaryViewActive will be set to true again in slotViewActivated(). I also added a test case for this bug.

This will only fix the test if you have a recent enough version of KIO. I wonder if we can find a more robust solution (which we'd also need in the 18.04 branch).

From a quick look it seems there is something strange going on here. I'll investigate in the next days, please don't push it yet.

One more reason to move this stuff from the Dolphin settings into a KCM...

UI looks good now, but I think there is still room for improvements in the code. See inline comment.

In D12327#250786, @rkflx wrote:

In D12327#250761, @elvisangelaccio wrote:

I'm not sure I agree with this change

At least it's not a -1 yet, so maybe we can convince you once everything is in place…!?

afaik Windows does not show the tree view by default in its file dialogs (unless you are referring to FolderBrowserDialog from .Net)

Testing Internet Explorer, Paint and Notepad, all show this detailed view (Explorer too!). Only when switching to the Pictures folder the view is changed to "text under icons". For a non-Microsoft app I tested Firefox, which also has detailed view. If you asked me, I'd say that's the default experience people get on Windows.

I'm not sure I agree with this change, and afaik Windows does not show the tree view by default in its file dialogs (unless you are referring to FolderBrowserDialog from .Net)

In D11245#247354, @michaelh wrote:

In D11245#246952, @elvisangelaccio wrote:

This patch should have bumped the minimum required version of baloo-widgets.

The current baloo-widgets version 5.0.0 (build/baloowidgets_version.h) has not been changed since April 2014 (https://cgit.kde.org/baloo-widgets.git/diff/CMakeLists.txt?id=a62a6a223b7927bcc18c625f436e454b6601255e). Shouldn't that be bumped first?
Contrary to baloo it seems not to be bumped automatically.

In D12094#244029, @sharvey wrote:

I don't believe there's any getting around having two entries in the available actions for toolbar/menu editing. Perhaps one could be given a different label and/or icon? It's awkward because they're both "find"-related actions, but have to be implemented separately.

I get a crash when I try to open the control menu:

This patch should have bumped the minimum required version of baloo-widgets.

+1

As for D11569, make sure we don't break the konqueror test before pushing.

In D12206#246735, @ngraham wrote:

Henrik suggested it:

In D12206#246415, @rkflx wrote:

+1 for only showing the Open With menu, without an additional top app. (But make sure to consolidate the number of separators…)

But I can do that in another patch.

Yes, what I meant was to show only the "Open with" entry for folders.

For the record, see also D10246 and https://git.reviewboard.kde.org/r/127054/

In D11324#228552, @emateli wrote:

In D11324#228467, @elvisangelaccio wrote:

The feature is that you can open a file by typing its name in the url navigator, see e9463ffe2ac11019.

That does sound like a feature we'd be breaking which I was unaware of.

@emateli Sounds like plasma should pass the --select paramater then?

I'm not sure why the shell should be aware on how to launch Dolphin with special parameters when the parameter is a file or a directory. The way it is now is consistent with all DE that i've used so far.

With that in mind however I'd like to propose the following:

• We keep and fix the navigation from the KUrlNavigator. If a directory is typed in then the view will change, otherwise if it's a file, the application will launch it while remaining in the same directory. This IMO should work only for local files (and it's also much easier to implement, at that). If you type some ftp url it should not execute it (but with the current patch it will select it)
• We change the behaviour of the parameter to what the patch proposes. No file execition when launched from the shell but rather selection.
• When requesting to browse a file which is an archive it will be opened inside Dolphin only when the appropriate configuration is active, otherwise it will fallback to the default behaviour(run if via kurlnavigator) otherwise select. (This one I'm less convinced overall, but seems like a more consistent behaviour given that the BrowseThroughArchives setting exists)
  
  Thoughts?

Go for it!

@fvogt @lbeltrame @davidedmundson Any remaining objections?

I didn't notice that this change also affects the context menu of folders. That doesn't make much sense and just clutters the menu, imho.

I also don't see the problem of double click + selection markers and I don't understand why Folder View doesn't show them.

Yes. Thanks!

I don't think there is a logic bug, this is just a missing check for an off-by-one error caused by the places panel (if you hide it, the warning is gone).

Commit message also needs to be updated (it should not mention 'open in new tab' or it should say why we are ignoring it).

I cannot reproduce with Qt 5.11, let's see if someone else can.

In D11703#238754, @elvisangelaccio wrote:

Ok, I was finally able to reproduce the issue. I'll investigate in the next days...

I cannot reproduce the loop of FocusIn events. At step 4. there is only one searchbox (the one in the first tab), the new tab doesn't have a searchbox when I open it.

In D11972#240808, @cfeck wrote:

and is m_renamedItems cleared anywhere before or after renaming?

• Set m_renamedItems also in single-rename case

In D11972#240794, @cfeck wrote:

m_renamedItems is emitted in renamingFinished(). I haven't checked carefully, but is m_renamedItems correct for the single file rename?

+1

I'll have a look this weekend.

New "After" screenshot:

• Yes -> Quit Dolphin

Right. How about "Close All Tabs"? But I'd also be ok with "Quit Dolphin".

Before:

Ok, I was finally able to reproduce the issue. I'll investigate in the next days...

+1

In D11022#238385, @ngraham wrote:

Does it even make sense to have the information panel this wide in the first place? Neither the status quo nor the proposed patch really make sense when it's super wide. Maybe we shouldn't optimize for this extreme corner case.

In D11022#238214, @ngraham wrote:

In D11022#238047, @elvisangelaccio wrote:

In D11022#235434, @xyquadrat wrote:

In D11022#234894, @elvisangelaccio wrote:

If we make all icons of the same size (which I'm not sure I'd like), that code would become useless and should then be removed.

1. I am totally fine with abandoning this patch, but it was brought up on a few occasions on Bugzilla so I simply went ahead & made a patch

The thing is, for one user that complains on bugzilla there are probably 10x people who are happy about the current implementation. Some of them *will* complain after this patch goes in.

What do we suspect that they will complain about?

In D11022#238050, @xyquadrat wrote:

In D11022#238047, @elvisangelaccio wrote:

Still, as I mentioned in bugzilla, the only inconsistency that I'd fix is that the information panel does not check whether the preview setting is enabled.

And what do you think about the size when the information panel is resized? Right now, folders without previews look pretty bad if you expand the panel:

I have another comment on the UI: I don't understand how #name, #bname, and #ext are supposed to work.

In D11022#235434, @xyquadrat wrote:

In D11022#234894, @elvisangelaccio wrote:

If we make all icons of the same size (which I'm not sure I'd like), that code would become useless and should then be removed.

1. I am totally fine with abandoning this patch, but it was brought up on a few occasions on Bugzilla so I simply went ahead & made a patch

There are still a few bits of code to be ported:

Can you explain how to reproduce the "scrolls to an incorrect location" issue?

I'd just remove the code and explain why in the commit message.

Qt minimum version for plasma-integration is 5.9 these days, so this patch should be updated.