D8199: Allow abortion of starting another NativeAppJob

[Milian Wolff]

mwolff requested changes to this revision.
mwolff added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> nativeappjob.cpp:139
>      auto currentJobs = ICore::self()->runController()->currentJobs();
> +    std::set<NativeAppJob*> ignoreJobs{this};
>      for (auto it = currentJobs.begin(); it != currentJobs.end();) {

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

> nativeappjob.cpp:142
>          NativeAppJob* job = findNativeJob(*it);
> -        if (job && job != this && job->m_name == m_name) {
> -            QMessageBox::StandardButton button = QMessageBox::question(nullptr, i18n("Job already running"), i18n("'%1' is already being executed. Should we kill the previous instance?", m_name));
> -            if (button != QMessageBox::No && ICore::self()->runController()->currentJobs().contains(*it)) {
> -                (*it)->kill();
> +        if (job && job->m_name == m_name && !ignoreJobs.count(job)) {
> +            switch (QMessageBox::question(nullptr, i18n("Job already running"), i18n("'%1' is already being executed. Should we kill the previous instance?", m_name), QMessageBox::Yes | QMessageBox::No | QMessageBox::Cancel)) {

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

> nativeappjob.cpp:143
> +        if (job && job->m_name == m_name && !ignoreJobs.count(job)) {
> +            switch (QMessageBox::question(nullptr, i18n("Job already running"), i18n("'%1' is already being executed. Should we kill the previous instance?", m_name), QMessageBox::Yes | QMessageBox::No | QMessageBox::Cancel)) {
> +                case QMessageBox::Yes: // kill the running instance

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

> nativeappjob.cpp:145
> +                case QMessageBox::Yes: // kill the running instance
> +                    if (ICore::self()->runController()->currentJobs().contains(*it)) {
> +                        (*it)->kill();

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

REPOSITORY
  R32 KDevelop

REVISION DETAIL
  https://phabricator.kde.org/D8199

To: croick, #kdevelop, mwolff
Cc: mwolff, kdevelop-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20171008/7fb42fd9/attachment-0001.html>