Differential D19926 Diff 56004 src/dolphinbookmarkhandler.cpp

Changeset View

Standalone View

View Options

src/dolphinbookmarkhandler.cpp

  • This file was added.
1​/***************************************************************************
2​ * Copyright (C) 2019 by David Hallas <david@davidhallas.dk> *
3​ * *
4​ * This program is free software; you can redistribute it and/or modify *
5​ * it under the terms of the GNU General Public License as published by *
6​ * the Free Software Foundation; either version 2 of the License, or *
7​ * (at your option) any later version. *
8​ * *
9​ * This program is distributed in the hope that it will be useful, *
10​ * but WITHOUT ANY WARRANTY; without even the implied warranty of *
11​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the *
12​ * GNU General Public License for more details. *
13​ * *
14​ * You should have received a copy of the GNU General Public License *
15​ * along with this program; if not, write to the *
16​ * Free Software Foundation, Inc., *
17​ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA *
18​ ***************************************************************************/
19​
20​#include "dolphinbookmarkhandler.h"
21​#include "dolphinmainwindow.h"
22​#include "dolphinviewcontainer.h"
23​#include "global.h"
24​#include <KBookmarkMenu>
25​#include <KIO/Global>
26​#include <QDir>
27​#include <QStandardPaths>
28​
29​DolphinBookmarkHandler::DolphinBookmarkHandler(DolphinMainWindow *mainWindow,
30​ KActionCollection* collection,
31​ QMenu* menu,
32​ QObject* parent) :
33​ QObject(parent),
34​ m_mainWindow(mainWindow)
35​{
36​ QString bookmarksFile = QStandardPaths::locate(QStandardPaths::GenericDataLocation,
37​ QStringLiteral("dolphin/bookmarks.xml"));
38​ if (bookmarksFile.isEmpty()) {
elvisangelaccioUnsubmitted
Not Done

Prefer QLatin1String for concatenation.

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

elvisangelaccio: Prefer `QLatin1String` for concatenation. Or we could do `QStringliteral("%1/dolphin").arg…
39​ bookmarksFile = QStringLiteral("%1/dolphin").arg(QStandardPaths::writableLocation(
40​ QStandardPaths::GenericDataLocation));
41​ QDir().mkpath(bookmarksFile);
elvisangelaccioUnsubmitted
Not Done

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

elvisangelaccio: What if `QStandardPaths::writableLocation()` returns an empty string?
hallasAuthorUnsubmitted
Not Done

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

hallas: I am not quite sure what to do in this case. When I look in Konqueror or Okular, which has very…
elvisangelaccioUnsubmitted
Done

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

elvisangelaccio: We could at least print a qWarning() so that the users might have a chance to figure out why…
hallasAuthorUnsubmitted
Not Done

Good idea - I have added that in the latest patch.

hallas: Good idea - I have added that in the latest patch.
42​ bookmarksFile += QStringLiteral("/bookmarks.xml");
elvisangelaccioUnsubmitted
Done

Prefer QLatin1String for concatenations.

elvisangelaccio: Prefer `QLatin1String` for concatenations.
43​ }
elvisangelaccioUnsubmitted
Done

typo: "might not"

But actually I'd just say "will not".

elvisangelaccio: typo: "might not" But actually I'd just say "will not".
44​ m_bookmarkManager = KBookmarkManager::managerForFile(bookmarksFile, QStringLiteral("dolphin"));
45​ m_bookmarkManager->setUpdate(true);
46​ m_bookmarkMenu.reset(new KBookmarkMenu(m_bookmarkManager, this, menu, collection));
47​}
48​
49​DolphinBookmarkHandler::~DolphinBookmarkHandler()
50​{
51​}
52​
53​void DolphinBookmarkHandler::fillControlMenu(QMenu* menu, KActionCollection* collection)
54​{
55​ m_bookmarkControlMenu.reset(new KBookmarkMenu(m_bookmarkManager, this, menu, collection));
56​}
57​
58​QString DolphinBookmarkHandler::currentTitle() const
59​{
60​ return getTitle(m_mainWindow->activeViewContainer());
61​}
62​
63​QUrl DolphinBookmarkHandler::currentUrl() const
64​{
65​ return getUrl(m_mainWindow->activeViewContainer());
66​}
67​
68​QString DolphinBookmarkHandler::currentIcon() const
69​{
70​ return getIcon(m_mainWindow->activeViewContainer());
71​}
72​
73​bool DolphinBookmarkHandler::supportsTabs() const
74​{
75​ return true;
76​}
77​
78​QList<KBookmarkOwner::FutureBookmark> DolphinBookmarkHandler::currentBookmarkList() const
79​{
80​ QList<DolphinViewContainer*> allViewContainers = m_mainWindow->allViewContainers();
81​ QList<FutureBookmark> bookmarks;
82​ bookmarks.reserve(allViewContainers.size());
83​ for (int i = 0; i < allViewContainers.size(); ++i) {
84​ DolphinViewContainer* viewContainer = allViewContainers.at(i);
85​ bookmarks << FutureBookmark(getTitle(viewContainer), getUrl(viewContainer), getIcon(viewContainer));
elvisangelaccioUnsubmitted
Done

Missing const, which would also enable us to use the C++11 for loop below.

elvisangelaccio: Missing `const`, which would also enable us to use the C++11 for loop below.
86​ }
87​ return bookmarks;
88​}
89​
90​bool DolphinBookmarkHandler::enableOption(KBookmarkOwner::BookmarkOption option) const
91​{
92​ switch (option) {
93​ case BookmarkOption::ShowAddBookmark: return true;
94​ case BookmarkOption::ShowEditBookmark: return true;
95​ }
96​ return false;
elvisangelaccioUnsubmitted
Not Done

Why not just

return (option == BookmarkOption::ShowAddBookmark || option == BookmarkOption::ShowEditBookmark)
elvisangelaccio: Why not just return (option == BookmarkOption::ShowAddBookmark || option == BookmarkOption…
hallasAuthorUnsubmitted
Not Done

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.

hallas: The reason would be, that if a new enum entry is added to `KBookmarkOwner::BookmarkOption` then…
97​}
98​
99​void DolphinBookmarkHandler::openBookmark(const KBookmark& bm, Qt::MouseButtons, Qt::KeyboardModifiers)
elvisangelaccioUnsubmitted
Done

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

elvisangelaccio: Please use a descriptive variable name (e.g. `bookmark`). Same for the functions below.
hallasAuthorUnsubmitted
Not Done

Good point!

hallas: Good point!
100​{
101​ m_mainWindow->changeUrl(bm.url());
102​}
103​
104​void DolphinBookmarkHandler::openFolderinTabs(const KBookmarkGroup& bm)
105​{
106​ m_mainWindow->openDirectories(bm.groupUrlList(), false);
107​}
108​
109​void DolphinBookmarkHandler::openInNewTab(const KBookmark& bm)
110​{
111​ m_mainWindow->openNewTabAfterCurrentTab(bm.url());
112​}
113​
114​void DolphinBookmarkHandler::openInNewWindow(const KBookmark& bm)
115​{
116​ Dolphin::openNewWindow({bm.url()}, m_mainWindow);
117​}
118​
119​QString DolphinBookmarkHandler::getTitle(DolphinViewContainer* viewContainer) const
120​{
121​ return viewContainer->caption();
122​}
123​
124​QUrl DolphinBookmarkHandler::getUrl(DolphinViewContainer* viewContainer) const
125​{
126​ return viewContainer->url();
127​}
128​
129​QString DolphinBookmarkHandler::getIcon(DolphinViewContainer* viewContainer) const
130​{
131​ return KIO::iconNameForUrl(viewContainer->url());
132​}