Added the option to limit the displayed folders in the folder panel (F7) to the tree below the user's home directory if the current URL is inside the home directory.
This can be configured in the preferences General/Behaviour tab by checking the corresponding check box.
Details
- Reviewers
elvisangelaccio emmanuelp - Group Reviewers
Dolphin - Commits
- R318:94fab8c80ed6: Limit folder panel to home directory if inside home
Diff Detail
- Repository
- R318 Dolphin
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Thanks for the contribution.
There is a small regression: if I enable the new option, the folder panel no longer follows the URL of the main dolphin view. Is that intended?
src/panels/folders/folderspanel.cpp | ||
---|---|---|
195 | Coding style: missing braces | |
src/settings/dolphin_generalsettings.kcfg | ||
65–68 ↗ | (On Diff #18582) | Use dolphin_folderspanelsettings.kcfg instead. Also, the default should probably be true (see https://bugs.kde.org/show_bug.cgi?id=191917). |
src/settings/general/behaviorsettingspage.cpp | ||
87 ↗ | (On Diff #18582) | I don't think we should add a checkbox in the main settings dialog, we could just add it in the context menu of the panel (treeviewcontextmenu.cpp). |
- Implemented changes requested by Elvis Angelaccio:
- moved checkbox from main settings dialog to context menu of folders panel
- added missing braces (coding style)
- changed default to enabled
- fixed regression: folders not expanding, caused by wrong directory string splitting
src/panels/folders/folderspanel.cpp | ||
---|---|---|
326 | Use Dolphin::homeUrl() from global.h | |
355 | The condition here is different from (homeUrl.isParentOf(url) || homeUrl == url) used above. Is this intended? Why? | |
365 | Why this change? | |
src/panels/folders/treeviewcontextmenu.cpp | ||
129 | "folder panel" is kinda redundant/implicit now, maybe just use ""Limit to home directory if inside home" |
Implemented changes reqeuested by Elvis Angelaccio: - using Dolphin::homeUrl() - added comment - changed menu item text - removed unnecessary change
src/panels/folders/folderspanel.cpp | ||
---|---|---|
355 | At this point, a previous selection needs to be cleared, only if homeUrl == url. The previous selection won't correspond to the currently active home directory. If homeUrl is a parent of url and not equal, the selection is correctly changed. | |
365 | I changed this, because the selection wasn't updated correctly. But if I change it back, I cannot reproduce this behaviour now. The change in kfileitemmodel.cpp has fixed this, too, so I removed the change here. |
Nice work! :)
I have added some suggestions and questions.
Please make use of QString::SkipEmptyParts, everything else looks really good!
src/kitemviews/kfileitemmodel.cpp | ||
---|---|---|
647 | Better use the QString::SkipEmptyParts split behavior provided by QString::split [1]. | |
src/panels/folders/folderspanel.cpp | ||
195 | Why is this change required? | |
327–333 | Personal taste: To make the condition more readable I would split it up like this: const bool isInHomeFolder = Dolphin::homeUrl().isParentOf(url) || (Dolphin::homeUrl() == url); if (FoldersPanelSettings::limitFoldersPanelToHome() && isInHomeFolder) { .... } else { .... } | |
src/panels/folders/folderspanel.h | ||
87 | Personal taste: I would just name it refresh because the FoldersPanel suffix is somehow redundant. As far as I can see, this slot can be converted into a private method? Maybe name it reloadTree because this is what it actually does. |
src/panels/folders/folderspanel.cpp | ||
---|---|---|
359 | This shouldn't be required, see FoldersPanel::slotLoadingCompleted. |
Implemented changes requested by Emmanuel Pescosta:
- using QString::SkipEmptyParts
- removed unnecessary change
- rephrased condition for readability
- renamed refreshFoldersPanel(), converted to private method reloadTree()
src/panels/folders/folderspanel.cpp | ||
---|---|---|
195 | This change is from our original patch against KDE4 I ported. Neither do I know why this was changed, nor does it seem to have any negative effects to leave it out. So I removed the change. | |
359 | Here, it is required: if a subdirectory of home has been selected previously before selecting home itself, the previous selection is not cleared. There is nothing to be newly selected in the folders panel in this case. |
src/panels/folders/folderspanel.cpp | ||
---|---|---|
359 | FoldersPanel::slotLoadingCompleted already clears the selection and thus this special case shouldn't be required. Your patch, without this changes, works fine locally (e.g. when going from / to ~). | |
src/panels/folders/treeviewcontextmenu.cpp | ||
129 | More consistent would be something like "Limit to Home Directory if Inside Home", because we use capital letters for action labels. But I would prefer a shorter action label like "Limit to Home Directory" but with an additional tool tip to describe the exact behavior. @elvisangelaccio, @hoffmannrobert: What do you think? |
src/panels/folders/folderspanel.cpp | ||
---|---|---|
359 |
|
src/panels/folders/treeviewcontextmenu.cpp | ||
---|---|---|
129 | Tooltips are disabled by default, so if somebody looks at Trash or Network, for example, and opens the folders panel context menu, the 'Limit to Home Directory' without further explanation would not make sense. In these cases, the option should be invisible or deactivated, I suggest. |
Last round
src/panels/folders/treeviewcontextmenu.cpp | ||
---|---|---|
129 |
+1 Can you please implement it? Thanks! |
Implemented changes requested by Emmanuel Pescosta:
- renamed option to 'Limit to Home Directory'
- show option only if inside home
src/panels/folders/treeviewcontextmenu.cpp | ||
---|---|---|
129 | Now, the option is only shown if the selected url is inside home. I think, this is expected behaviour in a context menu. |
src/panels/folders/folderspanel.cpp | ||
---|---|---|
359 | Thanks for the steps! I can now reproduce the problem. Please note that the same behavior occurs when e.g. going back from /home to /. The reason is that the index is -1 (preventing the immediate updateCurrentItem in line 347) but the url is already loaded and so slotLoadingCompleted won't be called. I would suggest something like this to fix the problem: if (index >= 0) { updateCurrentItem(index); } else if (url == baseUrl) { // clear the selection when visiting the base url updateCurrentItem(-1); } else { ... } |
Implemented changes requested by Emmanuel Pescosta:
- clear selection when visiting base url
Thanks, too. This is my first patch to a KDE project, so I don't have a developer account. Could you please land it for me? Thanks.