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

Fix state update handling: Current page widgets do not receive update if the decorator marks the page as dirty #128

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

ulusoyca
Copy link
Collaborator

Description

This Pull Request introduces an improvement to the state management within the WoltModalSheet package. Specifically, it addresses the scenario where the state update does not reflect on the modal sheet page due to unchanged page index yet with a new widget set provided.

Issue and Solution

Previously, when the root decorator widget, typically utilized as a ChangeNotifierProvider, updated the state, the new set of widgets associated with the same page index did not update the UI as expected. This was because the internal mechanism did not adequately mark the new widget set as "dirty" for rendering.

To resolve this, the PR implements a strategy where the incoming widget set is explicitly set to null following a state change. Subsequently, it forcibly rebuilds the current page set with the updated widgets. This ensures that the new state is accurately represented on the modal sheet page, even when the page index remains constant.

Impact

This change ensures that the UI reflects the current state without requiring a page index change. This enhancement is particularly crucial for applications where modal sheet content dynamically updates based on user interaction or other state changes without navigation between pages.

Performance Considerations

While this fix ensures the correctness of the UI, it is acknowledged that setting the widget set to null and rebuilding the entire page is not the most efficient approach. There is a planned comprehensive internal refactor to improve performance and address this inefficiency.

Testing

This change has been manually tested to ensure that the widget set updates correctly without requiring a page index change. Automated tests will be developed as part of the planned refactor to ensure no regression in the future.

Before After
state_no_update.mp4
state_update_after.mp4

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

Copy link
Collaborator

@MbIXjkee MbIXjkee left a comment

Choose a reason for hiding this comment

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

LGTM

@ulusoyca ulusoyca merged commit 81ff778 into main Jan 29, 2024
2 checks passed
@ulusoyca ulusoyca deleted the fix-current-page-not-updated-when-widget-is-updated branch January 29, 2024 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants