Add Bookmark Handling. Adds complete bookmark support as provided by
other KDE applications like Konsole and Konqueror. This allows you to
bookmark individual folders, create bookmark folders and open them.
Details
- Reviewers
elvisangelaccio ngraham - Group Reviewers
Dolphin - Maniphest Tasks
- T5408: Integrate KBookmarks (incl. bookmarks editor) into Dolphin
- Commits
- R318:2fac50f5f59b: Add Bookmark Handling
Go -> Bookmark -> Add Bookmark
Go -> Bookmark -> [Open the bookmark you selected]
FEATURE: 171366
Diff Detail
- Repository
- R318 Dolphin
- Branch
- bookmarks (branched from master)
- Lint
No Linters Available - Unit
No Unit Test Coverage - Build Status
Buildable 11347 Build 11365: arc lint + arc unit
My sense is that this should probably be a top-level menu item, especially since currently the menu is actually not shown by default and can therefore be considered a "expert feature"--same as this bookmarks feature.
In the default "Control" menu, it should probably be a top-level item too. It might also be nice if you could add an optional-and-off-by-default toolbar button that exposes this, too.
I have changed the menu entry to be a top-level Bookmarks entry, just like Konqueror and Konsole. I have also implemented the various suggestions.
src/dolphinbookmarkhandler.h | ||
---|---|---|
54โ55 | Removed - a habit from another project ;) |
What about the Control menu? Remember that the menubar is hidden by default in dolphin, so the average user probably won't be able to discover this feature.
Ah, that was the secret sauce, thanks!
This generally works quite well and is a very nice feature. One request: can you disable the Bookmark Tabs As Folder... menu item when there's only one tab open? Also yeah, we should probably consider putting this somewhere in the Control menu too.
I haven't found a way where I can conditionally remove the Bookmark Tabs As Folder... :( I would need to change the API of KBookmarks to do this. Any ideas for this are more than welcome :D
Ehh, this is probably fine for now then. :) Though it would be nice to look into that.
I ended up looking at it anyway ;) I have pushed this D20209 which extends KBookmarks with support for communicating if you actually have tabs open, and if not the Bookmark Tabs As Folder... entry is not added
Hang on, isn't this kind of redundant with Places? At the very least they overlap a lot. Is this feature individually strong enough to warrant that overlap?
Let's keep in mind that Dolphin came about to reboot Konqueror with more focus.
That was my first thought too, but then I tried it out and read through the linked bug report and changed my mind. The Places panel is really for frequent and immediate access whereas bookmarks are more for long-term organization. And you can have folders and hierarchies in the bookmarks menu, which the Places panel doesn't support.
I could see this being very useful for my mother if she used Dolphin. her macOS Finder's Places panel equivalent has like 50 entries in it--enough that it becomes almost uselessly slow to actually use. If she used Dolphin with this feature, she could keep the Places panel clean and put all of those entries in the bookmarks menu, and actually organize them properly in a hierarchy (which I know she would do).
The Places panel is really for frequent and immediate access whereas bookmarks are more for long-term organization.
This is a good argument; it's how I explain to people why we have both panel launchers and menu favorites when they tell me one should be removed.
However, this implies that the feature not be prominently exposed, because the idea of compartmentalizing lesser-used things away is to move them out of direct line of sight.
I could see this being very useful for my mother if she used Dolphin. her macOS Finder's Places panel equivalent has like 50 entries in it--enough that it becomes almost uselessly slow to actually use. If she used Dolphin with this feature, she could keep the Places panel clean and put all of those entries in the bookmarks menu, and actually organize them properly in a hierarchy (which I know she would do).
Would she find the feature if it's hidden away? If was prominently placed, I think it'd defeat the first argument, and also be a too costly tax on the users who don't need it.
Overall I'm still not super-sold on it, BTW.
If it were hidden away, she wouldn't find it, but once I showed it to her, she'd use it. Not sure what that amounts to though. :)
But I still think it is a pretty strong argument that it is one of the most voted for features for Dolphin, and it is something that was supported previously (because people used konqueror for file management). Also, the feature is _ot very intrusive. You get a new menu entry and a keyboard shortcut, that's it :)
So, I still think that if it helps a lot of people to use the application more effectively without cluttering the UI/menus and not adding a ton of code to maintain, then I am all for :D
I'm not sure the "Konqueror had it and people are asking
for it back" argument is strong. Because Konqueror was also unpopular enough to get replaced by Dolphin and to end up largely unmaintained (although it is still released and available). To me that indicates that becoming more like Konqueror may actually be the wrong direction.
The "does adding this clutter the app" thing is also sometimes evaluated wrong IMHO. It's usually looked at in this way: Is the UI now worse than before. But the cost to adding things is also paid forward! Next time someone wants to add a feature to the UI, it has to slot into a UI that already has the last thing that got added. Which is then already fuller, more cluttered situation to start with. In other words, adding stuff is more like a budgetary concern - "should I spend this complexity? I might need it later for something stronger". Because removing things later is almost impossible without a new app. Which is what happened last time.
That said, I don't know the user audience for Dolphin as well as the people working on Dolphin more regularly. If you all think this feature is strong or non-niche enough to be in there despite the above concerns, cool.
I think you have a valid point, and I think it is good that you question if features should be added :D I probably don't know the Dolphin user base better then you, I based the decision to add it solely on the comments in Bugzilla. How do we (as a community) decide on these things? Do we perform user polls? I usually put @ngraham on as reviewer as he seems to be very good at taking the average user perspective, but I don't know if that is enough?
Suuper good question that I also don't have a really good answer to :( But it's always super good when a project figures out how to answer it (e.g. Krita took off after it made a call to be a painting app).
I'm not infallible either, and do have a tendency to say "yes" a lot. Eike's perspective is quite welcome, and I won't pretend to have the answers myself.
Since Dolphin has a formal maintainer, maybe we should punt the hard decision to @elvisangelaccio? :)
Do other file managers offer bookmarks besides those that are always visible in the sidebar?
Unrelated to the first question, but can the user add the bookmarks action to the toolbar in case he doesn't use the menu bar, but only the control button? I remember there were problems when the action was to be used both by the menu as well as the toolbar.
Let's see what the dolphin philosophy says: https://github.com/KDE/dolphin/blob/master/HACKING.md
Relevant part:
Before a feature is added in Dolphin, it is checked whether the feature is mandatory for the target user group. If this is not the case, then this does not mean that the feature cannot be added; first it must be clarified whether the feature might be non-intrusive, so that it adds value for users outside the primary target user group of Dolphin. Non-intrusive is mainly related to the user interface. A feature that adds a lot of clutter to the main menu, context menus or toolbar might harm the target user group. In this case the feature will not be added.
This is a perfect example of feature that adds value for 'Jeff', but not for the target user group.
So, it this feature non-intrusive? Currently I think the answer is no: even though the menubar is hidden by default, the space on it is very limited.
We should probably reserve the space on the menubar for new features that will be actually aimed at the primary target group.
That said, I still think that a good compromise could be to put the "Bookmark" menu under the "Tools" menu.
a good compromise could be to put the "Bookmark" menu under the "Tools" menu.
Currently I placed the 'Bookmarks' menu under the 'Go' menu, is that a good idea?
Hm, looks to me more fitting than "Tools"
So, should we put this under "Tools" or under "Go"?
I think it is most logical to put it in the "Go" menu.
src/dolphinmainwindow.cpp | ||
---|---|---|
1002โ1005 | Please move this under the Go menu too. |
src/dolphinbookmarkhandler.cpp | ||
---|---|---|
40โ42 | What if QStandardPaths::writableLocation() returns an empty string? | |
43 | Prefer QLatin1String for concatenations. | |
93โ97 | Why not just return (option == BookmarkOption::ShowAddBookmark || option == BookmarkOption::ShowEditBookmark) | |
100 | Please use a descriptive variable name (e.g. bookmark). Same for the functions below. | |
src/dolphinbookmarkhandler.h | ||
52โ54 | Coding style: don't use getFoo() for getters, but just foo() | |
src/dolphinmainwindow.h | ||
79 | The "all" prefix is kinda redundant, viewContainers() would be clear enough. |
src/dolphinbookmarkhandler.cpp | ||
---|---|---|
40โ42 | I am not quite sure what to do in this case. When I look in Konqueror or Okular, which has very similar bookmark handling, then this case is not handled at all. In the case here, we would end up trying to create /dolphin (which would probably fail) and then append /bookmarks.xml and use that. Then KBookmarkManager would end up trying to read/write that file, which would also fail, so you wouldn't get your bookmarks persisted. I don't know if that is all bad :/ | |
93โ97 | The reason would be, that if a new enum entry is added to KBookmarkOwner::BookmarkOption then I would get a compiler warning hinting that I should probably handle that case here as well. | |
100 | Good point! |
src/dolphinbookmarkhandler.cpp | ||
---|---|---|
40โ42 | We could at least print a qWarning() so that the users might have a chance to figure out why their bookmarks are not saved. |
src/dolphinbookmarkhandler.cpp | ||
---|---|---|
40โ42 | Good idea - I have added that in the latest patch. |
src/dolphinbookmarkhandler.cpp | ||
---|---|---|
43 | typo: "might not" But actually I'd just say "will not". | |
85 | Missing const, which would also enable us to use the C++11 for loop below. | |
src/dolphinbookmarkhandler.h | ||
52โ54 | These 3 helper functions could all be static. | |
src/dolphinmainwindow.h | ||
79 | We could use QVector here, which should be preferred to QList for new code. |
We would probably like to allow the bookmarks to be accessible from KIOFileWidgets open/save dialog, somehow.
There is already a bookmark feature there, it would be great if the two matches.
KFileBookmarkHandler loads from "kfile/bookmarks.xml" the bookmarks.
Could this xml file be shared with dolphin ?
Good idea! I didn't know that the open/save dialogs had this feature :D
Basically changing the path in the dolphinbookmarkhandler.cpp is all it takes (I just tried it out locally) - so should we do that? @ngraham - what do you think?
Yep, makes sense to me to sync them up. You probably didn't know the open/save dialogs had this feature because it's off by default and the setting to turn it on is super hidden behind the "two sliders" button in the top-right corner that nobody ever clicks on :)
@elvisangelaccio - is it ok to merge this change now or do we need to wait for anything else?
There is a big tutorial about bookmark handling Dolphin in userbase, that do I need to update?