feat(gui): Conditionally hide the Landscape Config page based on registry #835
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #835 +/- ##
==========================================
+ Coverage 88.25% 88.35% +0.10%
==========================================
Files 104 105 +1
Lines 6733 6758 +25
==========================================
+ Hits 5942 5971 +29
+ Misses 616 613 -3
+ Partials 175 174 -1 ☔ View full report in Codecov by Sentry. |
CarlosNihelton
force-pushed
the
hide-landscape-reg
branch
2 times, most recently
from
b6e8fc2 to
441b931
Compare
For reading keys from registry Or preloaded values for integration testing.
By default false. Affects the canConfigureLandscape getter. Simplify the sealed-hierarchy, removing unneeded parameters from derived classes constructors: - keep info about whether Landscape configuration is allowed in the base class. Adds test cases to assert that getter is false when Landscape is globally disabled no matter the config source.
The page itself doesn't change. But we need to pass a boolean flag from the app to the model. Here we use the Wizard route userdata param. More tests are added, to assert that the "Configure Landscape" button is not shown when globally disabled.
And propagate through the wizard: 1. The Landscape page is excluded from the Wizard if the feature is disabled. 2. SubscriptionStatus route userdata holds the flag, informing the respective page the situation of the Landscape feature.
Disabled by default Enabled via settings/registry Enabled on integration testing. Pass a bool flag to the SubscribeNowModel In the wizard composition, via the route userdata. No longer reading env vars. Updates tests.
That's affecting integration tests due render overflows.
CarlosNihelton
force-pushed
the
hide-landscape-reg
branch
from
441b931 to
1882839
Compare
|
There is a modest coverage on the lines reported as uncovered, because we don't report coverage of Flutter tests run on Windows (to avoid more complexity in CI installing tools to install tools on Windows to be able to parse and report coverage output). |
didrocks
approved these changes
Jul 15, 2024
CarlosNihelton
added a commit
that referenced
this pull request
Aug 6, 2024
... for developers and contributors. We don't show Landscape config page nor the subscribe button by default. Those willing to test those features by hand need to enable them explicitly via the Windows registry.
CarlosNihelton
added a commit
that referenced
this pull request
Aug 6, 2024
... for developers and contributors. We don't show Landscape config page nor the subscribe button by default. Those willing to test those features by hand need to enable them explicitly via the Windows registry.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Opening this PR on top of #834 because otherwise CI won't go green ;) )
This implements a "Settings" feature on top of the Windows registry via which we can enable Landscape configuration and MS Store purchase (previously configured via an environment variable), which are both currently disabled by default.
We make sure to not depend on the Windows registry for integration testing, i.e. everything is tested as if all features are enabled.
For smaller test cases we test with both features enabled and disabled to ensure correctness.
This PR keeps the dependency on the Settings on the highest level, only propagating the decision to the implementation of the related pages.
While this is a simpler design, it has a drawback of not reacting to changes in the underlying data source (the Windows registry).
Adding a listener for registry changes is more complex and not worth in my opinion.
UDENG-3360