D16648: Open externally called files/directories in new tabs

fvogt added a comment.


  IMO some of the slots taking a QString/QStringList instead of their QUrl counterpart should have a comment why this is the case.
  From a first glance a method `isUrlOpen(QString)` looks weird, but with a comment that would be come clearer.

INLINE COMMENTS

> global.cpp:87
> +    QVector<UrlsForService> dolphinServices;
> +    if (preferredService != "") {
> +        QSharedPointer<QDBusInterface> preferred(

`!preferredService.isEmpty()`

> global.cpp:127
> +            QDBusReply<bool> isUrlOpen = service.m_service->call(QStringLiteral("isUrlOpen"), url);
> +            if (isUrlOpen.isValid()) {
> +                if(isUrlOpen.value()) {

`if (isUrlOpen.isValid() && isUrlOpen.value()) {`

> global.cpp:143
> +        if (!service.m_urls.isEmpty()) {
> +            service.m_service->call(openFiles ? QStringLiteral("openFiles") : QStringLiteral("openDirectories"), service.m_urls, splitView);
> +            service.m_service->call(QStringLiteral("activateWindow"));

IMO this function should take a `const QList<QUrl> &urls` parameter and only convert it to a `QStringList` here for the call.

REPOSITORY
  R318 Dolphin

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

To: feverfew, #dolphin, elvisangelaccio
Cc: dfaure, fvogt, fikrim, magar, fbg13, davidedmundson, kwin, ngraham, elvisangelaccio, anthonyfieroni, kfm-devel, alexde, feverfew, meven, spoorun, navarromorales, firef, andrebarros, emmanuelp, mikesomov