D29385: Introduce KIO::OpenUrlJob, a rewrite and replacement for KRun

[Friedrich W. H. Kossebau]

kossebau added a comment.


  Quick first nitpicks from initial scan what this patch is about, without having looked closer.

INLINE COMMENTS

> openurljobtest.cpp:108
> +{
> +    std::for_each(m_filesToRemove.begin(), m_filesToRemove.end(), [](const QString & f) {
> +        QFile::remove(f);

why no simple range-based for loop?

> openurljob.h:73
> +     */
> +    void setRunFlags(ApplicationLauncherJob::RunFlags runFlags);
> +

Makes me wonder if those runflags should not be rather part of KIO namespace, to decouple classes here.

> openurljob.h:96
> +     */
> +    void setRunExecutables(bool allow);
> +

Could this and the following bool flags be turned into some flags instead? Just wondering, not looked further.

Also wondering if there should not be some getter as well, when using a flagset that would be just a single method/symbol.

> openurljob.h:127
> +     */
> +    void mimetypeFound(const QString &mimeType);
> +

-> "mimeTypeFound"? would match other casing.

> openurljobhandlerinterface.cpp:34
> +
> +OpenUrlJobHandlerInterface::~OpenUrlJobHandlerInterface()
> +{

`= default;` instead?

> openurljobhandlerinterface.h:33
> +/**
> + * @brief The OpenUrlJobHandlerInterface class allows OpenUrlJob to
> + * prompt the user about which application to use to open URLs that do not

Please prepend a line

  * @class OpenUrlJobHandlerInterface openurljobhandlerinterface.h <KIO/OpenUrlJobHandlerInterface>

at the top, to trick doxygen into documenting the full CamelCase include.

> openurljobhandlerinterface.h:54
> +     */
> +    virtual ~OpenUrlJobHandlerInterface();
> +

override instead?

> openurljobhandlerinterface.h:84
> +private:
> +    class Private;
> +    Private *const d;

Prevent use of nested Private class here, declare a class OpenUrlJobHandlerInterfacePrivate outside, also use QScopedPointer for consistency and preparedness in case there ever is an actual object.

> widgetsopenurljobhandler.h:45
> +private:
> +    class Private;
> +    Private *const d;

Do not use embedded Private class. Also pimp needed here, given not a public class?

REPOSITORY
  R241 KIO

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

To: dfaure, ahmadsamir, broulik, meven, kossebau, davidedmundson, nicolasfella, svuorela
Cc: kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20200503/e84578c6/attachment-0001.html>