Open externally called files/directories in new tabs
ClosedPublic

Authored by feverfew on Nov 3 2018, 6:43 PM.

Details

Summary

FEATURE: 183429
FIXED-IN: 19.08.0

Externally called files/directories are opened in a a new tab of an instance of Dolphin that already exists. If any of the given URIs are already open in a tab, then those tabs are activated instead of a new tab being opened. If there is no instance then the files/directories are opened in a new window. The newly opened file/directory has its tab activated, and consequently, the window is also activated.

When the user clicks "Open In New Window" or "Detach Tab", the files/directories are opened in a new window.

Test Plan

[Manual]
Before testing, set the default file manager in system settings as the newly built Dolphin executable.
One must also include the new dolphin executable in the $PATH, otherwise some functions will attempt to open the system dolphin instead of the new one.

Furthermore, running two different versions of Dolphin (in particular, where one does not have this patch included) can result in bugs appearing, in particular, new tabs not opening as old instances will not recognise the DBus commands sent to it. However, I see no reason why a user will have two different versions of Dolphin (apart from people like us :D).

Open directories with the help of auxillary programs (i.e. a browser). The files/directories should appear in a new window if an instance does not exist. If an existence already exists, then a new tab should be opened and activated in that instance and the window activated.
Use QDBusViewer to open folders/items by calling the ShowFolders/ShowItems methods in org.freedesktop.FileManager1 of the Dolphin instance.
When a user chooses to "Open In New Window"/"Detach Tab" then the files/directories should be opened in a new window.

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D16648
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 12260
Build 12278: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
feverfew marked 4 inline comments as done.May 21 2019, 11:39 AM
fvogt added a subscriber: fvogt.May 21 2019, 12:17 PM

It looks like several QUrls were changed to QString here, why? Most of those QStrings, if not all, actually contain URLs.

It looks like several QUrls were changed to QString here, why? Most of those QStrings, if not all, actually contain URLs.

If I remember correctly Qt methods that are publicised on DBus cannot have parameters as QList<QUrl>, hence I had to use QStringList. This seems to happen often enough that Qt even has helper functions to do the conversion. There are some more conversions to avoid unnecessarily changing method signatures of existing functions.

fvogt added a comment.May 21 2019, 5:35 PM

It looks like several QUrls were changed to QString here, why? Most of those QStrings, if not all, actually contain URLs.

If I remember correctly Qt methods that are publicised on DBus cannot have parameters as QList<QUrl>, hence I had to use QStringList. This seems to happen often enough that Qt even has helper functions to do the conversion. There are some more conversions to avoid unnecessarily changing method signatures of existing functions.

Not quite sure whether Qt supports that, but you could overload the methods of MainWindow so that the slots accept a QStringList and just pass it on.

It looks like several QUrls were changed to QString here, why? Most of those QStrings, if not all, actually contain URLs.

If I remember correctly Qt methods that are publicised on DBus cannot have parameters as QList<QUrl>, hence I had to use QStringList. This seems to happen often enough that Qt even has helper functions to do the conversion. There are some more conversions to avoid unnecessarily changing method signatures of existing functions.

Not quite sure whether Qt supports that, but you could overload the methods of MainWindow so that the slots accept a QStringList and just pass it on.

QUrl::fromStringList and QUrl::toStringList were created for the express purpose of dealing with DBus calls: https://gitlab.com/pteam/pteam-qtbase/commit/6b9545a980f09d8fb034d930cfe67f6110357d0c.
In fact @dfaure was the one who created these functions himself :)
I think I'll stick with using those static methods unless there is a different reason to objection.

fvogt added a comment.EditedMay 21 2019, 5:57 PM

It looks like several QUrls were changed to QString here, why? Most of those QStrings, if not all, actually contain URLs.

If I remember correctly Qt methods that are publicised on DBus cannot have parameters as QList<QUrl>, hence I had to use QStringList. This seems to happen often enough that Qt even has helper functions to do the conversion. There are some more conversions to avoid unnecessarily changing method signatures of existing functions.

