Differential D16830 Diff 45748 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​ Copyright (C) 2018 GregorMi <codestruct@posteo.org>
3​
4​ This library is free software; you can redistribute it and/or
5​ modify it under the terms of the GNU Library General Public
6​ License as published by the Free Software Foundation; either
7​ version 2 of the License, or (at your option) any later version.
8​
9​ This library 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 GNU
12​ Library General Public License for more details.
13​
14​ You should have received a copy of the GNU Library General Public License
15​ along with this library; see the file COPYING.LIB. If not, write to
16​ the Free Software Foundation, Inc., 51 Franklin Street, Fifth Floor,
17​ Boston, MA 02110-1301, USA.
18​*/
19​#include "katefileactions.h"
20​
21​#include <ktexteditor/application.h>
22​#include <ktexteditor/document.h>
23​#include <ktexteditor/editor.h>
24​
25​#include <KIO/DeleteJob>
26​#include <KIO/CopyJob>
27​#include <KLocalizedString>
28​#include <KMessageBox>
29​#include <KProcess>
30​
31​#include <QDebug>
32​#include <QInputDialog>
33​#include <QUrl>
34​
35​void KateFileActions::renameDocumentFile(QWidget* parent, KTextEditor::Document* doc)
36​{
37​ if (!doc) {
38​ return;
39​ }
40​
41​ const QUrl oldFileUrl = doc->url();
42​
43​ if (oldFileUrl.isEmpty()) { // NEW
44​ return;
45​ }
46​
47​ const QString oldFileName = doc->url().fileName();
48​ bool ok;
49​
50​ QString newFileName = QInputDialog::getText(parent, // ADAPTED
51​ i18n("Rename file"), i18n("New file name"), QLineEdit::Normal, oldFileName, &ok);
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…
52​ if (!ok) {
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​ return;
54​ }
55​
56​ QUrl newFileUrl = oldFileUrl.adjusted(QUrl::RemoveFilename | QUrl::StripTrailingSlash);
57​ newFileUrl.setPath(newFileUrl.path() + QLatin1Char('/') + newFileName);
58​
59​ if (!newFileUrl.isValid()) {
60​ return;
61​ }
62​
63​ if (!doc->closeUrl()) {
64​ return;
65​ }
66​
67​ doc->waitSaveComplete();
68​
69​ KIO::CopyJob* job = KIO::move(oldFileUrl, newFileUrl);
70​ QSharedPointer<QMetaObject::Connection> sc(new QMetaObject::Connection());
71​ auto success = [doc, sc] (KIO::Job*, const QUrl&, const QUrl &realNewFileUrl, const QDateTime&, bool, bool) {
72​ doc->openUrl(realNewFileUrl);
73​ doc->documentSavedOrUploaded(doc, true);
74​ QObject::disconnect(*sc);
75​ };
76​ *sc = parent->connect(job, &KIO::CopyJob::copyingDone, doc, success);
77​
78​ if (!job->exec()) {
79​ KMessageBox::sorry(parent, i18n("File \"%1\" could not be moved to \"%2\"", oldFileUrl.toDisplayString(), newFileUrl.toDisplayString()));
80​ doc->openUrl(oldFileUrl);
81​ }
82​}
83​
84​void KateFileActions::removeDocumentFile(QWidget* parent, KTextEditor::Document* doc)
85​{
86​ if (!doc) {
87​ return;
88​ }
89​
90​ const auto& url = doc->url();
91​
92​ if (url.isEmpty()) { // NEW
93​ return;
94​ }
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​ bool go = (KMessageBox::warningContinueCancel(parent,
97​ i18n("Do you really want to delete file \"%1\"?", url.toDisplayString()),
98​ i18n("Delete file"),
99​ KStandardGuiItem::yes(), KStandardGuiItem::no(), QLatin1String("filetreedeletefile")
100​ ) == KMessageBox::Continue);
101​
102​ if (!go) {
103​ return;
104​ }
105​
106​ if (!KTextEditor::Editor::instance()->application()->closeDocument(doc)) {
107​ return; // no extra message, the internals of ktexteditor should take care of that.
108​ }
109​
110​ if (url.isValid()) {
111​ KIO::DeleteJob *job = KIO::del(url);
112​ if (!job->exec()) {
113​ KMessageBox::sorry(parent, i18n("File \"%1\" could not be deleted.", url.toDisplayString()));
114​ }
115​ }
116​}
117​
118​std::vector<std::pair<QString, bool>> KateFileActions::supportedDiffTools()
119​{
120​ std::vector<std::pair<QString, bool>> resultList;
121​
122​ resultList.push_back({ QStringLiteral("kdiff3"), true });
123​ resultList.push_back({ QStringLiteral("kompare"), true });
124​ resultList.push_back({ QStringLiteral("meld"), true });
125​
126​ return resultList;
127​}
128​
129​void KateFileActions::compareWithExternalProgram(KTextEditor::Document* documentA, KTextEditor::Document* documentB, const QString& diffExecutable)
130​{
131​ qDebug() << documentA->url() << documentB->url();
132​
133​ KProcess process;
134​ process << diffExecutable << documentA->url().toLocalFile() << documentB->url().toLocalFile();
135​ process.startDetached();
136​}
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…