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

C++ Template App Follow-Ups #206

Merged
merged 8 commits into from
May 14, 2024
Merged

Conversation

dacharyc
Copy link
Collaborator

@dacharyc dacharyc commented May 9, 2024

  • Handle auth errors
  • Adjust button and window constraints so the button labels are less likely to disappear
  • Use a delegate to have the HomeController change its own state, instead of letting DatabaseManager mutate its state, per this PR review comment: (DOCSP-36597): C++ Template App, MVC style #197 (comment)

Note for reviewer: after making this change, IntelliSense is saying the DatabaseManager constructor is never used, and is warning me that the _delegate I added to the DatabaseManager may be null. I'm not getting those warnings in the other places that have delegates (AuthManager/AppController, for example). So I think I may have missed something here - BUT it is all working properly.

@dacharyc dacharyc requested a review from cbush May 13, 2024 13:49
Copy link
Collaborator

@cbush cbush left a comment

Choose a reason for hiding this comment

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

Try from clean build. Not sure what the IntelliSense error is. The default (no args) constructor of DatabaseManager should be deleted by the fact that the other constructor exists, so there should be no way that constructor is unused.

sync-todo/v2/client/cpp/controllers/home_controller.cpp Outdated Show resolved Hide resolved
@dacharyc dacharyc merged commit 5c335c7 into mongodb-university:main May 14, 2024
1 check passed
@dacharyc dacharyc deleted the DOCSP-38139 branch May 14, 2024 15:04
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