Not quite sure whether Qt supports that, but you could overload the methods of MainWindow so that the slots accept a QStringList and just pass it on.

QUrl::fromStringList and QUrl::toStringList were created for the express purpose of dealing with DBus calls: https://gitlab.com/pteam/pteam-qtbase/commit/6b9545a980f09d8fb034d930cfe67f6110357d0c.
In fact @dfaure was the one who created these functions himself :)
I think I'll stick with using those static methods unless there is a different reason to objection.

I think you misunderstand. I'm suggesting adding overloads like this:

void DolphinMainWindow::openDirectories(const QList<QUrl>& dirs, bool splitView)
{
    m_tabWidget->openDirectories(dirs, splitView);
}

void DolphinMainWindow::openDirectories(const QStringList& dirs, bool splitView)
{
    this->openDirectories(QUrl::fromStringList(dirs), splitView);
}
feverfew updated this revision to Diff 58447.May 21 2019, 11:12 PM
  • Addressing code review comments
  • Adding back macro
  • Allow window activation without forceActiveWindow()
  • Avoiding duplication of tabs
  • Distributing URLs among all instances
  • Making ShowFolders/ShowItems also check if URLs are open in other tabs
  • Ensuring current instance is considered when needed
  • Removing unnecessary include
  • Overloading openFiles/openDirectories
  • Removing unnecessary new line
  • Amend test
  • Fixing header file
fvogt added a comment.May 22 2019, 7:02 AM

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.

src/global.cpp
147

!preferredService.isEmpty()

187

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

203

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

feverfew added inline comments.May 23 2019, 10:54 AM
src/global.cpp
187

This might be dangerous? The wording of the docs doesn't make it too clear: https://doc.qt.io/qt-5/qdbusreply.html#value

elvisangelaccio requested changes to this revision.May 23 2019, 8:40 PM
elvisangelaccio added inline comments.
src/dolphinmainwindow.cpp
207

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

1728

Coding style: opening brace should go to next line

src/dolphinmainwindow.h
95

Unrelated style change, please move it to another commit.

97

Accidental comment change?

src/dolphintabwidget.cpp
386

Coding style: missing camelcase

Do we actually need this variable, though?

390

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

393

Coding style: opening brace should go to previous line.

src/dolphintabwidget.h
85

Missing const.

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

src/global.cpp
136–143

Why not use QPair instead of defining this struct ?

160

Coding style: missing space before the colon.

src/global.h
25

Unnecessary include

49

Prefer QString() to "".

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

This revision now requires changes to proceed.May 23 2019, 8:40 PM
feverfew updated this revision to Diff 58618.May 24 2019, 5:53 PM
  • Styling fixes
feverfew updated this revision to Diff 58619.May 24 2019, 5:56 PM
  • Simplifying conditional
feverfew updated this revision to Diff 58620.May 24 2019, 6:02 PM
feverfew marked 14 inline comments as done.
  • Coding style change
feverfew marked an inline comment as done.May 24 2019, 6:03 PM
feverfew added inline comments.
src/global.cpp
187

Even though isUrlOpen.value() may give an "undefined" value, it doesn't matter as isUrlOpen.isValid() will be false.

src/global.h
49

At a cursory glance, might make it more messier as we'll need if conditionals as we can't set flags over DBus.

Almost there :)

src/dolphinmainwindow.cpp
1730

We should use QUrl::fromUserInput() here.

Hmm, I just noticed that KWindowSystem::activateWindow() doesn't seem to work on Wayland.

@davidedmundson Any idea what we might be missing?

zzag added a subscriber: zzag.EditedMay 27 2019, 12:32 PM

Wayland clients can't raise or activate themselves (so maybe ignore that for now?).

If I recall correctly, @davidedmundson proposed a protocol (for rfc) some time ago on wayland-devel that would allow a wayland client to pass the focus to another client.

