D25315: KDirModel: implement showing "/" as a root node, optionally

[David Faure]

dfaure added a comment.


  In D25315#619727 , @rrosch wrote:
  
  > > Keep only matters for further calls to openUrl, not the first one. It's 
about whether to *add* or *replace* the currently open URL.
  > >  KDirModel takes care of that.
  >
  > Ah ok, so it's going to do that without me needing to explicitly specify 
"Keep"?
  
  
  Yes. Basically KDirModel is one layer above KDirLister, it encapsulates it. 
If you use KDirModel, you shouldn't have to use KDirLister API directly.
  
  >>> Which should show the node for "/home" as the root, but is instead giving 
me a flat listing of all the child nodes without "/home" as the root.
  >> 
  >> Oh, hmm, that's not how setRootIndex works (that's a *view* feature, we 
can't change that).
  >>  I changed the model to have one more node for "/", while I see now that 
what you want is that it *always* shows a root node even when the root is 
another directory.
  > 
  > Yeah that was my intent with the above. How come it doesn't work in the 
view for `file:///home/myuser` but it works for `file:///`?
  
  I thought the plan was to always show a tree that starts at the root, like 
iirc the old "Root" tab in konqueror-kde4, or the Root tree view in dolphin.
  This is why KDirModel::openUrl("file:///home/myuser", ShowRoot) shows
  
"/"
| - home
| - myuser
  
  > In any case, the code here you provided works great for at least `file:///` 
and that gets us quite far. How much of a rework would getting it to work with 
subdirectories (like `file:///home/myuser`) take?
  
  No idea. I'll work on it and let you know how much work it was :)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25315

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: rrosch, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D25315: KDirModel: implement showing "/" as a root node, optionally

[Raphael Rosch]

rrosch added a comment.


  > Keep only matters for further calls to openUrl, not the first one. It's 
about whether to *add* or *replace* the currently open URL.
  >  KDirModel takes care of that.
  
  Ah ok, so it's going to do that without me needing to explicitly specify 
"Keep"?
  
  >> I couldn't however get the following to work:
  >> 
  >>   QModelIndex index = getIndexFromUrl("/home/myuser");
  > 
  > Invalid URL, that's a path, not a URL. You need QUrl::fromLocalFile().
  
  Sorry that was me using shorthand to illustrate the problem, the url in the 
code is an absolute url stored in a variable with the schema and all the 
necessary things.
  
  >> Which should show the node for "/home" as the root, but is instead giving 
me a flat listing of all the child nodes without "/home" as the root.
  > 
  > Oh, hmm, that's not how setRootIndex works (that's a *view* feature, we 
can't change that).
  >  I changed the model to have one more node for "/", while I see now that 
what you want is that it *always* shows a root node even when the root is 
another directory.
  
  Yeah that was my intent with the above. How come it doesn't work in the view 
for `file:///home/myuser` but it works for `file:///`?
  
  In any case, the code here you provided works great for at least `file:///` 
and that gets us quite far. How much of a rework would getting it to work with 
subdirectories (like `file:///home/myuser`) take?

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25315

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: rrosch, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D25315: KDirModel: implement showing "/" as a root node, optionally

[David Faure]

dfaure planned changes to this revision.
dfaure added a comment.


  In D25315#619436 , @rrosch wrote:
  
  > Tested on patched KF5 5.64, on Fedora 30 (32bit)... root node shows up! I 
had to change the call in my code that was originally:
  >
  >   model->dirLister()->openUrl(QUrl::fromLocalFile("/"), KDirLister::Keep); 
  >
  > [to]   model->openUrl(QUrl::fromLocalFile("/"), KDirModel::ShowRoot);
  >  and don't really know the implications of the change at this point. (Will 
it "Keep"? Does it matter that I now call the model's openURL vs the 
dirLister's?)
  
  
  It's part of the documentation for this change, that you're actually supposed 
to call model->openUrl(). So yes it matters :-)
  
  Keep only matters for further calls to openUrl, not the first one. It's about 
whether to *add* or *replace* the currently open URL.
  KDirModel takes care of that.
  
  > I couldn't however get the following to work:
  > 
  >   QModelIndex index = getIndexFromUrl("/home/myuser");
  
  Invalid URL, that's a path, not a URL. You need QUrl::fromLocalFile().
  
  >   if (index.isValid()) {
  >   treeView->setRootIndex(index.parent());
  >   }
  > 
  >   
  
  
  
  > Which should show the node for "/home" as the root, but is instead giving 
me a flat listing of all the child nodes without "/home" as the root.
  
  Oh, hmm, that's not how setRootIndex works (that's a *view* feature, we can't 
change that).
  I changed the model to have one more node for "/", while I see now that what 
