Differential D8199 Diff 20544 plugins/execute/nativeappjob.cpp

Changeset View

Standalone View

View Options

plugins/execute/nativeappjob.cpp

Context not available.
19 19​
20#include "nativeappjob.h" 20​#include "nativeappjob.h"
21 21​
22​#include <QAbstractButton>
22#include <QFileInfo> 23​#include <QFileInfo>
23#include <QMessageBox> 24​#include <QMessageBox>
25​#include <QPointer>
24 26​
25#include <KConfigGroup> 27​#include <KConfigGroup>
26#include <KLocalizedString> 28​#include <KLocalizedString>
Context not available.
133 135​
134void NativeAppJob::start() 136​void NativeAppJob::start()
135{ 137​{
136 // we kill any execution of the configuration 138​ QVector<QPointer<NativeAppJob> > currentJobs;
mwolffUnsubmitted
Done

use a QVector please

mwolff: use a QVector please
137 auto currentJobs = ICore::self()->runController()->currentJobs(); 139​ // collect running instances of the same type
mwolffUnsubmitted
Done

use QSet, or unordered_set - the order isn't needed and the hash-based containers are faster

mwolff: use QSet, or unordered_set - the order isn't needed and the hash-based containers are faster
138 for (auto it = currentJobs.begin(); it != currentJobs.end();) { 140​ for (auto j : ICore::self()->runController()->currentJobs()) {
139 NativeAppJob* job = findNativeJob(*it); 141​ NativeAppJob* njob = findNativeJob(j);
140 if (job && job != this && job->m_name == m_name) { 142​ if (njob && njob != this && njob->m_name == m_name)
mwolffUnsubmitted
Done

I'd prefer QSet and then you could use the more expressive !contains here instead of !count

mwolff: I'd prefer QSet and then you could use the more expressive `!contains` here instead of `!count`
141 QMessageBox::StandardButton button = QMessageBox::question(nullptr, i18n("Job already running"), i18n("'%1' is already being executed. Should we kill the previous instance?", m_name)); 143​ currentJobs << njob;
mwolffUnsubmitted
Done

please put the QMessageBox::question call into a separate line, and break it up. Save the return value in a temp var and switch over it then

Also reword the message to '%1' is already being executed.. Then set proper texts on the button such that it's clear what they do:

Yes -> Kill
No -> Start Another
Cancel -> Cancel (can probably stay)

I'd also say that the default action should be cancel here, or maybe "start another", but not a destructive kill

mwolff: please put the `QMessageBox::question` call into a separate line, and break it up. Save the…
142 if (button != QMessageBox::No && ICore::self()->runController()->currentJobs().contains(*it)) { 144​ }
143 (*it)->kill(); 145​
mwolffUnsubmitted
Done

why this line? you do this, to check whether the job is still alive? If so, store it in a QPointer and check its validity

mwolff: why this line? you do this, to check whether the job is still alive? If so, store it in a…
144 } 146​ if (!currentJobs.isEmpty()) {
145 currentJobs = ICore::self()->runController()->currentJobs(); 147​ QMessageBox msgBox(QMessageBox::Question,
146 it = currentJobs.begin(); 148​ i18n("Job already running"),
147 } else { 149​ i18n("'%1' is already being executed.", m_name),
148 ++it; 150​ QMessageBox::Yes | QMessageBox::No | QMessageBox::Cancel);
151​ msgBox.button(QMessageBox::No)->setText(i18n("Kill All Instances"));
152​ msgBox.button(QMessageBox::Yes)->setText(i18n("Start Another"));
153​ msgBox.setDefaultButton(QMessageBox::Cancel);
154​
155​ switch (msgBox.exec()) {
156​ case QMessageBox::Yes: // simply start another job
157​ break;
158​ case QMessageBox::No: // kill the running instance
159​ for (auto & job : currentJobs) {
160​ if (job)
161​ job->kill();
162​ }
163​ break;
164​ default: // cancel starting a new job
165​ kill();
166​ return;
149 } 167​ }
150 } 168​ }
151 169​
Context not available.