D16484: [WIP] Add scratchpad plugin

[Sven Brauch]

brauch added inline comments.

INLINE COMMENTS

> CMakeLists.txt:2
> +add_definitions(-DTRANSLATION_DOMAIN=\"kdevscratchpad\")
> +set(scratchpad_SRCS
> +    scratchpad.cpp

In general I would personally omit this variable step in modern CMake code, there is no reason for it. Just list the sources directly in the command adding the target. If you need them elsewhere, they are easily retrieved with get_target_property().

> scratchpad.cpp:141
> +    } else {
> +        emit actionFailed(i18n("Failed removing scratch: %1", index.data().toString()));
> +    }

"failed to remove"

> scratchpad.cpp:147
> +{
> +    QString name = QStringLiteral("scratch1.cpp");
> +    for (int i = 2; !m_model->findItems(name).isEmpty(); ++i) {

This is too C++-specific for a general KDevelop plugin.

> scratchpad.cpp:155
> +        file.close();
> +    }
> +

else { // handle error }

> scratchpad.cpp:164
> +        m_model->setData(index, previousName); // undo
> +        emit actionFailed(i18n("Failed renaming scratch: Names must not include '/'"));
> +        return;

failed to rename

> scratchpadjob.cpp:48
> +
> +    m_process->setShellCommand(substituted);
> +    setCapabilities(Killable);

You need to check whether the command is non-empty, otherwise you create unkillable zombie processes.

> scratchpadjob.cpp:52
> +    setStandardToolView(KDevelop::IOutputView::RunView);
> +    setTitle(i18n("scratch:%1", item->text()));
> +

I think the i18n calls with the word "scratch" need a context, otherwise translators will not know what's going on here

> scratchpadjob.cpp:62
> +    m_process->setOutputChannelMode(KProcess::MergedChannels);
> +    connect(m_process, static_cast<void(KProcess::*)(int, QProcess::ExitStatus)>(&KProcess::finished),
> +            this, &ScratchpadJob::processFinished);

QOverload<int, QProcess::ExitStatus>::of(&KProcess::finished)

> scratchpadjob.cpp:92
> +    qCDebug(PLUGIN_SCRATCHPAD) << "process encountered error" << error;
> +    outputModel()->appendLine(i18n("Process encountered error: %1.",
> +                                   QLatin1String(QMetaEnum::fromType<QProcess::ProcessError>().valueToKey(error))));

This is always an internal error, i.e. this plugin doing something wrong. I think it should be presented to the user as such.

> scratchpadview.cpp:176
> +    auto config = KSharedConfig::openConfig()->group("Scratchpad");
> +    config.writeEntry("command", commandWidget->text());
> +}

probably the command should be per scratch ...?

REPOSITORY
  R32 KDevelop

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

To: amhndu
Cc: brauch, kdevelop-devel, glebaccon, antismap, iodelay, vbspam, geetamc, Pilzschaf, akshaydeo, surgenight, arrowd
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20181029/629f9eeb/attachment-0001.html>