D19926: Add Bookmark Handling

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> dolphinbookmarkhandler.cpp:39-41
> +        bookmarksFile = QStringLiteral("%1/dolphin").arg(QStandardPaths::writableLocation(
> +                                                             QStandardPaths::GenericDataLocation));
> +        QDir().mkpath(bookmarksFile);

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

> dolphinbookmarkhandler.cpp:42
> +        QDir().mkpath(bookmarksFile);
> +        bookmarksFile += QStringLiteral("/bookmarks.xml");
> +    }

Prefer `QLatin1String` for concatenations.

> dolphinbookmarkhandler.cpp:92-96
> +    switch (option) {
> +    case BookmarkOption::ShowAddBookmark: return true;
> +    case BookmarkOption::ShowEditBookmark: return true;
> +    }
> +    return false;

Why not just

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

> dolphinbookmarkhandler.cpp:99
> +
> +void DolphinBookmarkHandler::openBookmark(const KBookmark& bm, Qt::MouseButtons, Qt::KeyboardModifiers)
> +{

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

> dolphinbookmarkhandler.h:51-53
> +    QString getTitle(DolphinViewContainer* viewContainer) const;
> +    QUrl getUrl(DolphinViewContainer* viewContainer) const;
> +    QString getIcon(DolphinViewContainer* viewContainer) const;

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

> dolphinmainwindow.h:78
> +     */
> +    QList<DolphinViewContainer*> allViewContainers() const;
> +

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

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/20190420/3fad1a3e/attachment.htm>