Add an option to show tabs from last time when Dolphin starts
Changes PlannedPublic

Authored by ngraham on Mar 16 2018, 7:46 AM.

Details

Reviewers
rominf
Group Reviewers
Dolphin
VDG
Summary

Many web browsers (Firefox for example) offer a function to show tabs from last time when a browser starts. This patch implements this functionality as an option.

The settings window is accordingly adjusted to be clear about what applies when:


This setting is off by default, pending a discussion regarding whether it makes sense to have it on by default.

FEATURE: 215138
FIXED-IN: 19.12.0

Test Plan
  1. Go to SettingsConfigure Dolphin...Startup
  2. Check Show tabs from last time
  3. Save settings
  4. Open a few tabs (probably with the splits)
  5. Close Dolphin
  6. Observe no notification on closing multiple tabs
  7. Open Dolphin
  8. Observe tabs and splits from last time

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D11382
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 16028
Build 16046: arc lint + arc unit
rominf requested review of this revision.Mar 16 2018, 7:46 AM
rominf created this revision.
rominf edited the test plan for this revision. (Show Details)Mar 16 2018, 7:47 AM
rominf edited the test plan for this revision. (Show Details)
rominf edited the test plan for this revision. (Show Details)

How does that behave when I start Dolphin a second time? I'm a person that usually has 5 Dolphin windows open with tons of tabs inside of them.

src/dolphinmainwindow.cpp
193

Instead of hardcoding those settings everywhere, please use KConfigXt (the kcfg files you can see for other places in Dolphin), like GeneralSettings

src/settings/startup/startupsettingspage.cpp
117

This wording could be improved (can't think of one right now)

rominf added a comment.EditedMar 16 2018, 7:59 AM

How does that behave when I start Dolphin a second time? I'm a person that usually has 5 Dolphin windows open with tons of tabs inside of them.

It opens a new instance of Dolphin identical to the previous one... I guess that's a wrong behavior. Should the new instance be fresh if there are other Dolphins running? If so, how to check this? DBus?

It opens a new instance of Dolphin identical to the previous one... I guess that's a wrong behavior. Should the new instance be fresh if there are other Dolphins running?

I don't know. Perhaps restore the set only once? (This would make for an awesome crash recovery feature btw) Best add VDG team as reviewers so they can chime in

rominf added a reviewer: VDG.Mar 16 2018, 11:08 AM

@rominf, are you planning to come back and work on this at all?

Restricted Application added a project: Dolphin. · View Herald TranscriptFeb 21 2019, 11:10 PM
Restricted Application added a subscriber: kfm-devel. · View Herald Transcript

@rominf, are you planning to come back and work on this at all?

Hi everyone. I worked on KDE because I was severely ill and couldn't work at a normal job and I wanted to do something valuable. Now I'm healthy and have a full-time job and also a business, so I don't have time for contributing to Free Software. So for now, I'm not planning to contribute to KDE in the nearest future.

Thanks for letting us know. I'm glad to hear that you're healthy, wealthy, and wise! :)

ngraham commandeered this revision.Feb 22 2019, 5:03 PM
ngraham added a reviewer: rominf.

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?

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?

Yep, hope to be getting the patch merged soon. To note that D16648 is not implementing a strict "single window mode". As in whilst all external calls are opened in an existing window (if it exists), one can still have multiple windows, similar to how most browser's open externally called URIs. I suggest the way this feature should be implemented is that the last dolphin instance closed will have its tabs re-opened. It's the simplest one to code up and makes sense to the user. However, users may find it annoying if they have many tabs sprawled over many windows and want to save them all; users can't drag tabs to other windows.

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?

Yep, hope to be getting the patch merged soon. To note that D16648 is not implementing a strict "single window mode". As in whilst all external calls are opened in an existing window (if it exists), one can still have multiple windows, similar to how most browser's open externally called URIs. I suggest the way this feature should be implemented is that the last dolphin instance closed will have its tabs re-opened. It's the simplest one to code up and makes sense to the user. However, users may find it annoying if they have many tabs sprawled over many windows and want to save them all; users can't drag tabs to other windows.

Yeah, that makes a lot of sense. Moving tabs between windows is something we should support in another patch. :)

intika added a subscriber: intika.EditedFeb 27 2019, 5:08 PM

