New welcome screen and tweaks to update checking flow #28671
Conversation
mathesoncalum
force-pushed
the
welcome_dialog
branch
from
11b7df5 to
a34fffc
Compare
|
|
||
| if (appUpdateScenario()) { | ||
| ++totalChecksExpected; | ||
| DEFER { |
There was a problem hiding this comment.
The content of the DEFER is executed when leaving the scope within which it appears. And since it is the last statement in this scope (i.e. the {} of the if), that means the content is executed immediately, so the fact that it is inside DEFER makes no difference.
There was a problem hiding this comment.
Oof - thanks for letting me know, I'd assumed it was tied to the method scope...
| @@ -48,12 +48,12 @@ class UpdateScenario : public IUpdateScenario, public Injectable, public async:: | |||
| UpdateScenario(const modularity::ContextPtr& iocCtx) | |||
| : Injectable(iocCtx) {} | |||
|
|
|||
| void checkForUpdate(bool manual) override; | |||
| void checkForUpdate(bool manual, const CheckForUpdateCompleteCallback& callback = nullptr) override; | |||
There was a problem hiding this comment.
We don't work with such callbacks very often. Instead, would it be an idea to expose a Progress, or make this a Promise?
| return; | ||
| } | ||
|
|
||
| if (!configuration()->needCheckForUpdate() || multiInstancesProvider()->instances().size() != 1) { |
There was a problem hiding this comment.
Wouldn't it be preferable to move these checks plus the timer to the caller (i.e. StartupScenario), so that checkForUpdate becomes just doCheckForUpdate and nothing else?
I also feel like we might now have two sources of delay, namely the one second from AUTO_CHECK_UPDATE_INTERVAL, and whatever delay mechanism the caller might use.
src/app/internal/guiapp.cpp
Outdated
| if (splashScreen) { | ||
| splashScreen->close(); | ||
| delete splashScreen; | ||
| const Version welcomeDialogLastShownVersion(appshellConfiguration()->welcomeDialogLastShownVersion()); |
There was a problem hiding this comment.
Can we move all this logic into StartupScenario?
Ideally, this part should only include method calls like runOnSplashScreen, runAfterSplashScreen, and so on.
There was a problem hiding this comment.
I would also recommend keeping the methods more abstract, because in your case you’re exposing the internal logic of the scenario.
|
|
||
| void WelcomeDialogModel::init() | ||
| { | ||
| // TODO: This is entirely placeholder code - figure out where to store/pull this information... |
There was a problem hiding this comment.
Let’s move this to StartupScenario and make WelcomeDialog more abstract - that way, if we decide to fetch data from the server in the future, part of the work will already be in place, and the dialog will be ready for it.
mathesoncalum
force-pushed
the
welcome_dialog
branch
from
a34fffc to
05e8af4
Compare
mathesoncalum
force-pushed
the
welcome_dialog
branch
from
05e8af4 to
60dd664
Compare
|
@Eism @cbjeukendrup thanks guys - updated now:
|
No description provided.