you want is that it *always* shows a root node even when the root is another 
directory.
  
  > (Also, it would be great to have a setRootPath() that works in conjunction 
to expandToUrl or openUrl, to save processing cycles wasted on loading the 
parts of the model that won't be shown in the new rootIndex, like 
https://doc.qt.io/qt-5/qfilesystemmodel.html#setRootPath )
  
  Yes it sounds like that's exactly what's needed in KDirModel. Damn, I need to 
rework all this then.
  
  > As it is relevant, I also tested the patch at 
http://www.davidfaure.fr/2019/kdirmodel_haschildren.diff for `hasChildren()`, 
which works as expected! Thank you!
  
  Thanks, posted as D27731 

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25315

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: rrosch, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D25315: KDirModel: implement showing "/" as a root node, optionally

[Raphael Rosch]

rrosch added a comment.


  Ok, I found one of the "implications". With 
`model->dirLister()->openUrl(m_initURL, KDirLister::Keep);` I can list KIO urls 
like `remote:`, `font:`, and `applications:`, but with 
`model->openUrl(m_initURL, KDirModel::ShowRoot);` I cannot, and only seems to 
display the the local filesystem (specifically it lists "/"). I can probably 
work around it, but just wanted to pass that info along. I think it would be 
great if the "root" could be shown for these as well.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25315

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: rrosch, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D25315: KDirModel: implement showing "/" as a root node, optionally

[Raphael Rosch]

rrosch added a comment.


  Tested on patched KF5 5.64, on Fedora 30 (32bit)... root node shows up! I had 
to change the call in my code that was originally:
  
model->dirLister()->openUrl(QUrl::fromLocalFile("/"), KDirLister::Keep);
  
  to
  
model->openUrl(QUrl::fromLocalFile("/"), KDirModel::ShowRoot);
  
  and don't really know the implications of the change at this point. (Will it 
"Keep"? Does it matter that I now call the model's openURL vs the dirLister's?)
  
  I couldn't however get the following to work:
  
QModelIndex index = getIndexFromUrl("/home/myuser");
if (index.isValid()) {
treeView->setRootIndex(index.parent());
}
  
  Which should show the node for "/home" as the root, but is instead giving me 
a flat listing of all the child nodes without "/home" as the root. Am I doing 
something wrong here? That part is called on a slot for the signal 
`::expand`. (Also, it would be great to have a setRootPath() that 
works in conjunction to expandToUrl or openUrl, to save processing cycles 
wasted on loading the parts of the model that won't be shown in the new 
rootIndex, like https://doc.qt.io/qt-5/qfilesystemmodel.html#setRootPath )
  
  As it is relevant, I also tested the patch at 
http://www.davidfaure.fr/2019/kdirmodel_haschildren.diff for `hasChildren()`, 
which works as expected! Thank you!
  
  ---
  
  For the curious, I just downloaded the source rpm for `kf5-kio`, installed it 
as unprivileged user, edited the .spec file to include the raw diff here as a 
the //second// patch, and the raw diff for D25249 
 as the //first// patch (and copying them 
to `~/rpmbuild/SOURCES/` of course). I also had to change the `Release: ` line 
to something greater than the one already there so it installs without 
conflicts (I changed mine from "1" to "2patched"). Then just
  
rpmbuild -bb ~/rpmbuild/SPECS/kf5-kio.spec 
  
  and install the relevant rpms. No need to rebuild all of KF5 from git (which 
I started to attempt and bailed pretty quickly after thinking this might be the 
easier/faster/less space-using way. Knowing this process will be useful for 
newcomers to help test features on their system without the daunting (and 
probably impossible due to limited space) task of compiling all of KF5. This 
could be added to the dev documentation somewhere.
  
  In any case, thanks again for implementing the feature! (Next step is to try 
to follow the same procedure to test the "crash on close of view" fix you 
submitted for Qt5, thanks also for that!)

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25315

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: rrosch, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D25315: KDirModel: implement showing "/" as a root node, optionally

[David Faure]

dfaure updated this revision to Diff 70183.
dfaure added a comment.


  Add ShowRoot flag, excellent idea!

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D25315?vs=69778=70183

BRANCH
  2019_11_kdirmodel_root

REVISION DETAIL
  https://phabricator.kde.org/D25315

AFFECTED FILES
  autotests/kdirmodeltest.cpp
  autotests/kdirmodeltest.h
  src/widgets/kdirmodel.cpp
  src/widgets/kdirmodel.h
  tests/kdirmodeltest_gui.cpp

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25315: KDirModel: implement showing "/" as a root node, optionally

[Méven Car]

meven added inline comments.

INLINE COMMENTS

> kdirmodel.h:78
> + * Otherwise, this is equivalent to dirLister()->openUrl(url, flags)
> + * @param url   the URL of the directory whose contents should be 
> listed. The item for this directory will NOT be shown, the model starts at 
> its children.
> + *  If the URL is an empty QUrl, the model will display a 
> root "/" node.

Would it make sense to instead of interpreting QUrl(), OpenUrlFlag would 
include a ShowRoot flag ?
I really like enums are they are explicit from the callee perspective.
Then it could call expandAllParentsUntil directly, this would allow in a single 
call to openUrl with a shown root node.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D25315

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D25315: KDirModel: implement showing "/" as a root node, optionally

[David Faure]

dfaure created this revision.
dfaure added reviewers: stefanocrocco, elvisangelaccio, meven, apol.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
dfaure requested review of this revision.

REVISION SUMMARY
  The API for this is to call openUrl(QUrl()), which is understood
  to be the parent of "file:///".
  
  KDirModel has always been lacking a openUrl method anyway, it's always
  been weird to create a KDirModel but then call
  dirModel->dirLister()->openUrl().
  
  This feature has been requested by Raphael Rosch who is reimplementing
  the konqueror sidebar tree on top of KDirModel. Having a visible item
  for "/" is necessary in order to be able to click on it and have it
  listed in the main file view.

TEST PLAN
  kdirmodeltest_gui and 2 new methods in kdirmodeltest (unittest)

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D25315

AFFECTED FILES
  autotests/kdirmodeltest.cpp
  autotests/kdirmodeltest.h
  src/widgets/kdirmodel.cpp
  src/widgets/kdirmodel.h
  tests/kdirmodeltest_gui.cpp

To: dfaure, stefanocrocco, elvisangelaccio, meven, apol
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns