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

refactor of the foundational code per the updated spec #11

Merged
merged 26 commits into from
Dec 10, 2024

Conversation

mattkae
Copy link

@mattkae mattkae commented Nov 25, 2024

What's new?

A lot! I completely refactored window.dart and how the multi window reference app works.

This work:

  • decouples the createX methods that reach out to the engine from the dart widgets
  • introduces dart widgets (e.g. RegularWindow) that can be declaratively used to handle the creation and deletion of a window
  • introduce the concept of a WindowController (e.g. RegularWindowController) which enables callers to make changes to a window and listen for changes on a window
  • hides a lot of unnecessary symbols that were being exported in favor of a simpler api

Overall, this is a much needed change! It significantly changed the way that the multi window reference app is done, but for the better in my opinion.

Side note

I have a feeling that it will be a better use of our time to not merge this up in the longer run, but rather to make new PRs off of this. The API has changed so drastically that the newer concepts map very little onto existing PRs.

@hbatagelo
Copy link

This looks like a great refactor. I haven't thoroughly reviewed the code yet, but I noticed some potential issues while running the app:

  • The first row in the active window table doesn't seem to be selectable. I think this is fine for now, but it might prevent making the main window the parent of another window (dialog, popup, etc) in the future.
  • The layout of the regular window appears to be from an older version of the reference app. It doesn't display the animated wireframe cube and doesn't show the view size. See the pictures below.

This PR:
Screenshot 2024-12-06 115209

Latest ref app (of course, the "Child Popup" button would be removed for now):
Screenshot 2024-12-06 130209

@mattkae
Copy link
Author

mattkae commented Dec 9, 2024

@hbatagelo Fixed those two issues! Good catch

Copy link

@hbatagelo hbatagelo left a comment

Choose a reason for hiding this comment

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

A few additional suggestions, this time about window.dart.

One small nitpick: the text shown in the regular windows still appears to be from the older reference app (It's fine to update it in a future PR).

@mattkae mattkae requested a review from hbatagelo December 9, 2024 18:55
Copy link

@hbatagelo hbatagelo left a comment

Choose a reason for hiding this comment

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

Also:

  • Consider adding vector_math: ^2.1.4 as a dependency in pubspec.yaml.
  • Check whether CPP_WRAPPER_SOURCES_WINDOWING is needed in examples/multi_window_ref_app/windows/flutter/CMakeLists.txt. It doesn't seem to work with the 1st engine PR.

@mattkae mattkae merged commit 39de462 into foundation Dec 10, 2024
1 check passed
@mattkae mattkae deleted the foundation-refactor branch December 10, 2024 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants