D16648: Open externally called files/directories in new tabs

[Elvis Angelaccio]

elvisangelaccio requested changes to this revision.
elvisangelaccio added a comment.
This revision now requires changes to proceed.


  In D16648#447625 <https://phabricator.kde.org/D16648#447625>, @ngraham wrote:
  
  > @elvisangelaccio is this the patch we're going with?
  
  
  Yes! :)

INLINE COMMENTS

> dbusinterface.cpp:39
>  {
> -    Q_UNUSED(startUpId);
>      const QList<QUrl> urls = Dolphin::validateUris(uriList);

Why drop the `Q_UNUSED` ? This variable is still unused.

> dbusinterface.cpp:46
>      }
> -    Dolphin::openNewWindow(urls);
> +    QString serviceName = QString("org.kde.dolphin-") + QString::number(qApp->applicationPid());
> +    QScopedPointer<QDBusInterface> service;

- Please add `const`
- Please prefer `QCoreApplication::applicationPid()`
- Please use `QStringLiteral("org.kde.dolphin-%1").arg(...)`

> dolphinmainwindow.cpp:199
> +{
> +    KWindowSystem::forceActiveWindow( window()->effectiveWinId() );
>  }

Please use `KWindowSystem::activateWindow()` instead. Normal applications are not supposed to call `forceActiveWindow()`.

And I'd call this method `activateWindow()` as well, rather than `tryRaise()`.

> global.cpp:54-55
>  {
>      QString command = QStringLiteral("dolphin");
> +    command.append(QLatin1String(" --new-window"));
>  

Please use `QStringLiteral("dolphin --new-window")` to save a string concatenation.

> global.cpp:74
> +    const QString pattern = QStringLiteral("org.kde.dolphin-");
> +    const QString myPid = QString::number(qApp->applicationPid());
> +    QScopedPointer<QDBusInterface> bestService;

`QCoreApplication::applicationPid()`

> global.cpp:79
> +    for (const QString& service: services)
> +    {
> +        if (service.startsWith(pattern) && !service.endsWith(myPid))

Brace should start at the end of the previous line

> global.cpp:81
> +        if (service.startsWith(pattern) && !service.endsWith(myPid))
> +        {
> +            // Check if instance can handle our URLs

Brace should start at the end of the previous line

> global.cpp:85-86
> +                                                 QStringLiteral("/dolphin/Dolphin_1"), QStringLiteral("org.kde.dolphin.MainWindow")));
> +            if(!bestService->isValid()) 
> +                break;
> +

Missing braces

> main.cpp:42-45
> +#include <QDBusConnection>
> +#include <QDBusInterface>
> +#include <QDBusAbstractInterface>
> +#include <QDBusConnectionInterface>

Please move these includes after `#include <QCommandLineParser>`

> main.cpp:147
>          // We need at least one URL to open Dolphin
> -        urls.append(Dolphin::homeUrl());
> +        urls.push_back(Dolphin::homeUrl().toString());
>      }

Unrelated change. Please keep `append()`.

REPOSITORY
  R318 Dolphin

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

To: feverfew, #dolphin, elvisangelaccio
Cc: ngraham, elvisangelaccio, anthonyfieroni, kfm-devel, alexde, feverfew, meven, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20190411/0e70fac9/attachment.htm>