Brings back the konqueror sidebar panel, with a few new features. Buttons for: home, root, fonts, bookmarks, places, settings, remote, and others.
Automatically detects and selects in the panel the current location of the active view (except for the places panel, for now).
Details
Details
- Reviewers
dfaure - Commits
- R226:64bfeab6e5bf: Fifth revision of the konq sidebar code.
Press F9 to bring up the panel. Try multiple combinations of:
having split window views
toggling the panel on and off
toggling the panel visibility on and off
clicking on items in the panel listings
navigating using the locationbar, history, or clicking inside the view itself
selecting different active view, including those with loaded urls that match and don't match the panel button
Diff Detail
Diff Detail
- Repository
- R226 Konqueror
- Branch
- konqsidebar_tree_module_PHAB (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 25271 Build 25289: arc lint + arc unit
| sidebar/tree_module/tree_module.h | ||
|---|---|---|
| 42 | This will of course be set to the appropriate value in the landed version. Whatever that value needs to be. | |
Comment Actions
Nice work!
| sidebar/bookmarks_module/bookmarks_module.cpp | ||
|---|---|---|
| 51 | unused, remove | |
| 77 | No neither can be null. | |
| 88 | missing space after , | |
| 105 | remove virtual when using override (repeats) | |
| sidebar/bookmarks_module/bookmarks_module.h | ||
| 27 | class QTreeView; would be enough | |
| 28 | class QStandardItemModel; | |
| 29 | move to .cpp file | |
| sidebar/places_module/places_module.cpp | ||
| 30 | deprecated, and unused | |
| 76 | style()->pixelMetric(QStyle::PM_SmallIconSize) | |
| sidebar/sidebar_part.h | ||
| 71 | ?? (the .cpp has the bigfoot address too, so either change both or none) | |
| sidebar/sidebar_widget.cpp | ||
| 186 | initialize in header, to make sure it's not used before it's initialized? | |
| sidebar/sidebar_widget.h | ||
| 75 | remove | |
| sidebar/tree_module/tree_module.cpp | ||
| 67 | use .isLocalFile() | |
| 78 | for loop? ;) | |
| 87 | rename slot to match signal name | |
| 97 | remove comment | |
| 106 | why not just do this on the calling side and remove this overload? It'll be clearer that there's QUrl parsing going on. | |
| 118 | put homePath() into a local variable to avoid calling it 3 times in the code? | |
| 143 | QUrl() | |
| 163 | yep better not do that, since it's async, the user can actually use the app meanwhile, better than a busy cursor | |
| 166 | KDirModel, there's no such thing as KDir Name slot something better? slotExpandRequested? slotDirectoryExpanded? slotDirectoryListed? | |
| 183 | It seems strange to me that changing selection opens URL. A shift+click would do strange things, for instance. This is usually done using the activated() signal instead. | |
| 213 | remove | |
| sidebar/tree_module/tree_module.h | ||
| 42 | git master can depend on KF 5.69, you can remove the version check and the define altogether. | |
| 55 | remove virtual | |
| 79 | move up with the other methods | |
| src/konqanimatedlogo.cpp | ||
| 41 ↗ | (On Diff #79177) | remove |
| sidebar/bookmarks_module/bookmarks_module.cpp | ||
|---|---|---|
| 105 | I assume I need to do this in tree_module.cpp as well (you didn't mention it below though), so I went ahead and did that. | |
| sidebar/bookmarks_module/bookmarks_module.h | ||
| 28 | Without it, I get a compile error: error: ‘QStandardItemModel’ does not name a type | |
| sidebar/tree_module/tree_module.cpp | ||
| 67 | I was actually wondering earlier, could this potentially work for non-local url's too? Since the tree module code can already display things like fonts: and remote:, and someone might theoretically come down the road one day and create a kio protocol implementation that might make good use of having the root node. That way I could just remove the check altogether. | |
| 78 | I did consider it, but thought it was ok like this, and kept things simpler. Should I use a loop instead? I figure if someone has some interesting implementation variation they wanted to try, where they show an extra column in the tree sidebar, they could just comment out or quickly edit the line to account for it. I don't mind either way. Let me know. | |
| 183 | If I remember correctly, this is to do with the history stack. I remember trying a few things a few months back, and this was the best approach I came up with. I just tried the shift+click and experienced no issues. Do you want me to change this here? If I need to take a different approach, it might take a while including testing and debugging. | |
| sidebar/tree_module/tree_module.h | ||
| 42 | Ok, I will do this in the next round, so I can update this current set of changes to the github version I have up, so people who can't yet upgrade their kf5 can still use this. | |
| sidebar/bookmarks_module/bookmarks_module.cpp | ||
|---|---|---|
| 105 | yes, when a review comment applies to multiple places in the code, I use "(repeats)", which means "look everywhere for other occurences" so I don't have to add 20 duplicated comments. | |
| sidebar/bookmarks_module/bookmarks_module.h | ||
| 28 | Well you need the include in the .cpp file, obviously. | |
| sidebar/tree_module/tree_module.cpp | ||
| 67 | Yes my code in KDirModel::openUrl should work for all protocols. You can remove the check. | |
| 78 | It's easy enough to call setColumnHidden(4, false) after the loop, to show only that column. | |
| 106 | comment not handled? | |
| 117 | prepend const | |
| 119 | homePath + '/' no need to modify homePath. | |
| 183 | If the tree can get focus, then also a simple Key_Up/Key_Down will trigger this and open urls unexpectedly. Oh well, we can move ahead with this, and revisit at the first bug report ;-) | |
| sidebar/bookmarks_module/bookmarks_module.h | ||
|---|---|---|
| 28 | Oh, heh, I definitely misunderstood the request. Done now. | |
| sidebar/tree_module/tree_module.cpp | ||
| 106 | Oops, I thought I had responded, not sure what happened. The reason is line 53, since we read directly from configGroup, and the .desktop file might have the "~" in the url. If I remember correctly, I had tried doing this on the plugin side and I couldn't get it to work, hence the note I left there for myself, since I remember you asking for this in a previous iteration (with a lot of changes in between, so I might have forgotten to mention it after). | |
| sidebar/bookmarks_module/bookmarks_module.h | ||
|---|---|---|
| 28 | I hope I did it right now. It's pretty late and things are getting blurry. | |
Comment Actions
@rrosch Please ensure you have a single commit on top of master then do arc diff HEAD~ again. There's nothing to review at the moment.
So close to the goal... :-)
Comment Actions
Let's land this. You can do the small fixes in a followup commit.
| sidebar/bookmarks_module/bookmarks_module.h | ||
|---|---|---|
| 27 | it's back? I thought you had a fwd-decl... | |
| sidebar/tree_module/tree_module.cpp | ||
| 74 | There's nothing called KDir. You mean KDirModel ;) | |
| 106 | I just mean to remove the QString overload and call cleanURL(QUrl(myQString)) where necessary. | |
| 226 | this method could be const | |
| src/konqanimatedlogo.cpp | ||
| 28 ↗ | (On Diff #80176) | unused, remove |
| sidebar/tree_module/tree_module.h | ||
|---|---|---|
| 64 | making a method const means const at the end. Returning a const object is useless and a bad idea. | |