Skip to content

Reorder popup/view model instantiation#3071

Merged
TheCodeTraveler merged 2 commits intomainfrom
feature/sl-fix-popup-instantiation
Feb 7, 2026
Merged

Reorder popup/view model instantiation#3071
TheCodeTraveler merged 2 commits intomainfrom
feature/sl-fix-popup-instantiation

Conversation

@bijington
Copy link
Copy Markdown
Contributor

@bijington bijington commented Feb 6, 2026

Description of Change

This PR changes the order of instantiation to avoid unnecessary instantiations of the popups view model.

Linked Issues

PR Checklist

Additional information

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts how PopupService resolves popup content so that when a popup is shown via its registered ViewModel type, the ViewModel is not instantiated redundantly (fixing the double-creation behavior reported in #3065).

Changes:

  • Updated ShowPopup*/ShowPopupAsync* overloads to resolve popup content via GetPopupContent<T>() instead of resolving T up-front.
  • Refactored GetPopupContent to resolve the mapped popup View first (when available) and only resolve T directly when needed.

@bijington bijington enabled auto-merge (squash) February 6, 2026 18:02
@bijington bijington disabled auto-merge February 6, 2026 18:03
pictos
pictos previously approved these changes Feb 6, 2026
Copy link
Copy Markdown
Contributor Author

@bijington bijington left a comment

Choose a reason for hiding this comment

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

@Copilot please address the comments

Copy link
Copy Markdown
Collaborator

@TheCodeTraveler TheCodeTraveler left a comment

Choose a reason for hiding this comment

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

Thanks Shaun!

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

var popupService = ServiceProvider.GetRequiredService<IPopupService>();

// Act
await popupService.ShowPopupAsync<SingleConstructionViewModel>(page.Navigation, cancellationToken: TestContext.Current.CancellationToken);
Copy link

Copilot AI Feb 7, 2026

Choose a reason for hiding this comment

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

This call uses a named argument cancellationToken, but the ShowPopupAsync<T>(INavigation, ...) overload’s parameter is named token. As written this won’t compile; use the correct parameter name or pass the token positionally.

Suggested change
await popupService.ShowPopupAsync<SingleConstructionViewModel>(page.Navigation, cancellationToken: TestContext.Current.CancellationToken);
await popupService.ShowPopupAsync<SingleConstructionViewModel>(page.Navigation, token: TestContext.Current.CancellationToken);

Copilot uses AI. Check for mistakes.
@TheCodeTraveler TheCodeTraveler merged commit c173fe5 into main Feb 7, 2026
15 of 16 checks passed
@TheCodeTraveler TheCodeTraveler deleted the feature/sl-fix-popup-instantiation branch February 7, 2026 18:47
@github-actions github-actions bot locked and limited conversation to collaborators Feb 9, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] PopupV2 PopupService.ShowPopupAsync<T> view model assigned to the popup is created twice.

5 participants