Almost there!

I still don't understand if we are fixing an issue or if this is just a removal of a redundant signal. AFAICS we are only removing a selectionChanged() instance called by clearSelection() in "search mode", but this signal is also emitted in many other places (e.g. is emitted 3 times if I change selection with the mouse).

More info on the i18n markers here: https://api.kde.org/frameworks/ki18n/html/prg_guide.html#good_ctxt

In D23557#543977, @meven wrote:

In D23557#543918, @elvisangelaccio wrote:

@meven Looks like this patch is not enough: https://build.kde.org/job/Applications/job/dolphin/job/kf5-qt5%20SUSEQt5.12/78/testReport/projectroot.src/tests/placesitemmodeltest/

Could you have a look?

It seems to me the test fails because is uses an older kio version than 5.63 with D7446 applied creating "Recently Saved" group instead of group "Recent".

FAIL! : PlacesItemModelTest::testGroups() Compared values are not the same

Actual   (groups.at(2).second.toString()): "Recently Saved"
Expected (QStringLiteral("Recent"))      : "Recent"

The actual shows that the old "Recently Saved" group was created when it should not have been.

I don't know how this jenkins job works, but I would recommend cleaning the cached artifact KIO 5.63.0 for dolphin somehow.
We can alternatively limit this test run only on KF5 >= 5.64

@meven Looks like this patch is not enough: https://build.kde.org/job/Applications/job/dolphin/job/kf5-qt5%20SUSEQt5.12/78/testReport/projectroot.src/tests/placesitemmodeltest/

+1 for the idea.

This is perfect, I love it!

In D20532#542673, @fbg13 wrote:

In D20532#542522, @elvisangelaccio wrote:

Adding a new setting makes the code and the UI more complex and harder to maintain in the long term. We should add new settings only when there is no other way (this usually happens if a new feature makes sense only for a subset of users).

But even if we go with just the "Go up" action, I think there should be an option to turn it off.
I often miss the file or folder when double clicking and going up would be annoying.

Thanks

Would you mind to move the isIndexingEnabled() refactoring to another commit?

In D24362#542578, @meven wrote:

I almost feel like we would need to add a setting to hide the hidden count, as at least some users will be annoyed by this, in particular those using local view settings with .directory files.

Another thing that I forgot: the double click trigger should only be enabled in single-click mode, probably.

Yep, the i18n issue if fixed now.

Patch seems to work fine, but the lack of commit message makes it hard to understand where the actual bugfix is. Please simplify the patch as mentioned inline.

In D24362#541836, @meven wrote:

In D24362#541694, @elvisangelaccio wrote:

In D24362#541173, @ngraham wrote:

Well, without this patch it's already showing wrong information. "There are only 3 things in here, why does the information panel say there are 7?" Hence the bug report from a user.

Sure, the current behavior is also wrong. But why switch from wrong to wrong when there is a way to make it right? ;)

The switch moves from wrong to right in most common cases.

This is what I get in the shortcuts dialog:

@elvisangelaccio, if you would like to revert that commit to keep the history cleaner I can rebase this one.

In D24362#541173, @ngraham wrote:

Well, without this patch it's already showing wrong information. "There are only 3 things in here, why does the information panel say there are 7?" Hence the bug report from a user.

In D24362#541135, @ngraham wrote:

In D24362#541125, @elvisangelaccio wrote:

Why? What' wrong with always showing "X items (Y hidden)" ?

I don't hate this idea, but for the average user I don't think it adds any value to show the number of hidden files, since they're hidden. The average user doesn't even know that there are hidden files. It's just noise.

I'll push to 19.08, thanks for the patch!

LGTM

In D24362#540790, @meven wrote:

I think we should include the number of hidden files only when hidden files are shown and then include them in the total count.

Why not assign a default shortcut? Alt + i seems to be the standard pattern for this (tried with Firefox and Chrome).

Thanks

@hallas I noticed the following warning on dolphin start:

Sorry for the long delay. I don't think we should add an option to let the user choose what to do on double clicks.

Oh, and there is a missing BUG: 371837 in the commit message ;)

While the Confirmation tab is not perfect, I don't think that moving this setting to the Behavior or Navigation tabs would be much better.

LGTM now, let's ship it so that we can move forward with the arrows patches.

https://git.reviewboard.kde.org/r/129383/ had a patch for this but it never got merged. Copying the diff here in case anyone wants to take over:

So should we close https://bugs.kde.org/show_bug.cgi?id=376992 ? ;)

LGTM now. Ship it! :)

Thanks, cool stuff :)

Sorry for the delay.

Feel free to push after fixing the last two nitpicks.

It seems to me that making the URL navigator buttons more discoverable and moving the URL navigator to the toolbar are two orthogonal things.

Needs one more rebase, sorry

Almost there!

Mention that URLs will be stored in the QUrl::FullyEncoded string format.

• Fixed typo

Yeah, that's my next patch ;)

In D23955#531647, @dfaure wrote:

OK (why does this matter to the user of the class, though?)

Nice, this seems to do the trick :)