Amazing :) great work and nice feature, tested this under v17.12.3 along with https://phabricator.kde.org/D19150... it works but

  1. Enabling the feature on the settings make the other startup settings grayed/disabled, this should not happen... may be because i am using v17, i will have a deeper look later on ;)
  1. Also this disable the warning about closing window containing multiple tab, it should not disable that warning even it tabs are saved.
  1. Restored window suffer from having right click disabled (can not right click on files)
ngraham updated this revision to Diff 62090.Jul 20 2019, 1:20 AM

DONE:

  • Rebase on master
  • Make it compile
  • Make it work
  • Don't disable other settings on the startup page when using the feature
  • Don't disable the multi-tab warning dialog when using the feature
  • Restore the state of the last-closed window

TODO:

  • Make subsequent instances open in the default location
  • Alter UI on startup settings page to better support this feature
ngraham planned changes to this revision.Jul 20 2019, 1:28 AM
ngraham marked an inline comment as done.
ngraham updated this revision to Diff 63867.Aug 16 2019, 3:51 PM

Adjust Settings window presentation to make sense with this new setting

ngraham edited the summary of this revision. (Show Details)Aug 16 2019, 3:53 PM
ngraham updated this revision to Diff 63871.Aug 16 2019, 4:23 PM

Adjust startup settings UI to only have two major sections

ngraham edited the summary of this revision. (Show Details)Aug 16 2019, 4:23 PM
ngraham planned changes to this revision.Aug 16 2019, 4:35 PM
ngraham updated this revision to Diff 63882.Aug 16 2019, 8:39 PM

Refactor DBus call to find existing Dolphin instances into a re-usable function so we can use it to find out of there are any instances running when the user opens more windows

ngraham planned changes to this revision.Aug 16 2019, 9:01 PM
ngraham updated this revision to Diff 63887.Aug 16 2019, 10:04 PM

Adjust settings UI again, this time to be perfectly clear about what applies when

ngraham edited the summary of this revision. (Show Details)Aug 16 2019, 10:05 PM
ngraham marked an inline comment as done and an inline comment as not done.

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.

ngraham edited the summary of this revision. (Show Details)Sat, Aug 17, 2:42 PM

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.

Firstly if you have open new folders in tabs enabled as well you'll be twice looping through the services to see if they can handle URLs. This can probably be merged somehow. I also had a similar issue when developing this patch sometimes, I assumed maybe it was long DBus replies... from services that couldn't handle URLs (i.e. errors). Getting the list of all services is unavoidable AFAIK and shouldn't take too long to loop through them. The time here is taken up by DBus messages obviously, whether it's errors or slow DBus messge passing that is the issue, I don't know...

src/dolphinmainwindow.cpp
197

I'm surprised this works at all. AFAICTL it'll never be empty as the instance will find itself and hence won't be empty, unless our DBus interface isn't initialised yet (tbf I don't know when it would be). Either way I don't see why this instance is preferred so I think the code would be clearer if it was simply QString() instead.

Thanks for the comments, @feverfew. Your assistance is definitely appreciated given that you wrote a lot of the code I'm mangling here.

src/dolphinmainwindow.cpp
197

It doesn't find itself because Dolphin::dolphinInstanceData() filters out the currently running instance by excludinv entries that match its PID.

feverfew added inline comments.Mon, Aug 19, 10:46 AM
src/dolphinmainwindow.cpp
197

The reason why preferredService was a parameter was for when ShowFolders/ShowItems was called via DBus. It specifically bypasses excluding the running instance as in that case you wanted the running instance to accept those URLs, i.e. marking a sevice as preferred means it is exempt from the check that filters out the currently running instance. In this case you want to see if any other instances (apart from this one) exist, and hence you should be passing QString(). The only reason I can think of why the patch works in this current state is that Q_CLASSINFO macro only initialises the DBus object for the main window after the constructor finishes...

@ngraham I believe if you rebase on D23672 then the FIXME is solved. I also believe that dolphinInstanceData() should have an empty QString() in your case. I've tested your patch with an empty QString() and it worked, it might not work with what you currently have...

ngraham planned changes to this revision.Tue, Sep 3, 12:39 AM

Thanks @feverfew, your latest changes solved the speed issue. :)