Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: landscape magic attach #882

Merged
merged 77 commits into from
Jan 30, 2025
Merged

feat: landscape magic attach #882

merged 77 commits into from
Jan 30, 2025

Conversation

matthew-hagemann
Copy link
Collaborator

@matthew-hagemann matthew-hagemann commented Nov 29, 2024

WIP: Currently just lifting the Ubuntu Pro pages from init out into bootstrap.

@matthew-hagemann matthew-hagemann added the snap/none Don't raise any snap release PRs label Nov 29, 2024
@matthew-hagemann matthew-hagemann changed the title feat: landscape integration feat: landscape magic attach Dec 3, 2024
@matthew-hagemann matthew-hagemann force-pushed the landscape-magic-attach branch 2 times, most recently from 5c28e4f to c182707 Compare December 4, 2024 15:16
@matthew-hagemann
Copy link
Collaborator Author

@d-loose still a WIP, and haven't removed the extra pages yet, but if you are happy with how the landscape page and model are looking and how I unblocked the tests, I'll close the ticket to move over to riverpod annotations. Thanks again for all the help!

matthew-hagemann added a commit that referenced this pull request Dec 5, 2024
Based on my learnings from stubbing out landscape integratino in #882, I
have put together some `dev-docs` outlining the broad steps required for
adding a new page to the insatller, as well as adding some doc comments
on some relevant dart components that did not have any.
@d-loose
Copy link
Member

d-loose commented Jan 8, 2025

One thing I forgot to mention: we briefly talked about https://pub.dev/packages/qr_flutter the other day - would be great if you could check whether that's a more future-proof package for QR code generation than the one we're currently using here

@matthew-hagemann
Copy link
Collaborator Author

One thing I forgot to mention: we briefly talked about https://pub.dev/packages/qr_flutter the other day - would be great if you could check whether that's a more future-proof package for QR code generation than the one we're currently using here

Discussed on MM, we found the current library produced (bottom) clearer QR codes, so will not swap libraries.
image

@matthew-hagemann matthew-hagemann force-pushed the landscape-magic-attach branch 2 times, most recently from e49a590 to 1fefcd1 Compare January 17, 2025 07:27
@matthew-hagemann matthew-hagemann marked this pull request as ready for review January 24, 2025 08:31
@d-loose
Copy link
Member

d-loose commented Jan 24, 2025

Looks really good so far!
One thing I noticed during a quick test with your mock server: if I change my mind and navigate back from the landscape page that shows the QR after entering an fqdn, the listener on the stream returned by watch() is never cancelled, so if a successful response comes back after some time, we still write the autoinstall file and restart the installer. I think that's not supposed to happen, so we should make sure we only listen to the response stream while the user is on that page.

More detailed review comments soon!

Copy link
Member

@d-loose d-loose left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a bit of a refactoring proposal (since you've already spent quite a lot of time on this PR, I wouldn't mind merging this and coming back to it later on, though).

Current state management:
There's a single view model (LandscapeDataModel) for both the 'landscape domain' page where attach() is called and the 'landscape' page where watch() is called. The state this model holds includes the domain entered by the user as well as the status response from the server. It also keeps track of a stream/provider subscription and updates its own state whenever a stream event is received (after calling watch()). Listening/cancelling needs to be handled explicitly through corresponding method calls.

Proposed refactor:
We can split this into two view models. The one for the 'landscape domain' page would

  • hold the domain entered by the user
  • provide an attach() method

Instead of storing the received userCode in the view model, we could simply pass it to the route that displays the 'landscape' page.

The model for the 'landscape' page would then be a stream provider family (i.e. a stream provider that receives the userCode as an argument to its build function) that exposes a stream of UI states for the page. Simply using ref.watch(autoinstallLandscapeProvider(userCode)) would then take care of the stream's lifecycle (i.e. start listening when the user enters the page, cancel when they leave).

-> UDENG-5870

@matthew-hagemann matthew-hagemann merged commit 9e58ff2 into main Jan 30, 2025
19 checks passed
@matthew-hagemann matthew-hagemann deleted the landscape-magic-attach branch January 30, 2025 08:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
snap/none Don't raise any snap release PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants