Open externally called files/directories in new tabs
Needs ReviewPublic

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

Details

Reviewers
elvisangelaccio
Group Reviewers
Dolphin
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 12040
Build 12058: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes

--new-window will be more readable, about me. Add braces on one line conditions, tryRise should be raise ('try' does not make sense).

Ok will add braces once we get a review of whether the patch should remain as an option.

The 'try' is there for two reasons. Firstly, raise() is already a function ( that is inherited) in DolphinMainWindow. Secondly, Qt does not allow you to raise a minimised a window above all others without you explicitly clicking on it in the task manager, or some other way. Although, KWindowSystem seems to have hacked something together to allow us to do so, as it makes sense in this case. As I was making this patch and reading around, the window may not always be raised, depending on the window manager.

Thanks for the patch. Before I look at the code: why a new option for this feature?

We should add new options only if there is a really good reason, because the more options we have the more complex the codebase becomes.

Also, looking at the bugzilla ticket it seems that this feature was already in konqueror and it was not ported to dolphin because it didn't have tabs at the time. That's one more reason to not make an option out of this.

Because it will be a big (but positive) change for some users. Of course, there is a reason why the setting is set as default, and only the change in how the program works is why I added it as an option. I am happy to let people discuss whether it should appear as an option. I personally, would actually prefer it to not be an option.

I vote to not make it an option. There is always time to reconsider if enough people complain about the new behavior :)

I'm also okay with not making it an option for now, pending real-world user feedback. :)

feverfew updated this revision to Diff 46087.EditedNov 23 2018, 6:41 PM
  • Removed opening new tabs as an option.
feverfew updated this revision to Diff 46088.Nov 23 2018, 6:46 PM
  • Make new window option clearer, remove unnecessary string and whitespace
feverfew updated this revision to Diff 46089.Nov 23 2018, 6:46 PM
  • Added braces to one line conditions

Any progress on the state of this patch?

Any progress on the state of this patch?

I'll try to review it after Christmas

Just a little bump, hope a review comes soon :)

Can you please rebase the patch? There is new code in dolphinmainwindowtest which needs to be adapted.

I'm not sure if I'm testing this patch the wrong way, but it doesn't seem to work here. What I did:

  1. Start patched dolphin (make sure no other instances are running)
  2. Call the ShowFolders method from QDBusViewer

The folder is opened in a new window, not in a new tab in the already running dolphin process.

Also, can you please update the Summary and the Test Plan? We should remove mentions of the option (which we dropped) and we should use FEATURE: instead of BUG:.

@feverfew, are you still around to work on these issues?

@feverfew, are you still around to work on these issues?

@ngraham Yes, sorry I've been incredibly busy recently with university and work. The bug that @elvisangelaccio has found has an easy fix, I just mad a bad assumption in one of the methods. I'll allocate some time to fix the issue by the end of the coming week.

Thanks, but don't let your studies suffer! :)

feverfew updated this revision to Diff 53092.Mar 4 2019, 12:18 AM
  • Fixed ShowFolder method
  • Merge remote-tracking branch 'origin' into DBusTabInstance
  • Refactoring test case
feverfew edited the summary of this revision. (Show Details)Mar 4 2019, 1:20 AM
feverfew edited the test plan for this revision. (Show Details)

I'm not sure if I'm testing this patch the wrong way, but it doesn't seem to work here. What I did:

  1. Start patched dolphin (make sure no other instances are running)
  2. Call the ShowFolders method from QDBusViewer

    The folder is opened in a new window, not in a new tab in the already running dolphin process.

That is the correct way to test my DBus changes (although, this is not the only way to externally open a new folder/tab). This bug has now been fixed.

However after merging the KF5 min version has changed to 5.56 and so I am unfortunately am unable to test the code atm and don't have the time to switch disros. On that note, I currently use Kubuntu 18.10, any recommendations on a distro which doesn't have archaic versions of KDE frameworks/applications. I was thinking OpenSUSE TumbleWeed or KDE Neon, thoughts?

However after merging the KF5 min version has changed to 5.56 and so I am unfortunately am unable to test the code atm and don't have the time to switch disros. On that note, I currently use Kubuntu 18.10, any recommendations on a distro which doesn't have archaic versions of KDE frameworks/applications. I was thinking OpenSUSE TumbleWeed or KDE Neon, thoughts?

Easy-but-might-not-work option: change the minimum frameworks version in CMakeLists.txt to match the latest version that you have on disk. If the version was bumped just because of new icons or something, this will work. If Dolphin then fails to build because the dependency was bumped because of using new API in KIO or something, then you'll probably to use the...

