| feverfew | |
| meven | |
| elvisangelaccio | |
| ndavis |
| Dolphin | |
| VDG |
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
With the new setting turned off:
With the new setting turned on:
| No Linters Available |
| No Unit Test Coverage |
| Buildable 25131 | |
| Build 25149: arc lint + arc unit |
| src/global.cpp | ||
|---|---|---|
| 106 | qAsConst | |
Okay, I think this is ready for re-review now. Clearly not material for 20.04, sadly.
| 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. ? | |
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–199 | I believe some comments are required here, I'm struggling to follow what's going on here | |
| 182 ↗ | (On Diff #79959) | 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? | |
| 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. | |
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. | |
| src/main.cpp | ||
|---|---|---|
| 192 | To not match daemonized instances | |
Fix layout on settings page (requires a bit of a hack, but not sure what we can do otherwise)
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. | |
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?
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.