Differential D16830 Diff 46694 kate/katefileactions.cpp

Changeset View

Standalone View

View Options

kate/katefileactions.cpp

  • This file was added.
1​/* This file is part of the KDE project
2​ *
3​ * Copyright (C) 2018 Gregor Mi <codestruct@posteo.org>
4​ *
5​ * This library is free software; you can redistribute it and/or
6​ * modify it under the terms of the GNU Lesser General Public
7​ * License as published by the Free Software Foundation; either
8​ * version 2.1 of the License, or (at your option) version 3, or any
9​ * later version accepted by the membership of KDE e.V. (or its
10​ * successor approved by the membership of KDE e.V.), which shall
11​ * act as a proxy defined in Section 6 of version 3 of the license.
12​ *
13​ * This library is distributed in the hope that it will be useful,
14​ * but WITHOUT ANY WARRANTY; without even the implied warranty of
15​ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
16​ * Lesser General Public License for more details.
17​ *
18​ * You should have received a copy of the GNU Lesser General Public
19​ * License along with this library. If not, see <http://www.gnu.org/licenses/>.
20​**/
21​
22​#include "katefileactions.h"
23​
24​#include <ktexteditor/application.h>
25​#include <ktexteditor/document.h>
26​#include <ktexteditor/editor.h>
27​
28​#include <KIO/CopyJob>
29​#include <KIO/DeleteJob>
30​#include <KIO/OpenFileManagerWindowJob>
31​#include <KLocalizedString>
32​#include <KMessageBox>
33​#include <KPropertiesDialog>
34​
35​#include <QApplication>
36​#include <QClipboard>
37​#include <QDebug>
38​#include <QInputDialog>
39​#include <QProcess>
40​#include <QUrl>
41​
42​void KateFileActions::copyFilePathToClipboard(KTextEditor::Document* doc)
43​{
44​ QApplication::clipboard()->setText(doc->url().toDisplayString(QUrl::PreferLocalFile));
45​}
46​
47​void KateFileActions::openContainingFolder(KTextEditor::Document* doc)
48​{
49​ KIO::highlightInFileManager({doc->url()});
50​}
gregormiAuthorUnsubmitted
Not Done

Hello @dhaumann @cullmann,
I wonder if it would be better for the functions in this file to use a Document reference instead of a pointer. Currently, these functions will crash on a nullptr. In a function farther down below I used assertions which could be removed if I use references instead of pointers. This makes the code more readable and safe (and seems the recommended way to pass parameters: http://www.modernescpp.com/index.php/c-core-guidelines-how-to-pass-function-parameters).
What do you think?

gregormi: Hello @dhaumann @cullmann, I wonder if it would be better for the functions in this file to use…
51​
52​void KateFileActions::openFilePropertiesDialog(KTextEditor::Document* doc)
dhaumannUnsubmitted
Done

RAII: here we request a resource without initialization. Correct is:

bool ok = false;

Even if the bool is set in the next line via out parameter. Over time the code may change and then we have an uninitialized variable. :)

dhaumann: RAII: here we request a resource without initialization. Correct is: bool ok = false…
gregormiAuthorUnsubmitted
Not Done

Thanks for the review! I didn't look at the copied code very closely so far. My bad.

Though "bool ok;" should be corrected, I woudn't call it RAII: https://en.cppreference.com/w/cpp/language/raii says RAII is about acquiring and releasing resources which exist in limited supply like heap memory, file handles etc. Creating a mere stack variable does not fit this description.

gregormi: Thanks for the review! I didn't look at the copied code very closely so far. My bad. Though…
53​{
54​ KFileItem fileItem(doc->url());
55​ QDialog* dlg = new KPropertiesDialog(fileItem);
56​ dlg->setAttribute(Qt::WA_DeleteOnClose);
57​ dlg->show();
58​}
59​
60​void KateFileActions::renameDocumentFile(QWidget* parent, KTextEditor::Document* doc)
61​{
62​ // TODO: code was copied and adapted from ../addons/filetree/katefiletree.cpp
63​ // (-> DUPLICATE CODE, the new code here should be also used there!)
64​
65​ if (!doc) {
66​ return;
67​ }
68​
69​ const QUrl oldFileUrl = doc->url();
70​
71​ if (oldFileUrl.isEmpty()) { // NEW
72​ return;
73​ }
74​
75​ const QString oldFileName = doc->url().fileName();
76​ bool ok = false;
77​ QString newFileName = QInputDialog::getText(parent, // ADAPTED
78​ i18n("Rename file"), i18n("New file name"), QLineEdit::Normal, oldFileName, &ok);
79​ if (!ok) {
80​ return;
81​ }
82​
83​ QUrl newFileUrl = oldFileUrl.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash);
84​ newFileUrl.setPath(newFileUrl.path() + QLatin1Char('/') + newFileName);
85​
86​ if (!newFileUrl.isValid()) {
87​ return;
88​ }
89​
90​ if (!doc->closeUrl()) {
91​ return;
92​ }
93​
94​ doc->waitSaveComplete();
dhaumannUnsubmitted
Done

