In D22708#516132, @ngraham wrote:

In D22708#516131, @feverfew wrote:

In D22708#516128, @ngraham wrote:

Also feel free to pop by #kde-fm for discussions or if you want to ask a question.

It kicks me out for not having a registered nickname even though I totally do and am using it. If it's not necessary anymore, it might be nice to remove that restriction.

In D22708#516128, @ngraham wrote:

Oh good. :) Is there anywhere I can follow the progress?

In D22708#507449, @ngraham wrote:

What?

Making comments clearer

In D11382#513245, @ngraham wrote:

This is ready for re-review, with one specific thing I could use help debugging: when you enable the new option, launching Dolphin and opening new windows becomes interminably slow, probably due to some extra DBus overhead that's still a bit beyond my comprehension. See the FIXME in the code.

Unnecessary min version upgraded

Addressing comments

Can't test, but looks sane to me. I have a small comment, but can be left for later if time is of the essence.

Also would like to see a push for advertising KDE Apps on Windows and Mac OS, where possible. This will bring us a wider audience, and can even help switch people to Linux.

I also believe T11093 can tie very well with this and T11080. If we brainstorm properly we can standardise certain widgets such that many goals are met at once, with a decreased maintenance burden for everyone involved.

Note that this goal is necessary for T11080 to succeed. It's quite common in a lot of companies/governments that they make suitable provisions for those with disabilities, where possible.

In D21736#484989, @ngraham wrote:

Thanks, the issue is fixed now. I could not find any other behavioral regressions, and I've verified that the new feature behaves correctly. The code looks good to me too!

Next, do you think you could work on https://bugs.kde.org/show_bug.cgi?id=408919?

Rebasing onto master

• Simplifying option
• Fixing regression

In D21736#484695, @ngraham wrote:

In D21736#484694, @feverfew wrote:

In D21736#484693, @ngraham wrote:

Nope, same behavior after a rebase on current master.

Be careful when you say nope/yes ;) was very confused if you meant rebasing it solved it or not.

Oh gosh, I'm sorry. I'm too tired for this right now and need to go to bed.

So for example if you open a jpg, and ask to open in containing folder it'll reopen in gwenview and also be selected in Dolphin?

Almost. Here's a video that shows the issue (with apologies to @broulik):

• Fixing regression

In D21736#484693, @ngraham wrote:

Nope, same behavior after a rebase on current master.

In D21736#484271, @ngraham wrote:

Ran into an interesting bug with this. With the feature turned off, using the "Open Containing Folder" feature in many KDE apps causes a new Dolphin window to open with the path of the file itself, not its containing folder. As a result, the file gets opened a second time.

With the feature turned on, everything works properly and the file's containing folder is opened in a new tab in the running Dolphin instance

@elvisangelaccio note I can't actually commit this as I don't have commit access.

• Simplifying option

• Making comment clearer

In D21736#478953, @ngraham wrote:

Thanks! Will review once the feature in and of itself works reliably enough for me to test it in various use cases; first I'd like to see https://bugs.kde.org/show_bug.cgi?id=408244 and https://bugs.kde.org/show_bug.cgi?id=408387 fixed.

Fix identation

• Removing trailing whitespace

• Removing trailing whitespace

In D16648#471347, @elvisangelaccio wrote:

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

@feverfew Do you have commit access?

• Implementing code review suggestion

• Adding fallback to always opening new window if running in Wayland session

• Coding style change

• Simplifying conditional

• Styling fixes

• 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

In D16648#468028, @fvogt wrote:

In D16648#467893, @feverfew wrote:

In D16648#467854, @fvogt wrote:

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.

In D16648#467854, @fvogt wrote:

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

• Removing unnecessary include

• 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

In D16648#450590, @davidedmundson wrote:

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.

In D16648#449696, @elvisangelaccio wrote:

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

• Adding back macro

• Addressing code review comments

In T10263#180285, @fvogt wrote:

In T10263#180221, @feverfew wrote:

In T10263#178973, @fvogt wrote:

@feverfew Sure! I don't know much about GSoC though, I hope that's not an issue.

@fvogt Great, I can tell you more about it on a platform of your choice. What's your preferred method of contact (.e.g email) so we can get a project submitted (deadline is 9th April)?

E-Mail is fine - you can use the one here on phab.

In D16648#404141, @elvisangelaccio wrote:

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:.

• Merge branch 'master' into arcpatch-D16648

In T10263#178973, @fvogt wrote:

@feverfew Sure! I don't know much about GSoC though, I hope that's not an issue.

@fvogt I have some free time this summer and would like to help out in getting https://gitlab.com/Vogtinator/kio-fuse finished. Would you be willing to get in contact and flesh out a project proposal for GSoC 2019?

In D16648#424157, @ngraham wrote:

In D16648#424130, @feverfew wrote:

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.

@ngraham I'll be having some free time this summer. I was thinking if we could solidify this task into exactly what we need to do so we can get a proposal in for GSoC?

In D16648#404125, @elvisangelaccio wrote:

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.

• Fixed ShowFolder method
• Merge remote-tracking branch 'origin' into DBusTabInstance
• Refactoring test case

In D11382#417563, @ngraham wrote:

I'll take over from here.

We have some WIP work to add a "single window mode" to Dolphin where all new window calls generate new tabs in the current window instead (D16648, D19150). What do folks think about making this an option only when that mode is being used? Or would that be too restrictive?

In D16648#415799, @ngraham wrote:

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

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

Any progress on the state of this patch?

• Added braces to one line conditions

• Make new window option clearer, remove unnecessary string and whitespace

• Removed opening new tabs as an option.

In D16648#353559, @anthonyfieroni wrote:

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

• Make new window option clearer, remove unnecessary string and whitespace

• Removed unnecessary string

@ngraham a.saoutkin@gmail.com Thanks!

@ngraham Just checked on Firefox and if you right click the last tab it allows you to choose "Close Tab" even though that will also close the window. Hence, seeming as most people aren't bothered by this behaviour in Firefox, I think we can probably just land this as is?

In D15278#340898, @elvisangelaccio wrote:
I still think this should be addressed. Maybe we can change the text of the action from "Close Tab" to "Close Window" if there is only one tab ?

I think including the patch as is would only annoy the people who hold the close tab shortcut in attempt to close all tabs bar one, whoever that may be. We could also always show the tab bar, to make it more obvious that Dolphin uses tabs (as a Windows user I always used multiple windows). I think the best option would be to add closing Dolphin via closing the last tab as a setting. However, I originally tried to do this, the problem is that when I tried to add it as a setting it would only apply when the last tab was changed (DolphinMainWidget::tabCountChanged) and the change would never show on the menubar until Dolphin was restarted. The best solution for this patch is a setting that actually works, otherwise, if the features is not deemed too unobtrusive, this should go through, even if I may be a bit biased (seeming as it is my first patch for KDE) :)

Removed line due to it being redundant