Enable adding place as first child
AbandonedPublic

Authored by gvgeo on Jan 29 2020, 10:53 AM.

Details

Reviewers
meven
broulik
dfaure
ngraham
Group Reviewers
Frameworks
Summary

When index is null, move new bookmark into first place, by sending null in moveBookmark.

Also style was fixed for KBookmark bookmark line, before copying on top.

Test Plan

Before:
Adding a place as first child will place it second.

After:
A new place, will always get the correct index.

Test:
Apply D27032 for dolphin.
Add a new place, by dragging a folder at the top, bottom and a random
position of the places panel.
Should take the position indicated.
Existing bugs:
1 Dragging folder into empty space does not work.
2 Attempt to place the indicator outside of 'Places' will trigger an
other bug, and will place it in a random position.
3 Adding a place while hovering the same existing folder(shows
denied red icon), won't work.

Add two new places by right clicking a folder. (Option isn't available
if folder is already in places.)
Should take last position.

Try to break:
Move/add places around (and outside of 'Places').
Hide/show entries.
Try the bugs.
Test again.

Checking other programs:
If right-click adds a place at the top, they need patch.

Diff Detail

Repository
R241 KIO
Branch
after (branched from master)
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 21889
Build 21907: arc lint + arc unit
gvgeo created this revision.Jan 29 2020, 10:53 AM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 29 2020, 10:53 AM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
gvgeo requested review of this revision.Jan 29 2020, 10:53 AM

Marked as WIP, cause I don't know how to avoid code duplication.

Don't know what other places, that expect -1 to be last, need fix.
Only have D26984 for dolphin, which is the reason I made this patch.

gvgeo edited the summary of this revision. (Show Details)Jan 29 2020, 11:02 AM
gvgeo added a reviewer: Frameworks.
meven requested changes to this revision.Jan 29 2020, 4:10 PM
meven added inline comments.
src/filewidgets/kfileplacesmodel.cpp
1046–1052

It seems to me you don't need to edit this function.

This revision now requires changes to proceed.Jan 29 2020, 4:10 PM
gvgeo added a comment.Jan 29 2020, 4:43 PM

Although the target of this patch is the else part below, there is a need to add a place at the end.

src/filewidgets/kfileplacesmodel.cpp
1046–1052

As it is, with a null QModelIndex, the function below will move the new item to the top.

meven accepted this revision.Jan 29 2020, 6:14 PM
meven added a reviewer: dfaure.

I am not a Frameworks reviewer
Please give some time to others to have a look as well

src/filewidgets/kfileplacesmodel.cpp
1046–1052

I missed this, you could pass the current modelIndex or the first to save some code here, but that's just some nitpicking.

This revision is now accepted and ready to land.Jan 29 2020, 6:14 PM
gvgeo updated this revision to Diff 74688.Jan 30 2020, 1:16 PM

Removed duplicate code, and used count instead.

I was hopping for something better, this has couple more steps to complete.

gvgeo retitled this revision from [WIP]Enable adding place as first child to Enable adding place as first child.Jan 30 2020, 1:16 PM
gvgeo marked 3 inline comments as done.
gvgeo updated this revision to Diff 74689.Jan 30 2020, 1:18 PM

Revert style edit

gvgeo planned changes to this revision.Jan 30 2020, 1:32 PM
gvgeo updated this revision to Diff 74690.Jan 30 2020, 1:53 PM

count -1

This revision is now accepted and ready to land.Jan 30 2020, 1:53 PM
meven added a comment.Jan 30 2020, 2:42 PM
Before:
Adding a place as first child will place it at the end.

What is after ? How do I test this exactly ?

gvgeo planned changes to this revision.Jan 30 2020, 7:12 PM
gvgeo edited the test plan for this revision. (Show Details)Jan 31 2020, 6:45 AM
gvgeo edited the test plan for this revision. (Show Details)Jan 31 2020, 6:53 AM
gvgeo edited the test plan for this revision. (Show Details)
gvgeo updated this revision to Diff 74749.EditedJan 31 2020, 10:23 AM
gvgeo edited the test plan for this revision. (Show Details)

