D19926: Add Bookmark Handling

[David Hallas]

hallas added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in dolphinbookmarkhandler.cpp:39-41
> What if `QStandardPaths::writableLocation()` returns an empty string?

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

> elvisangelaccio wrote in dolphinbookmarkhandler.cpp:92-96
> Why not just
> 
>   return (option == BookmarkOption::ShowAddBookmark || option == BookmarkOption::ShowEditBookmark)

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.

> elvisangelaccio wrote in dolphinbookmarkhandler.cpp:99
> Please use a descriptive variable name (e.g. `bookmark`). Same for the functions below.

Good point!

REPOSITORY
  R318 Dolphin

REVISION DETAIL
  https://phabricator.kde.org/D19926

To: hallas, #dolphin, elvisangelaccio, ngraham
Cc: loh.tar, cfeck, hein, kfm-devel, alexde, feverfew, meven, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190422/50488aa4/attachment.htm>