Here, I would prefer

const auto && url = ...

Reasoning: && uses a reference if the returned value is a reference, otherwise a copy. In this case, it's more what we want in long term (aka it's always correct, even if the API changes).

dhaumann: Here, I would prefer const auto && url = ... Reasoning: && uses a reference if the…
95​
96​ KIO::CopyJob* job = KIO::move(oldFileUrl, newFileUrl);
97​ QSharedPointer<QMetaObject::Connection> sc(new QMetaObject::Connection());
98​ auto success = [doc, sc] (KIO::Job*, const QUrl&, const QUrl &realNewFileUrl, const QDateTime&, bool, bool) {
99​ doc->openUrl(realNewFileUrl);
100​ doc->documentSavedOrUploaded(doc, true);
101​ QObject::disconnect(*sc);
102​ };
103​ *sc = parent->connect(job, &KIO::CopyJob::copyingDone, doc, success);
104​
105​ if (!job->exec()) {
106​ KMessageBox::sorry(parent, i18n("File \"%1\" could not be moved to \"%2\"", oldFileUrl.toDisplayString(), newFileUrl.toDisplayString()));
107​ doc->openUrl(oldFileUrl);
108​ }
109​}
110​
111​void KateFileActions::deleteDocumentFile(QWidget* parent, KTextEditor::Document* doc)
112​{
113​ // TODO: code was copied and adapted from ../addons/filetree/katefiletree.cpp
114​ // (-> DUPLICATE CODE, the new code here should be also used there!)
115​
116​ if (!doc) {
117​ return;
118​ }
119​
120​ const auto&& url = doc->url();
121​
122​ if (url.isEmpty()) { // NEW
123​ return;
124​ }
125​
126​ bool go = (KMessageBox::warningContinueCancel(parent,
127​ i18n("Do you really want to delete file \"%1\"?", url.toDisplayString()),
128​ i18n("Delete file"),
129​ KStandardGuiItem::yes(), KStandardGuiItem::no(), QLatin1String("filetreedeletefile")
130​ ) == KMessageBox::Continue);
131​
132​ if (!go) {
133​ return;
134​ }
135​
136​ if (!KTextEditor::Editor::instance()->application()->closeDocument(doc)) {
137​ return; // no extra message, the internals of ktexteditor should take care of that.
138​ }
139​
dhaumannUnsubmitted
Done

Can't you use QProcess? :) Wasn't KProcess close to deprecated? I think KProcess is only required in rare corner cases nowadays.

dhaumann: Can't you use QProcess? :) Wasn't KProcess close to deprecated? I think KProcess is only…
140​ if (url.isValid()) {
141​ KIO::DeleteJob *job = KIO::del(url);
142​ if (!job->exec()) {
143​ KMessageBox::sorry(parent, i18n("File \"%1\" could not be deleted.", url.toDisplayString()));
144​ }
145​ }
146​}
147​
148​QStringList KateFileActions::supportedDiffTools()
149​{
150​ // LATER: check for program existence and set some boolean value accordingly
151​ // Can this be even done in an easy way when we don't use the absolute path to the executable?
152​ // See https://stackoverflow.com/questions/42444055/how-to-check-if-a-program-exists-in-path-using-qt
153​
154​ QStringList resultList;
155​ resultList.push_back(QStringLiteral("kdiff3"));
156​ resultList.push_back(QStringLiteral("kompare"));
157​ resultList.push_back(QStringLiteral("meld"));
158​
159​ return resultList;
160​}
161​
162​bool KateFileActions::compareWithExternalProgram(KTextEditor::Document* documentA, KTextEditor::Document* documentB, const QString& diffExecutable)
163​{
164​ Q_ASSERT(documentA);
165​ Q_ASSERT(documentB);
166​
167​ QProcess process;
168​ QStringList arguments;
169​ arguments << documentA->url().toLocalFile() << documentB->url().toLocalFile();
170​ return process.startDetached(diffExecutable, arguments);
171​}