Add Bookmark Handling
ClosedPublic

Authored by hallas on Mar 20 2019, 7:36 PM.

Details

Summary

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.

Test Plan

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 10741
Build 10759: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
src/dolphinbookmarkhandler.cpp
38

Prefer QLatin1String for concatenation.

Or we could do QStringliteral("%1/dolphin").arg(QStandardPaths::writableLocation(QStandardPaths::GenericDataLocation))

src/dolphinbookmarkhandler.h
54โ€“55

Why the trailing underscore?

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.

hallas updated this revision to Diff 54510.Mar 21 2019, 7:50 PM

Fixed minor review comments.
Changes menu entry to be a top-level entry instead.

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.

hallas marked 2 inline comments as done.Mar 23 2019, 12:26 PM
hallas added inline comments.
src/dolphinbookmarkhandler.h
54โ€“55

Removed - a habit from another project ;)

hallas updated this revision to Diff 54603.Mar 23 2019, 12:26 PM
hallas marked an inline comment as done.

Implemented review comments

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.

Hmm, when I apply this patch I don't actually see the new menu item.

Hmm, when I apply this patch I don't actually see the new menu item.

Make sure to delete ~/.local/share/kxmlgui5/dolphin/dolphinui.rc first.

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.

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

hallas updated this revision to Diff 54917.Mar 27 2019, 5:42 AM

Add the Bookmarks menu to the Control menu

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.

hallas added a comment.Apr 2 2019, 6:50 PM

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

hein added a subscriber: hein.EditedApr 2 2019, 6:54 PM

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).

hein added a comment.Apr 2 2019, 7:21 PM

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. :)

hallas added a comment.Apr 3 2019, 4:02 AM

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

hein added a comment.Apr 3 2019, 8:23 AM
This comment was removed by hein.
hein added a comment.EditedApr 3 2019, 8:26 AM

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.

hallas added a comment.Apr 3 2019, 5:25 PM
In D19926#442734, @hein wrote:

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?

hein added a comment.Apr 3 2019, 5:32 PM

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? :)

cfeck added a subscriber: cfeck.Apr 3 2019, 8:10 PM

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.

That makes a lot of sense @elvisangelaccio.

loh.tar added a subscriber: loh.tar.Apr 7 2019, 6:38 PM

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"

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.

I'm okay with that.

hallas updated this revision to Diff 56004.Apr 11 2019, 5:45 PM

Moved the Bookmarks menu to the Go menu

ngraham accepted this revision.Apr 11 2019, 5:50 PM

Nice work. Let's wait for @elvisangelaccio's final review now!

This revision is now accepted and ready to land.Apr 11 2019, 5:50 PM
elvisangelaccio requested changes to this revision.Apr 20 2019, 2:54 PM
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
995โ€“998

Please move this under the Go menu too.

This revision now requires changes to proceed.Apr 20 2019, 2:54 PM
src/dolphinbookmarkhandler.cpp
39โ€“41

What if QStandardPaths::writableLocation() returns an empty string?

42

Prefer QLatin1String for concatenations.

92โ€“96

Why not just

return (option == BookmarkOption::ShowAddBookmark || option == BookmarkOption::ShowEditBookmark)
99

Please use a descriptive variable name (e.g. bookmark). Same for the functions below.

src/dolphinbookmarkhandler.h
51โ€“53

Coding style: don't use getFoo() for getters, but just foo()

src/dolphinmainwindow.h
78

The "all" prefix is kinda redundant, viewContainers() would be clear enough.

hallas updated this revision to Diff 56740.Apr 22 2019, 12:54 PM
hallas marked 5 inline comments as done.

Review comments

hallas added inline comments.Apr 22 2019, 12:54 PM
src/dolphinbookmarkhandler.cpp
39โ€“41

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 :/

92โ€“96

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.

99

Good point!

src/dolphinbookmarkhandler.cpp
39โ€“41

We could at least print a qWarning() so that the users might have a chance to figure out why their bookmarks are not saved.

hallas updated this revision to Diff 56757.Apr 22 2019, 5:15 PM

Print qWarning if GenericDataLocation is empty

hallas marked an inline comment as done.Apr 22 2019, 5:15 PM
hallas added inline comments.
src/dolphinbookmarkhandler.cpp
39โ€“41

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
78

We could use QVector here, which should be preferred to QList for new code.

hallas updated this revision to Diff 57102.Apr 27 2019, 1:54 PM
hallas marked 5 inline comments as done.

Review comments

elvisangelaccio accepted this revision.Apr 28 2019, 11:06 AM

Please wait a week or so before pushing, in case @hein @cfeck or someone else wants to chime in.

This revision is now accepted and ready to land.Apr 28 2019, 11:06 AM
meven added a subscriber: meven.Apr 29 2019, 8:00 AM

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.

meven added a comment.Apr 29 2019, 8:07 AM

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 ?

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?

ngraham added a comment.EditedApr 29 2019, 3:03 PM

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 :)

hallas updated this revision to Diff 57193.Apr 29 2019, 3:06 PM

Share bookmarks.xml with kfile

hallas added a comment.May 6 2019, 5:28 PM

@elvisangelaccio - is it ok to merge this change now or do we need to wait for anything else?

elvisangelaccio accepted this revision.May 11 2019, 9:42 AM

Go for it ;)

ognarb added a subscriber: ognarb.May 11 2019, 11:08 AM

There is a big tutorial about bookmark handling Dolphin in userbase, that do I need to update?

There is a big tutorial about bookmark handling Dolphin in userbase, that do I need to update?

Yes, that tutorial will need to be updated.

This revision was automatically updated to reflect the committed changes.