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

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

Details

Summary

All modern web browsers offer a function to show tabs from last time when a browser starts, and many apps today
restore their prior state when they're launched. This patch implements thatfunctionality as an option and turns it on by default.

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

FEATURE: 413564
FIXED-IN: 20.04.0

Depends on D25106
Depends on D25219

Test Plan

With the new setting turned off:

  • No behavioral changes at all

With the new setting turned on:

  • When launched from the GUI or CLI without any URLs, dolphin restores session
  • When rebooting with Dolphin open, it restores session normally after the system comes back (i.e. no behavioral change here)
  • When launched with URLs, Dolphin window is opened showing those URLs instead of restoring session
  • When Dolphin is already running and a new window is opened, that new window shows a single tab with the same URL as was visible in the previously-open Dolphin instance (i.e. no behavioral change here)
  • "Open Containing folder" functionality in other apps works regardless of whether or not Dolphin is running

Diff Detail

Repository
R318 Dolphin
Branch
arcpatch-D11382
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 18270
Build 18288: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
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)Aug 17 2019, 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
207

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
207

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.Aug 19 2019, 10:46 AM
src/dolphinmainwindow.cpp
207

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.Sep 3 2019, 12:39 AM

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

ngraham edited the summary of this revision. (Show Details)Oct 28 2019, 7:46 PM
ngraham updated this revision to Diff 68906.Oct 28 2019, 7:55 PM
ngraham marked an inline comment as done.

Remove FIXME now that it's fixed

ngraham updated this revision to Diff 68913.Oct 28 2019, 8:35 PM

Delete any saved state when the feature is turned off

ngraham updated this revision to Diff 68914.Oct 28 2019, 8:40 PM

Remove last TODO

ngraham edited the summary of this revision. (Show Details)Oct 28 2019, 8:44 PM
ngraham edited the test plan for this revision. (Show Details)

This is now ready for review again.

ngraham edited reviewers, added: feverfew, meven; removed: rominf.Oct 28 2019, 8:46 PM
ngraham added a reviewer: elvisangelaccio.
ndavis added a subscriber: ndavis.Oct 28 2019, 9:51 PM

I think the hierarchy of controls is not quite right. The radio buttons should both be next to each other. Otherwise, the connection to the label at the top is not so clear when you have all those other widgets in between.

ndavis added a comment.EditedOct 28 2019, 9:54 PM

I see no reason why the filter bar and editable location bar options should be grouped under the first radio button

ngraham updated this revision to Diff 68927.Oct 28 2019, 10:11 PM

Improve settings window UI and don't disable filter bar and location bar editability controls when using the new feature

ngraham edited the summary of this revision. (Show Details)Oct 28 2019, 10:12 PM

Are split views also restored when tabs are restored?

Are split views also restored when tabs are restored?

Yeah, that's why it's in that group now.

ndavis accepted this revision.Oct 29 2019, 12:33 AM

Design looks good to me

This revision is now accepted and ready to land.Oct 29 2019, 12:33 AM
ndavis added a comment.EditedOct 29 2019, 12:36 AM

Actually, the left margin of the line edit for the startup location looks odd next to the margin of the text for the top radio button. Is that something you have any control over? It would be nice if they were aligned.

Alignment with widgets in layouts is a pain in the butt if the defaults don't work for you. I'll see what I can do about it.

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

So should that discussion happen later or can it happen here? I personally think it should be enabled by default. I think it's more common that people will want to come back to the set of folders they were looking at previously than they will want to start fresh. I think it's especially true for anyone who is trying to get work done.

I agree. I guess we can have that discussion here. :)

ndavis added a comment.EditedOct 29 2019, 12:55 AM

What happens when a new window is opened when another window with restored tabs is also opened? Are they both going to have all of the restored tabs or is it like web browsers where the new window just has the home page? If the latter is true, it may actually make more sense to allow a home page to be set and have a checkbox for remembering tabs. Firefox does the same thing. This could also make it so you don't have to fix the margins of the line edit.

What happens when a new window is opened when another window with restored tabs is also opened? Are they both going to have all of the restored tabs or is it like web browsers where the new window just has the home page? If the latter is true, it may actually make more sense to allow a home page to be set and have a checkbox for remembering tabs. Firefox does the same thing. This could also make it so you don't have to fix the margins of the line edit.

That case is explicitly handled and the behavior does not change from the status quo: a new window is opened showing the contents of the currently-viewed window/tab/split pane.

I agree that it would be nice to offer options for what happens when an additional window is opened, but that can be done afterwards as it is not strictly speaking relevant to this patch because the behavior doesn't change at all.

