Differential D9610

Revive folderpanel when outside $HOME
ClosedPublic
Actions

Authored by michaelh on Jan 2 2018, 2:30 PM.

Details

Reviewers

elvisangelaccio

Group Reviewers

Dolphin

Commits

R318:7b595b3387cd: Revive folderpanel when outside $HOME

Summary

Currently the folderpanel is inert when browsing outside of home because urls end up as 'file:////a/b/c'

Premptive bug fix.

Test Plan

Open dolphin
Leave home directory

Diff Detail

Repository

R318 Dolphin

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

michaelh requested review of this revision.Jan 2 2018, 2:30 PM

michaelh created this revision.

These are in fact the first 5 lines I've ever written in C++. So please be patient if my code is clumsy, ugly or just wrong.

BTW, I did not understand the comment following my lines. "first subdir ... does not end with" ?

anthonyfieroni added a subscriber: anthonyfieroni.Jan 2 2018, 2:50 PM

anthonyfieroni added inline comments.

src/kitemviews/kfileitemmodel.cpp
637–638	QUrl urlToExpand = m_dirLister->url().ajusted(QUrl::StripTrailingSlash);

ngraham added a subscriber: ngraham.Jan 2 2018, 2:59 PM

Just tested it. It's not working.
url().adjusted(QUrl::StripTrailingSlash) does not strip from 'file:///'

src/kitemviews/kfileitemmodel.cpp
637–638	Just tested it. It's not working. url().adjusted(QUrl::StripTrailingSlash) does not strip from 'file:///'

anthonyfieroni, I was so much involved in figuring out what to do next, I forgot to thank for your comment.
So: Thanks for your comment.

Thanks for the patch. Can you explain exactly what is not working and how to reproduce the bug?

In D9610#184929, @michaelh wrote:

BTW, I did not understand the comment following my lines. "first subdir ... does not end with" ?

Please check https://phabricator.kde.org/D7477

When navigating outside home

panels/folders/folderspanel.cpp:

332 baseUrl = QUrl::fromLocalFile(QDir::rootPath());       // = "file:///"

this eventually becomes

m_dirLister->url()       // = "file:///"

the last slash is not a trailing slash but the root directory, consequently

url().adjusted(QUrl::StripTrailingSlash)

does not remove it (I guess).

651  urlToExpand.setPath(urlToExpand.path() + '/' + subDirs.at(i));

appends a 4th '/' resulting in "file:////<subdir>" which does not exist.

That bug can be reproduced by navigating outside your home folder.
folderspanel does not expand the folder you navigated to.

With patch applied

anthonyfieroni added inline comments.Jan 2 2018, 6:49 PM

