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.
meven | |
broulik | |
dfaure | |
ngraham |
Frameworks |
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.
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.
No Linters Available |
No Unit Test Coverage |
Buildable 21852 | |
Build 21870: arc lint + arc unit |
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.
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
1046 | It seems to me you don't need to edit this function. |
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 | As it is, with a null QModelIndex, the function below will move the new item to the top. |
I am not a Frameworks reviewer
Please give some time to others to have a look as well
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
1046 | I missed this, you could pass the current modelIndex or the first to save some code here, but that's just some nitpicking. |
Removed duplicate code, and used count instead.
I was hopping for something better, this has couple more steps to complete.
Before: Adding a place as first child will place it at the end.
What is after ? How do I test this exactly ?
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.
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 : ""
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
1054 | unrelated change |
src/filewidgets/kfileplacesmodel.cpp | ||
---|---|---|
1054 | 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. |
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.
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.