Harder-but-more-correct option: build all the frameworks from source. It's not actually hard at all. Here's how: https://community.kde.org/Get_Involved/development#One-time_setup:_your_development_environment. I'm generally available to help on the #kde-devel IRC/Matrix channel. If you don't want to do this, then there's always the...

Easy-but-uses-a-ton-of-disk-space option: Put KDE Neon dev unstable in a VM and develop in that. When you compile, install to /usr. Boom, done. This does tend to take up quite a lot of space, though.

However after merging the KF5 min version has changed to 5.56 and so I am unfortunately am unable to test the code atm and don't have the time to switch disros. On that note, I currently use Kubuntu 18.10, any recommendations on a distro which doesn't have archaic versions of KDE frameworks/applications. I was thinking OpenSUSE TumbleWeed or KDE Neon, thoughts?

Easy-but-might-not-work option: change the minimum frameworks version in CMakeLists.txt to match the latest version that you have on disk. If the version was bumped just because of new icons or something, this will work. If Dolphin then fails to build because the dependency was bumped because of using new API in KIO or something, then you'll probably to use the...

Harder-but-more-correct option: build all the frameworks from source. It's not actually hard at all. Here's how: https://community.kde.org/Get_Involved/development#One-time_setup:_your_development_environment. I'm generally available to help on the #kde-devel IRC/Matrix channel. If you don't want to do this, then there's always the...

Easy-but-uses-a-ton-of-disk-space option: Put KDE Neon dev unstable in a VM and develop in that. When you compile, install to /usr. Boom, done. This does tend to take up quite a lot of space, though.

Thanks, I decided to build Qt from source and rebuilt KF5 which has helped a lot but I'm getting a compiler error, in particular undefined references to several KFilePlaceEditDialog methods in placespanel.cpp:

I'm not entirely sure why this linker error is occurring as I do have KIO with the required minimum version and I can tell from the cmake.log that it is using the libraries that I built, not the distribution ones.

You should probably ask in the #kde-devel Freenode IRC/Matrix channel so we don't clutter up the review comments section here.

feverfew updated this revision to Diff 54902.Mar 26 2019, 11:21 PM
  • Merge branch 'master' into arcpatch-D16648
feverfew edited the test plan for this revision. (Show Details)Mar 26 2019, 11:23 PM

Also, can you please update the Summary and the Test Plan? We should remove mentions of the option (which we dropped) and we should use FEATURE: instead of BUG:.

Ok I've merged master into this branch and the patch works as expected. Ready for code review

@elvisangelaccio is this the patch we're going with?

elvisangelaccio requested changes to this revision.Apr 11 2019, 8:48 PM

@elvisangelaccio is this the patch we're going with?

Yes! :)

src/dbusinterface.cpp
39

Why drop the Q_UNUSED ? This variable is still unused.

47–48
  • Please add const
  • Please prefer QCoreApplication::applicationPid()
  • Please use QStringLiteral("org.kde.dolphin-%1").arg(...)
src/dolphinmainwindow.cpp
217

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().

src/global.cpp
54–55

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

156

QCoreApplication::applicationPid()

161

Brace should start at the end of the previous line

163

Brace should start at the end of the previous line

167–168

Missing braces

src/main.cpp
46–49

Please move these includes after #include <QCommandLineParser>

147

Unrelated change. Please keep append().

This revision now requires changes to proceed.Apr 11 2019, 8:48 PM
feverfew updated this revision to Diff 56121.Apr 13 2019, 9:31 AM
  • Addressing code review comments
feverfew updated this revision to Diff 56122.Apr 13 2019, 9:33 AM
  • Adding back macro
feverfew marked 9 inline comments as done.Apr 13 2019, 9:40 AM
feverfew added inline comments.
src/dolphinmainwindow.cpp
217

The problem with activateWindow() is that it doesn't raise the file manager - only a yellow background is put on the taskbar of the dolphin instance that the tab was opened to. In the current version of Dolphin, when we ask to open a folder in an external application a new instance of dolphin is opened, hence why it is raised to the top. For me not being able to raise the window to the top creates a bad UX as usually they want to see the folder they opened straight away, instead of having to click for the file manager after they've asked to see the folder. I think we should open this up for discussion.

Re activateWindow() vs forceActiveWindow(): some of the @kwin people or @davidedmundson may be able to comment, but IIRC activateWindow() does work if you go through the proper DBus channels, though I can't recall what those are at the moment