In D16648#470609, @zzag wrote:

Wayland clients can't raise or activate themselves (so maybe ignore that for now?).

If I recall correctly, @davidedmundson proposed a protocol (for rfc) some time ago on wayland-devel that would a wayland client to pass the focus to another client.

That's unfortunate. We can't just ignore it, the whole feature is built upon the assumption we can raise an existing dolphin window...

@feverfew Could you add a fallback path that opens a new windows if KWindowSystem::isPlatformWayland() is true?

feverfew updated this revision to Diff 58743.May 27 2019, 11:22 PM
  • Adding fallback to always opening new window if running in Wayland session
feverfew updated this revision to Diff 58744.May 27 2019, 11:26 PM
  • Implementing code review suggestion
feverfew marked 2 inline comments as done.May 27 2019, 11:26 PM
fvogt added inline comments.May 28 2019, 7:09 AM
src/global.cpp
140

As Wayland and X11 clients can be part of the same session and thus the same DBus, it would be more accurate to ask the running dolphin whether it's using Wayland or XCB. It would require a new DBus call though, so probably too complex and not worth it though.

elvisangelaccio accepted this revision.May 28 2019, 7:27 PM

Tested also on gnome, window activation works fine also there.

@feverfew Do you have commit access?

This revision is now accepted and ready to land.May 28 2019, 7:27 PM

Tested also on gnome, window activation works fine also there.

@feverfew Do you have commit access?

No.
Alexander Saoutkin
a.saoutkin@gmail.com

elvisangelaccio requested changes to this revision.May 29 2019, 9:22 PM

Sorry, one last thing: can you fix the trailing white spaces? The phabricator UI doesn't show them, but there are a lot of them (check with git diff).

Thanks!

This revision now requires changes to proceed.May 29 2019, 9:22 PM
feverfew updated this revision to Diff 58869.May 29 2019, 9:37 PM
  • Removing trailing whitespace
feverfew updated this revision to Diff 58870.May 29 2019, 9:41 PM
  • Removing trailing whitespace
elvisangelaccio accepted this revision.May 30 2019, 8:45 PM
This revision is now accepted and ready to land.May 30 2019, 8:45 PM
This revision was automatically updated to reflect the committed changes.
thomasp reopened this revision.Jun 2 2019, 12:10 PM
thomasp added a subscriber: thomasp.
thomasp added inline comments.
src/global.cpp
84

Prepend '-' to myPid so it does not accidentally skip pid {1123, 2123, ...} when the applicationPid is 123. (service.endsWith(myPid) on line 99)

This revision is now accepted and ready to land.Jun 2 2019, 12:10 PM

Reopening because I believe this is broken in combination with activities.

Problem 1:
You can not start a new Dolphin window on any activity as long as you have Dolphin open on any other activity.

Steps to reproduce:
Launch Dolphin on activity 1.
Launch Dolphin on activity 2.
Expected behaviour: Dolphin is running on activity 2.
Actual behaviour: your current activity is switched from 2 to 1.
Activities are meant to separate different tasks. Opening a tab on a different activity defeats the purpose.
Workaround: Open a new tab on the activity you were switched to. Detach the tab and move the new window to the activity you wanted Dolphin to open on.
Workaround 2: don't use the GUI to launch Dolphin but type dolphin --new-window into KRunner or Konsole.

Problem 2:
You are moved to a different activity if there happens to be a window with the path to open.

Steps to reproduce:
Launch Dolphin on activity 1.
Launch Dolphin on activity 2 (same path as on activity 1).
Make sure Dolphin on activity 2 has a DBus name sorting after activity 1.
Launch Dolphin on activity 2.
Expected behaviour: Switch to Dolphin on activity 2.
Actual behaviour: You are switched to Dolphin on activity 1.

Problem 3:
You are moved to a different activity if there happens to be a Dolphin window with the path to open (2).

