Adding option to open externally called folder in a new tab
ClosedPublic

Authored by feverfew on Jun 10 2019, 8:13 PM.

Details

Summary

Adds an option to open externally called folder in a new tab.

By default this option is enabled

Test Plan

If option selected:

  1. All valid arguments passed to Dolphin should be opened in tabs of an instance(s) (if it exists). Duplicate tabs just change activation to current tab.

If option not selected:

  1. All calls to Dolphin result in a new instance being opened

This option does not require Dolphin to be restarted to take effect.

Diff Detail

Repository
R318 Dolphin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
feverfew created this revision.Jun 10 2019, 8:13 PM
Restricted Application added a project: Dolphin. · View Herald TranscriptJun 10 2019, 8:13 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript
feverfew requested review of this revision.Jun 10 2019, 8:13 PM

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.

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.

Have you had a chance to test D21691?

Just tried out that patch and it fixes both bugs! Let's get it in. :)

src/settings/startup/startupsettingspage.cpp
104

Can we find a different wording here? I feel like "externally called" is too technical.

Maybe just a simpler "Open new folders in a tab" ?

ngraham added inline comments.Jun 16 2019, 2:13 PM
src/settings/startup/startupsettingspage.cpp
104

I feel like we need to make it clear just what will be opened in a new tab, or else some people might think that regular navigation using their existing window will create new tabs.

Maybe "Open new tabs instead of new windows"?

feverfew updated this revision to Diff 59987.Jun 17 2019, 11:24 AM
  • Simplifying option
feverfew added inline comments.Jun 17 2019, 11:25 AM
src/settings/startup/startupsettingspage.cpp
104

Okular (which has the same feature) presents the option as "open new files in tabs", so I've opted for "open new folders in tabs" for consistency.

ngraham requested changes to this revision.Sat, Jun 22, 2:16 PM

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

This revision now requires changes to proceed.Sat, Jun 22, 2:16 PM

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

Does it still occur if you rebase this patch onto master (now that D21691 has been committed)?

Nope, same behavior after a rebase on current master.

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.

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?

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

feverfew updated this revision to Diff 60438.Sun, Jun 23, 10:35 AM
  • Fixing regression

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

Ok I have a fix up. Again, can't replicate so please test for me. Hope I haven't derailed the sprint too much :D

Not at all, this is what sprints are for :)

I will test.

ngraham accepted this revision.Sun, Jun 23, 10:44 AM

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?

This revision is now accepted and ready to land.Sun, Jun 23, 10:44 AM

Can you rebase this on current master?

feverfew updated this revision to Diff 60450.Sun, Jun 23, 11:01 AM
  • Simplifying option
  • Fixing regression
feverfew updated this revision to Diff 60453.Sun, Jun 23, 11:04 AM

Rebasing onto master

This revision was automatically updated to reflect the committed changes.

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?

I think it would be more appropriate to focus on GSoC at the moment, so until the 19th August, which i think will probably fall after 19.08.0 anyway. So I guess, time permitting, I'll aim for 19.12.0. If I go to Akademy could possibly do it there, especially as I don't know the workflow of activities/virtual desktops that well.

Sounds good!