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

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.08.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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
There are a very large number of changes, so older changes are hidden. Show Older Changes
meven added inline comments.Dec 30 2019, 10:37 AM
src/global.cpp
106

qAsConst

ngraham updated this revision to Diff 78019.Mar 19 2020, 3:46 PM
ngraham marked an inline comment as done.

Rebase

ngraham planned changes to this revision.Mar 19 2020, 3:54 PM

bad rebase, fixing...

ngraham updated this revision to Diff 78023.Mar 19 2020, 4:27 PM

Fix the rebase

This revision is now accepted and ready to land.Mar 19 2020, 4:27 PM
ngraham updated this revision to Diff 78024.Mar 19 2020, 4:30 PM

Reduce diff

ngraham updated this revision to Diff 78026.Mar 19 2020, 4:43 PM

Finally make it work again!

Okay, I think this is ready for re-review now. Clearly not material for 20.04, sadly.

meven added inline comments.Mar 24 2020, 11:03 AM
src/settings/startup/startupsettingspage.cpp
57

"Folders and tabs that were open before" => "Folders and tabs as they were opened before" Since we also restore split view and such. ?

ngraham updated this revision to Diff 78369.Mar 24 2020, 3:43 PM
ngraham marked an inline comment as done.

Massage a string

ngraham marked 3 inline comments as done.Mar 24 2020, 3:44 PM
feverfew requested changes to this revision.Apr 8 2020, 11:39 AM

Apologies for the delay in reviewing this. I believe FIXED-IN will unfortunately have to change to 20.08. Also looking at the screenshot, the text and the buttons/input box are misaligned, is this resolvable in this patch, or is that a wider issue with settings? Also note in the commit message "thatfunctionality" -> "that functionality".

src/global.h
57

full data is ambiguous here. What this function does is just return all GUI capable instances, and leaves the QStringList empty, (which is space to hold all the URL's an instance currently has open, but isn't filled by this function).

src/main.cpp
143

I don't understand the point of this variable, just use urls.isEmpty() when needed

181–200

I believe some comments are required here, I'm struggling to follow what's going on here

182

Why is the debug output removed? (is it genuinely useless?)

187

Why the double negative? I believe removing ! and isEmpty() will have the same effect here.

src/settings/startup/startupsettingspage.cpp
68–69

Does this compile? Shouldn't QWidget actually be QWidget()?

86–87

Same question here

161

state state?

This revision now requires changes to proceed.Apr 8 2020, 11:39 AM
ngraham edited the summary of this revision. (Show Details)Apr 9 2020, 4:49 PM
ngraham updated this revision to Diff 79722.Apr 9 2020, 5:13 PM
ngraham marked 8 inline comments as done.

Address feedback and comments

ngraham added inline comments.Apr 9 2020, 5:13 PM
src/main.cpp
143

It's because we later mutate urls, so we need to know if it was empty originally, not that it's currently empty. I'll add a comment for clarity.

ngraham marked an inline comment as done.Apr 9 2020, 5:13 PM

As for the mis-alignment in the settings window, I was not able to figure out how to improve that. Suggestions are welcome.

elvisangelaccio requested changes to this revision.Apr 12 2020, 5:47 PM

As for the mis-alignment in the settings window, I was not able to figure out how to improve that. Suggestions are welcome.

Maybe there is some padding left in the layouts that needs to be zeroed out?

src/dolphinmainwindow.cpp
213–219

Please move this logic in DolphinMainWindow::closeEvent().

src/global.cpp
81

Please use auto.

src/main.cpp
144

Please give it a descriptive name, e.g. startedWithUrls, and make it const.

187

Typo: available

192

What is this check needed for?

src/settings/startup/startupsettingspage.cpp
134–135

Please use a single line.

This revision now requires changes to proceed.Apr 12 2020, 5:47 PM
ngraham added inline comments.Apr 12 2020, 6:24 PM
src/main.cpp
192

To not match daemonized instances

ngraham updated this revision to Diff 79957.Apr 12 2020, 7:13 PM
ngraham marked 7 inline comments as done.

Address comments

ngraham updated this revision to Diff 79958.Apr 12 2020, 7:13 PM

Fix the patch base

ngraham updated this revision to Diff 79959.Apr 12 2020, 7:37 PM

Fix layout on settings page (requires a bit of a hack, but not sure what we can do otherwise)

elvisangelaccio requested changes to this revision.Apr 13 2020, 6:06 PM
elvisangelaccio added inline comments.
src/global.h
59

I'd call this function dolphinGuiInstances.

src/main.cpp
185–189

The comment and the code say two different things, no? Looking at the comment I'd think all three conditions are required, but looking at the if I see that the 2nd and 3rd conditions are not both required.

This revision now requires changes to proceed.Apr 13 2020, 6:06 PM
ngraham marked an inline comment as done.Apr 13 2020, 6:54 PM

As for the padding issue on the settings page, GammaRay says that the checkbox itself is 20x20, but its frame size is 28x20, and I'm not understanding where that extra space comes from.

src/main.cpp
185–189

app.isSessionRestored() refers to session restoration after reboot. I'll clarify the comment.

ngraham updated this revision to Diff 80045.Apr 13 2020, 6:55 PM
ngraham marked 2 inline comments as done.

Address more comments

Clean build fails for me:

In file included from ../src/settings/viewpropertiesdialog.cpp:25:
../src/global.h:23:10: fatal error: 'dolphinmainwindowinterface.h' file not found
#include "dolphinmainwindowinterface.h"

@ngraham can you have a look?

Darn, didn't notice that since I was doing incremental builds. Can reproduce and will fix.

It looks like the autogenerated header file is generated too late because it is not marked as a dependency for global.h.

My CMake skills are pretty terrible and I'm having a hard time figuring out how to set that dependency. Does anyone have any quick pointers?

ngraham updated this revision to Diff 80720.Apr 21 2020, 3:51 AM

Forward-declare rather than including

Never mind, no CMake atrocities required to fix clean builds. :)

feverfew accepted this revision.Apr 21 2020, 10:08 AM
elvisangelaccio accepted this revision.Apr 26 2020, 5:10 PM

LGTM now. Ship it! :)

This revision is now accepted and ready to land.Apr 26 2020, 5:10 PM
This revision was automatically updated to reflect the committed changes.

Can somebody take a screenshot of this new version without background to replace /doc/preferences-startup.png? I cannot compile this new version on my machine to update docs.

Thanks in advance for your help.