Fix wrong window titles
ClosedPublic

Authored by nazark on Sep 3 2019, 3:21 PM.

Details

Summary

BUG: 389135

Currently dolphin shows a folder's name instead of a "place" name if the current path has trailing slash and place's name doesn't, or vice versa.
This patch fixes this behavior, so that dolphin always shows a "place" name.

Test Plan

Test configuration: https://imgur.com/a/U4zBp8c
Before (wrong window titles are in red): https://imgur.com/a/dB4xRwo
After (all window titles are correct): https://imgur.com/a/W7virBn

Diff Detail

Repository
R318 Dolphin
Lint
Lint Skipped
Unit
Unit Tests Skipped
nazark created this revision.Sep 3 2019, 3:21 PM
Restricted Application added a subscriber: kfm-devel. · View Herald TranscriptSep 3 2019, 3:21 PM
nazark requested review of this revision.Sep 3 2019, 3:21 PM
nazark edited the summary of this revision. (Show Details)
ngraham added a subscriber: ngraham.Sep 3 2019, 3:27 PM

Wasn't this just recently fixed with D23654? Can you check?

nazark added a comment.EditedSep 3 2019, 3:41 PM

As far as I understand, nope, that commit just changes the url which is going to be added to "places".
Firstly, it doesn't fix the bug, and secondly, a user still can add/modify a url in places so that it contains a trailing slash, and current version of dolphin will show it's name wrong (if opened manually, as on the screenshots in "Test Plan").

In that case it would seem like a simpler approach might be to just have url() always strip the trailing slash, rather than adding a bunch of complex code to detect, parse, and handle it.

nazark added a comment.EditedSep 3 2019, 3:47 PM

Yep, that's exactly what I did to fix that the first time I tried to write a patch!
The problem is, it doesn't solve it.
It will show the name of a folder wrong, if its url in places contains trailing slash (and it's easy to do, the patch that you linked still allows users to manually create/edit such urls in "places").
(The version which just always strips the trailing slash: https://i.imgur.com/QR0GGnx.png)

elvisangelaccio requested changes to this revision.Sep 14 2019, 12:47 PM

Please change line 459 to use the following code instead:

const auto& matchedPlaces = placesModel->match(placesModel->index(0,0), KFilePlacesModel::UrlRole, url().adjusted(QUrl::StripTrailingSlash), 1, Qt::MatchStartsWith);

i.e. always strip the trailing slash and check whether the place URL starts with the stripped URL.

This revision now requires changes to proceed.Sep 14 2019, 12:47 PM
nazark updated this revision to Diff 66065.Sep 14 2019, 4:46 PM

Updated!
I didn't think about this solution, thanks, clearly it's way better than mine.

Actually this introduces a regression, sorry about that. dolphin / will now show "Home" as window title...

Will have to think more about it. Maybe some regex could help us.

Oh, and you are right again.
The previous solution seems to fix it without a regression, but obviously the code itself feels at the very least "not good".

nazark added a comment.EditedSep 14 2019, 5:43 PM

What about something like this?

const auto& matchedPlaces = placesModel->match(placesModel->index(0,0), KFilePlacesModel::UrlRole, QUrl(url().adjusted(QUrl::StripTrailingSlash).toString().append("/?")), 1, Qt::MatchRegExp);

I did a few tests, and it seems like it does what it is supposed to do.

nazark updated this revision to Diff 66075.Sep 14 2019, 5:47 PM

Nice, this seems to do the trick :)

src/dolphinviewcontainer.cpp
459

We should probably use toString(QUrl::FullyEncoded), since this is how the URL is going to end up in the bookmark XML file used by the places model.

nazark updated this revision to Diff 66135.Sep 15 2019, 3:55 PM
nazark marked an inline comment as done.
elvisangelaccio accepted this revision.Sep 15 2019, 6:40 PM
This revision is now accepted and ready to land.Sep 15 2019, 6:40 PM
This revision was automatically updated to reflect the committed changes.