src/dbusinterface.cpp
47–49

This could be in a single line ;)

src/dolphinmainwindow.cpp
217

Well, the documentation is pretty clear:

There are two ways how to activate a window, by calling activateWindow() and forceActiveWindow(). Generally, applications shouldn't make attempts to explicitly activate their windows, and instead let the user to activate them. In the special cases where this may be needed, applications should use activateWindow(). Window manager may consider whether this request wouldn't result in focus stealing, which would be obtrusive, and may refuse the request.

The usage of forceActiveWindow() is meant only for pagers and similar tools, which represent direct user actions related to window manipulation. Except for rare cases, this request will be always honored, and normal applications are forbidden to use it.

If this results in a bad UX, it should be probably addressed in plasma.

src/global.h
50

Missing pass-by-const-reference for urls

This made me think: what about non-plasma systems?
e.g. GNOME doesn't even have a taskbar, what's gonna happen there?

Another thing: can you have a look about these warnings?

Skipped method "changeUrl" : Type not registered with QtDBus in parameter list: QUrl
Skipped method "slotTerminalDirectoryChanged" : Type not registered with QtDBus in parameter list: QUrl
Unsupported return type 65 QPixmap in method "grab"
Unsupported return type 65 QPixmap in method "grab"

This made me think: what about non-plasma systems?
e.g. GNOME doesn't even have a taskbar, what's gonna happen there?

Exactly. I have a feeling this will be a hard thing for us to send to Plasma for fix. How would Plasma be able to understand if an activateWindow was actioned by a user or by say, a background process? In our case I believe it is ok to use KWindowSystem::forceActivateWindow as it is only invoked when specifically requested by the user (i.e. only invoked when someone executes the dolphin executable or asks to open a folder/item via the org.freedesktop.FileManager1.ShowFolders/ShowItems DBus method. Note that a big inspiration in implementing this feature was derived from how Okular does it, in particular this snippet (https://github.com/KDE/okular/blob/a16c0be349dd0484f5d8e6174db594a52779c585/shell/shell.cpp#L488-L491).

davidedmundson added a comment.EditedApr 15 2019, 2:02 PM

activateWindow works if and only if your X startup info is correct.

When you launch dolphin through a UI (on gnome or Plasma) you will see an extra environment variable on your dolphin process.

DESKTOP_STARTUP_ID=David-Desktop;1555336591;362121;16299_TIME219914845

Qt normally handles this internally.

If you then call into an existing dolphin you need to pass this and import it the other side before calling window->activate().
See KStartupInfo or QX11Extras::setAppTime

Plasma isn't really involved.

fbg13 added a subscriber: fbg13.Apr 18 2019, 7:06 PM

Currently opening an already open path creates a new tab.
I think it's better to just activate the existing tab.

activateWindow works if and only if your X startup info is correct.

When you launch dolphin through a UI (on gnome or Plasma) you will see an extra environment variable on your dolphin process.

DESKTOP_STARTUP_ID=David-Desktop;1555336591;362121;16299_TIME219914845

Qt normally handles this internally.

If you then call into an existing dolphin you need to pass this and import it the other side before calling window->activate().
See KStartupInfo or QX11Extras::setAppTime

Plasma isn't really involved.

Does this also apply if dolphin is called via the terminal? If not then I think we should stick with forceActiveWindow, otherwise I'm happy to make the changes necessary.

Currently opening an already open path creates a new tab.
I think it's better to just activate the existing tab.

Good point. Happy to implement this if everyone else is.

Currently opening an already open path creates a new tab.
I think it's better to just activate the existing tab.

Good point. Happy to implement this if everyone else is.

I think that makes sense.

magar added a subscriber: magar.Sat, May 4, 8:47 PM
fikrim added a subscriber: fikrim.Sun, May 19, 7:01 AM
feverfew updated this revision to Diff 58400.Tue, May 21, 11:28 AM
  • 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
feverfew edited the summary of this revision. (Show Details)Tue, May 21, 11:31 AM
feverfew updated this revision to Diff 58401.Tue, May 21, 11:38 AM
feverfew marked an inline comment as done.
  • Removing unnecessary include
feverfew marked 4 inline comments as done.Tue, May 21, 11:39 AM
fvogt added a subscriber: fvogt.Tue, May 21, 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.Tue, May 21, 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.EditedTue, May 21, 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.Tue, May 21, 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.Wed, May 22, 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
87

!preferredService.isEmpty()

127

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

143

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