Fix bug where dolphin fails to select any item where a path is created using "Create New | Folder"
Needs ReviewPublic

Authored by emateli on Mar 14 2018, 12:38 AM.

Details

Summary

Selects the first ancestor from the current directory whenever a tree is created using Create New | Folder.
Let me know in case I missed any tests for this otherwise I'll see to it that I try add some.

Test Plan
  • Navigate to a directory
  • Right click, create new -> folder. Use a name as such "a/b/c"
  • Observe that the tree is created, directory a appears in the current directory

Expected: Dolphin selects directory "a"
Before patch: Selection is empty.
Actual: "a" is selected.


Create dir that already exists (~/Desktop)
Create subitem in a dir that exists (~/x exists -> create ~/x/y/z)
Create dir that doesn't exist (~/nothere)
Create path that doesn't exist (~/does/not/exist/yet)

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D11304
Lint
No Linters Available
Unit
No Unit Test Coverage
emateli created this revision.Mar 14 2018, 12:38 AM
Restricted Application added a subscriber: Dolphin. · View Herald TranscriptMar 14 2018, 12:38 AM
emateli requested review of this revision.Mar 14 2018, 12:38 AM
src/views/dolphinview.cpp
1340 ↗(On Diff #29447)

Are we sure m_model->directory() does not already end with a slash?

emateli added inline comments.Mar 15 2018, 10:51 PM
src/views/dolphinview.cpp
1340 ↗(On Diff #29447)

From the looks of it the trailing path delimiter is stripped at KIO's KCoreDirLister, which is what KFileItemModel uses to get the url.

https://cgit.kde.org/kio.git/tree/src/core/kcoredirlister.cpp#n118

elvisangelaccio requested changes to this revision.Mar 18 2018, 10:43 AM
elvisangelaccio added inline comments.
src/views/dolphinview.cpp
1340 ↗(On Diff #29447)

Ok. Then please let's do it like this:

const QString dir = QStringLiteral("%1/").arg(m_model->directory().toString());

Also, which dir? Maybe if we call it modelDir it's a bit clearer.

1343 ↗(On Diff #29447)

name of what? Maybe we can call it ancestorName ?

Also, don't use QDir::separator(), I think it might not work on Windows. From the its doc:

You do not need to use this function to build file paths. If you always use "/", Qt will translate your paths to conform to the underlying operating system. If you want to display paths to the user using their operating system's separator use toNativeSeparators().
This revision now requires changes to proceed.Mar 18 2018, 10:43 AM
emateli updated this revision to Diff 31057.Mar 31 2018, 6:58 PM

Moved the code which finds the child to a separate function + added a test suite for it.

  • add commonancestor function + tests
  • rename to directoryfindchilditem
  • rename test
  • fix ident
  • refactor
  • update moc file
emateli added inline comments.Mar 31 2018, 7:01 PM
src/views/dolphinview.cpp
1816

@elvisangelaccio Without this an item which already existed would not be selected(Say directory contains item a, and a/b/c is created). Not 100% sure if this is the right way to tackle this.

elvisangelaccio requested changes to this revision.Apr 8 2018, 3:14 PM
elvisangelaccio added inline comments.
src/views/dolphinview.cpp
1816

No, it's not. Sadly after this change the dolphin view no longer scrolls the a newly created folder (it does select it, though).

This revision now requires changes to proceed.Apr 8 2018, 3:14 PM
emateli planned changes to this revision.May 27 2018, 6:30 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptMay 27 2018, 6:30 PM
Restricted Application edited subscribers, added: kfm-devel; removed: Dolphin. · View Herald Transcript
rkflx resigned from this revision.Aug 24 2018, 10:48 PM
emateli updated this revision to Diff 47670.Dec 16 2018, 2:08 PM
  • Merge remote-tracking branch 'origin/master' into arcpatch-D11304
  • Fix item would not scroll and selection issue
emateli edited the test plan for this revision. (Show Details)Dec 16 2018, 2:09 PM
emateli updated this revision to Diff 47671.Dec 16 2018, 2:12 PM
  • remove qdebug include
  • remove unnecessary prefixes

Oops, sorry I missed this!

In the case that the user creates a/b/c, I think what would actually be most expected is to change the view to show a/b/, and then select c.