The last index is incorrect, and I can't find a way to get the proper one. Restoring first revision, which keeps the functionality as it was.

Also haven't find exactly, why last index doesn't work. I miss something important around this code, despite knowing that it works fine as it is.

This revision is now accepted and ready to land.Jan 31 2020, 10:23 AM
gvgeo added a comment.Feb 20 2020, 8:48 PM

After some digging, I found that KBookmarkGroup has more items that the KFilePlacesModel.
It is impossible to make KBookmark move at the end of a BookmarkGroup, if the item is not in the KFilePlacesModel.
I don't see a better solution than the current revision of the patch. (Assuming that my proposed change of using null as first item is acceptable.)

index: KBookmarkGroup -> KFilePlacesModel
0 :  "Home" -> "Home"
1 :  "Desktop" -> "Desktop"
2 :  "Network" -> "Trash"
3 :  "Trash" -> "Downloads"
4 :  "Recent Files" -> "kde"
5 :  "Recent Locations" -> "config"
6 :  "Modified Today" -> "Templates"
7 :  "Modified Yesterday" -> "Sync"
8 :  "Documents" -> "Pictures"
9 :  "Images" -> "Network"
10 :  "Audio" -> "Recent Files"
11 :  "Videos" -> "Recent Locations"
12 :  "--- separator ---" -> "--- separator ---"
13 :  "--- separator ---" -> "--- separator ---"
14 :  "--- separator ---" -> "--- separator ---"
15 :  "--- separator ---" -> "--- separator ---"
16 :  "--- separator ---" -> "--- separator ---"
17 :  "Downloads"
18 :  "kde"
19 :  "config"
20 :  "--- separator ---"
21 :  "--- separator ---"
22 :  "Templates"
23 :  "Sync"
24 :  "--- separator ---"
25 :  "Pictures"
26 :  "--- separator ---"
27 :  "--- separator ---"
28 :  ""
gvgeo edited the test plan for this revision. (Show Details)Mon, Mar 9, 8:36 AM
ngraham accepted this revision.Mon, Mar 9, 6:40 PM
ngraham added a subscriber: ngraham.
ngraham added inline comments.
src/filewidgets/kfileplacesmodel.cpp
1059

unrelated change

gvgeo added inline comments.Mon, Mar 9, 7:43 PM
src/filewidgets/kfileplacesmodel.cpp
1059

It is a style change, but it is hardly unrelated, I copy this line above. I need to mirror the style for ease of reading. And, I'm not going to copy a "bad"(in lack of a better word) style.

gvgeo edited the summary of this revision. (Show Details)Tue, Mar 10, 4:59 AM
gvgeo abandoned this revision.Wed, Mar 18, 8:16 AM

Thanks for your help @meven, but I 'll close this patch now.
It doesn't seem to be high in priorities.
And to be honest among other reasons, after I found how problematic the code is, I'm not so keen on building on top of it.

That's not how this works; reviewers being slow is not a good reason to abandon a patch. If you've lost interest in it, you can ask someone else can take it over, but just throwing away the work is pretty sad.

gvgeo added a comment.Wed, Mar 18, 3:26 PM

That's not how this works; reviewers being slow is not a good reason to abandon a patch. If you've lost interest in it, you can ask someone else can take it over, but just throwing away the work is pretty sad.

I'm pretty sure you misunderstood, I wouldn't abandon something because I lost interest. Reviewers being SO slow made me revisit this patch many times, each time I was seeing the same flaw in it. (Although the trigger to rethink it was different this time.)
If the code was good, it would be indeed sad. But bandaging the problems instead of fixing the core, is something that could create even more problems in the future.
The only thing I didn't like, was wasting meven's time who had taken time helping me.