Steps to reproduce:
Launch Dolphin on activity 1 with path /foo.
Launch Dolphin on activity 2 with path /bar.
Make sure Dolphin on activity 2 has a DBus name sorting before activity 1.
Launch Dolphin on activity 2 with path /foo.
Expected behaviour: The path /foo is opened in a tab on activity 2.
Actual behaviour: You are switched to activity 1.

Problem 4:
You are moved to a different activity if there happens to be a Dolphin window on a different activity where the DBus name sorts before the Dolphin window of the current activity.

Steps to reproduce:
Launch Dolphin on activity 1 with path /foo.
Launch Dolphin on activity 2 with path /bar.
Make sure Dolphin on activity 2 has a DBus name sorting after activity 1.
Launch Dolphin on activity 2 with path /baz.
Expected behaviour: The path /baz is opened in a tab on activity 2.
Actual behaviour: You are switched to activity 1 where /baz is opened in a new tab.

All of the above boils down to pseudo random behaviour if you have multiple activities and multiple Dolphin windows.

If you are used to the behaviour of Firefox which opens new URLs in the most recently used window the implemented behaviour is odd even on single activity/desktop setups since all new tab request (with unique URL) are opened in the window with the lowest DBus unique connection name.

Thank you for your consideration.
And thank you for putting all the work into KDE.

Note: DBus probably returns the services ordered by their connection names and not sorted by well-known name, so the first started dolphin window might always be considered the 'main' window even if the pid wraps around.

Thanks for the feedback. We haven't tested Activities indeed, sorry about that.

I'm not sure if those problems can be easily fixed. Maybe we should just make the feature configurable (people on reddit are already asking the option anyway) so that you could turn it off if you are an heavy Activities user.

What do you think?

Sounds like we should make the feature only function within the current Activity or Virtual Desktop; Dolphin instances in another one should not be touched and a new instance should be opened in the current Activity/VD instead.

Regardless, we should probably make it optional because I can imagine that some people just wouldn't like it.

I was about to suggest about the same, but ngraham beat me to it.
If it is easy to find out which Activity/VD a Dolphin window belongs to, then additionally filtering for those values in

Dolphin::attachToExistingInstance

should do the trick.

thomasp requested changes to this revision.Jun 2 2019, 6:11 PM

I failed to request changes for D16648#inline-121014.
This is bound to randomly exhibit strange behaviour.

This revision now requires changes to proceed.Jun 2 2019, 6:11 PM

Activity id can be appended to dbus name, so it will be trivial to check.

I failed to request changes for D16648#inline-121014.
This is bound to randomly exhibit strange behaviour.

Open distinct PR.

thomasp accepted this revision.Jun 2 2019, 6:48 PM

Opened distinct PR D21547.

This revision is now accepted and ready to land.Jun 2 2019, 6:48 PM

@feverfew this appears to have caused https://bugs.kde.org/show_bug.cgi?id=408244. Can you fix that? Thanks!

rjvbb added a subscriber: rjvbb.Apr 22 2020, 9:22 AM

A thought: shouldn't the KRun::run* functions use QCoreApplication::applicationFilePath() instead of invoking "dolphin" and hope the path leads to the same application?

I haven't read through the entire review to see if there has been discussion of this (beyond the allusions to having different versions in the test plan) nor what has been decided, but:

  • I'd expect "open in new window" to behave as if it opened a new window in the same application (and not, for instance, give me the KDE4 version if for some reason I'm testing Dolphin 5 on a system that still runs a Plasma 4 desktop)
  • typically there will not be a dolphin executable on the path when running on a Mac, nor on MS Windows. On the latter "open in new window" seems to work (as long as the application runs in its installation directory?), on the former the feature fails with the expected error that dolphin cannot be found.

(I'm bringing this up here because it's the latest discussed, relevant change to Dolphin::openNewWindow().)

fikrim removed a subscriber: fikrim.Apr 22 2020, 9:22 AM