-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
remove ControllerVisitor #3634
remove ControllerVisitor #3634
Conversation
This class served no purpose but made the code overcomplicated. DlgController already checks Controller's isMappable method to determine whether to enable the button to create DlgControllerLearning.
f01c57f
to
0a19f93
Compare
Did you check if LegacyControllerMappingVisitor/ConstLegacyControllerMappingVisitor are still required? Applying those GoF patterns with such a generic naming is fishy. Made these mistakes myself back then, that's why I mention it ;) |
Removing those will be the next step. Let's keep each step as small as possible. |
I don't understand the reasoning for introducing such roundabout code rather than using a cast in the first place. It dramatically increases coupling, makes the code extremely confusing, and offers no benefit as far as I can tell. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! LGTM
Pattern driven development ;) |
Textbook driven development? 🤷 Not appropriate for real code |
This class served no purpose but made the code overcomplicated.
DlgController already checks Controller's isMappable method to
determine whether to enable the button to create
DlgControllerLearning.
This is a step towards decoupling the legacy mapping system from the hardware I/O of the Controller classes. No functionality is changed by this PR.