src/kitemviews/kfileitemmodel.cpp
637–638	QUrl::StripTrailingSlash will strip in loop all '/' that why it's not working. Thanks for testing.
645–649	You can add check here. QString path = urlToExpand.path(); if (!path.endsWith(QLatin1Char('/')) { path.append(QLatin1Char('/')); } urlToExpand.setPath(path() + subDirs.at(i));

src/kitemviews/kfileitemmodel.cpp
645–649	At this point is there still a possibility that urlToExpand.path() ends with a slash? Under what conditions? I was wondering why the url is split with QDir::separator but joined again with '/'. And now it's QLatin1Char('/'). Why is that?

anthonyfieroni added inline comments.Jan 2 2018, 7:37 PM

src/kitemviews/kfileitemmodel.cpp
645–649	It's not possible to ends with '/' at that point but i'm searching for solution without setting empty paths urlToExpand.setPath(path.left(pos - 1)); will set path to empty string.

michaelh added inline comments.Jan 2 2018, 7:56 PM

src/kitemviews/kfileitemmodel.cpp
645–649	What about enforcing a trailing slash in panels/folders/folderspanel.cpp and use urlToExpand.setPath(urlToExpand.path() + subDirs.at(i) + '/' ? void KFileItemModel::expandParentDirectories(const QUrl &url) is used only once anyway

@elvisangelaccio, the problem is file:/// + / + subdir result in invalid path. With @dfaure we make a patch in KIO does we should do it here?

Ah right, this is yet another regression from the Qt 5.10 fallout.

In D9610#185157, @anthonyfieroni wrote:

@elvisangelaccio, the problem is file:/// + / + subdir result in invalid path. With @dfaure we make a patch in KIO does we should do it here?

That's not exported, we cannot use it from Dolphin.

src/kitemviews/kfileitemmodel.cpp
645–649	Here we need to append the slash only if the `path()` does not end with a slash already. This fixes the bug for me.

This revision now requires changes to proceed.Jan 3 2018, 10:32 AM

In D9610#185387, @elvisangelaccio wrote:

That's not exported, we cannot use it from Dolphin.

Yeah, i mean in same way.

I've incorporated your suggestion and removed most of my code. I tested the code and it works .
But now I'm stuck here https://phabricator.kde.org/differential/diff/24627/
The only option I have on that page is to create a new revsion. But I want to update this one.
Shall I proceed? Or will I mess things up when I do?

In D9610#185444, @michaelh wrote:

I've incorporated your suggestion and removed most of my code. I tested the code and it works .
But now I'm stuck here https://phabricator.kde.org/differential/diff/24627/
The only option I have on that page is to create a new revsion. But I want to update this one.
Shall I proceed? Or will I mess things up when I do?

Use the "Update diff" button and upload the new patch.

I revisited that page. Et voila: now I can update. Which I did.
Strange! Yesterday I could not (maybe it was my firefox).
Do I need to check any of the "Done" checkboxes?

elvisangelaccio accepted this revision.Jan 3 2018, 1:08 PM

This revision is now accepted and ready to land.Jan 3 2018, 1:08 PM

Closed by commit R318:7b595b3387cd: Revive folderpanel when outside $HOME (authored by michaelh, committed by elvisangelaccio). · Explain WhyJan 3 2018, 1:11 PM

This revision was automatically updated to reflect the committed changes.

Is that it?

In D9610#185446, @michaelh wrote:

Do I need to check any of the "Done" checkboxes?

That's a good practice :)

michaelh marked an inline comment as done.Jan 3 2018, 1:13 PM

I want to keep going. Is it correct to do

git pull
(work)
arc diff

for the next revision?

In D9610#185460, @michaelh wrote:

I want to keep going. Is it correct to do

git pull
(work)
arc diff

for the next revision?

In general, yes. And thanks for your work, btw!

For the record:

git pull
(work)
arc diff

did not work.

Propable reasons:

no developer account
fetching the code with kdesrc-build

What did work was:

git checkout origin/Applications/17.12

In D9610#185636, @michaelh wrote:
For the record:

git pull
(work)
arc diff

did not work.

Propable reasons:

no developer account

fetching the code with kdesrc-build

What did work was:
git checkout origin/Applications/17.12

Oh yes, sorry about that. The reason is that this commit has not been merged to the master branch, yet.
So ifyou need to do work on top of it, indeed you need to checkout the stable branch (which currently is Applications/17.12).

As I said I'm a total newbie in this field, so trying to work such things out helps to understand the inner workings of git et al. The downside of being ignorant: mistakes. And apparently I made one. After checking out origin/Applications/17.12
I compiled again and sadly dolphin it did not work as it should. I've fixed that already (in case you're curious: QString path has to be declared outside the loop) but before submitting it I have to sort out the mess of revisions I made here. I'll submit it within the next 3 days.

Side note:
I'm on opensuse Tumbleweed and as a consequence suffering under this bug since 2 weeks or so. On the other hand that made me approach C++ (shiverrrr). And hey, now I'm using my own fixed compiled dolphin.

To wrap things up:

$ git  checkout -b otto origin/Applications/17.12
$ git branch
    master
  * otto

In new KDevelop session:

import src/CMakeLists.txt
build
install (to ~/bin/otto/)
run in Debugger -> Dolphin works as expected

Then:

$ ~/bin/otto/dolphin 
Trying to convert empty KLocalizedString to QString.
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/lastmonth")
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/thismonth")
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/yesterday")
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/today")
org.kde.dolphin: Ignore KIO url: QUrl("search:/images")
org.kde.dolphin: Ignore KIO url: QUrl("search:/documents")
org.kde.dolphin: Ignore KIO url: QUrl("search:/videos")
org.kde.dolphin: Ignore KIO url: QUrl("search:/audio")
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/lastmonth")
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/thismonth")
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/yesterday")
org.kde.dolphin: Ignore KIO url: QUrl("timeline:/today")
org.kde.dolphin: Ignore KIO url: QUrl("search:/images")
org.kde.dolphin: Ignore KIO url: QUrl("search:/documents")
org.kde.dolphin: Ignore KIO url: QUrl("search:/videos")
org.kde.dolphin: Ignore KIO url: QUrl("search:/audio")
qt.accessibility.core: Cannot create accessible child interface for object:  PlacesView(0x23ca670)  index:  21

-> Dolphin works as expected

Just out of curiosity. Would tell me why you tried to avoid an empty path in

urlToExpand

Shouldn't this be merged to master?

Revision Contents
Changeset List

			Path	Packages
M			src/kitemviews/kfileitemmodel.cpp (8 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	24592	0f3a36e		Jan 2 2018, 2:11 PM	★	★
Diff 2	24627	0f3a36e		Jan 2 2018, 9:07 PM	★	★
Diff 3	24662	9d3a019	R318:7b595b3387cdb002c3e9a11e98a95d8801ca078d	Jan 3 2018, 1:07 PM	★	★

Commit	Tree	Parents	Author	Summary	Date
fa72b94db6dd	0c4b352f7a44	0f3a36e6d3d4	super	normalize trailing path separator	Jan 2 2018, 2:13 PM

Diff 24662

View Options

src/kitemviews/kfileitemmodel.cpp