It's a shame that we're re-using the session restore internals, but aren't calling KMainWindow::restore to do it all properly with just the one code path. I feel like it should be do-able with just a tiny change.

src/dolphinmainwindow.cpp
203

you need to check you're not being loaded as part of a session restore otherwise you'll have two paths call readProperties

qApp->isSessionRestored()

ngraham updated this revision to Diff 68937.Oct 29 2019, 1:59 AM
ngraham marked an inline comment as done.
  • Turn on the feature by default
  • Also check for session restoration
  • Make the split view setting UI independent of this new feature since it still works fine for the case of opening new windows
  • Haven't figured out how to fix the alignment issue yet, sorry

It's a shame that we're re-using the session restore internals, but aren't calling KMainWindow::restore to do it all properly with just the one code path. I feel like it should be do-able with just a tiny change.

Ah, I didn't know about that. I'll look into it!

ngraham edited the summary of this revision. (Show Details)Oct 29 2019, 2:01 AM
ndavis accepted this revision.Oct 29 2019, 2:24 AM

If you want to deal with the alignment later, I'm OK with that. This feature is a major improvement.

@davidedmundson I could use a hand with this if you have some time. It looks like Dolphin's existing session restoration feature in main.cpp--which I think I need to conditionally activate here--isn't working properly. KXmlGuiWindow::classNameOfToplevel(1) returns an empty string, and trying kRestoreMainWindows<DolphinMainWindow>(); (per the API docs here: https://api.kde.org/frameworks/kxmlgui/html/classKMainWindow.html#a46e01bd1aa6d488f1be2a5010030efb2) does nothing.

Okay, I've done some delving here and discovered the following:

  • Dolphin's existing session restoration code works fine when e.g. restarting the machine while Dolphin is open
  • Trying to hook into those mechanics when quitting and launching manually rather than formally restoring a session as in the above case does not work because of the following issues:
    • KMainWindow::classNameOfToplevel called in main.cpp doesn't return the class name of the main window because when it checks for an existing session config, it doesn't find it
    • KMainWindow::restore() called in main.cpp will not work for this use case because it explicitly checks for qApp->isSessionRestored(), which as far as I can tell will always be false here (maybe worth changing that in KXMLGUI?)
    • Calling KMainWindow::restore() manually with the session restoration bits commented out to bypass the above issues manages to run, but when it gets to KMainWindow::show();... nothing happens.

I'll keep digging, but this stuff is pretty challenging for me so if anyone following along is familiar with how this stuff works, assistance would be greatly appreciated.

So the main problem I want to fix is the session restore path ::savePropertiesInternal saves some other stuff (size, toolbars, menus) before calling saveProperties.

I also hoped that we could just do one KConfigGui::setSessionConfig() and re-use a load more code. But in hindsight, we can't re-use that much as the teardown order is different.
We want to save the window state when we quit, but from an app POV we only know we're going to quit after we've closed the window..so we end up putting code in the window anyway.

Dolphin: https://phabricator.kde.org/P487
KXmlGui: https://phabricator.kde.org/P488

I did a version ^. It fixes the first point, there's now only one restore path, but there's still two saving paths. So I'm less convinced it's much better.

ngraham updated this revision to Diff 69156.Nov 1 2019, 3:20 PM

Make everything work. Thanks so much, David!

ngraham edited the summary of this revision. (Show Details)Nov 1 2019, 3:28 PM

I'm still not 100% sure about the changes I implemented to be able to check for a running non-daemonized Dolphin instance. If there's a better way to do this, I'm all ears.

ngraham planned changes to this revision.Nov 1 2019, 3:40 PM

Opening Dolphin with CLI URLs is broken; fixing...

ngraham updated this revision to Diff 69158.Nov 1 2019, 4:06 PM

Fix opening Dolphin with URLs on the CLI

This revision is now accepted and ready to land.Nov 1 2019, 4:06 PM
ngraham edited the test plan for this revision. (Show Details)Nov 1 2019, 4:10 PM

Friendly ping. :)

19.12 has branched so we can target 20.04 for this and have time for lots and lots of testing. Can I request a review, @elvisangelaccio?

ngraham edited the summary of this revision. (Show Details)Nov 11 2019, 12:39 AM

19.12 has branched so we can target 20.04 for this and have time for lots and lots of testing. Can I request a review, @elvisangelaccio?

Sure, on my list ;)

Friendly ping!

Would you mind splitting the re-design of the Startup settings page to a different commit?

I dont think that's wise. It's an integral part of this change, since users have to know which settings are logically connected to this new mode and which ones are not.