D16648: Open externally called files/directories in new tabs

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

INLINE COMMENTS

> dolphinmainwindow.cpp:207
> +{
> +    this->openDirectories(QUrl::fromStringList(dirs), splitView);
> +}

Coding style: we don't usually use `this->` since it's implicit.

> dolphinmainwindow.cpp:1728
>  
> +bool DolphinMainWindow::isUrlOpen(const QString& url) {
> +    if (m_tabWidget->getIndexByURL(QUrl(url)) >= 0) {

Coding style: opening brace should go to next line

> dolphinmainwindow.h:93
>       */
> -    void openFiles(const QList<QUrl>& files, bool splitView);
> -
> -    /**
> +    void openFiles(const QList<QUrl> &files, bool splitView);
> +    

Unrelated style change, please move it to another commit.

> dolphinmainwindow.h:95
> +    
> +    /*
>       * Returns the 'Create New...' sub menu which also can be shared

Accidental comment change?

> dolphintabwidget.cpp:386
> +{
> +    const int num_tabs = count();
> +    for (int i = 0; i < num_tabs; i++) {

Coding style: missing camelcase

Do we actually need this variable, though?

> dolphintabwidget.cpp:390
> +        // i.e. to acknowledge that ~/ is equivalent to /home/user/
> +        if (url == tabPageAt(i)->activeViewContainer()->url() ||
> +            url.toDisplayString(QUrl::StripTrailingSlash) == 

Please put `tabPageAt(i)->activeViewContainer()->url()` in a variable to simplify this condition.

> dolphintabwidget.cpp:393
> +            tabPageAt(i)->activeViewContainer()->url().toDisplayString(QUrl::StripTrailingSlash))
> +        {
> +            return i;

Coding style: opening brace should go to previous line.

> dolphintabwidget.h:85
> +     */
> +    int getIndexByURL(const QUrl& url);
>  

Missing `const`.

I'd call it `getIndexByUrl`, uppercase `URL` is less frequent in Qt API names.

> global.cpp:76-81
> +    struct UrlsForService {
> +        QSharedPointer<QDBusInterface> m_service;
> +        QStringList m_urls;
> +        UrlsForService(const QSharedPointer<QDBusInterface> &service = nullptr) 
> +            :m_service(service) {}
> +    };

Why not use `QPair` instead of defining this struct ?

> global.cpp:100
> +    // find all dolphin instances
> +    for (const QString& service: services) {
> +        if (service.startsWith(pattern) && !service.endsWith(myPid)) {

Coding style: missing space before the colon.

> global.h:25
>  #include <QUrl>
> +#include <QStringLiteral>
>  

Unnecessary include

> global.h:50
> +     */
> +    bool attachToExistingInstance(const QStringList& urls, bool openFiles, bool splitView, const QString& preferredService = "");
>  

Prefer `QString()` to `""`.

I wonder if we could add new flags to `OpenNewWindowFlag` and get rid of these bools in the signature.

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