Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Upgrade Avalonia to 11.2.1 #159
Upgrade Avalonia to 11.2.1 #159
Changes from 10 commits
baf62c1
bce6309
67e26ed
edeb40c
bb3f236
6b388ec
24b6a82
0c923c5
bc15e5c
d3f9ce7
3da048e
5fd8667
32628a3
3f0da82
4ffc68c
488d0c3
a371dfb
3221292
9973ac9
80ef015
e265f4a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check notice on line 106 in src/Consolonia.Core/Controls/MessageBox.axaml.cs
GitHub Actions / build
Check warning on line 47 in src/Consolonia.Core/Drawing/DrawingContextImpl.cs
GitHub Actions / build
Check notice on line 18 in src/Consolonia.Core/Infrastructure/ArrowsAndKeyboardNavigationHandler.cs
GitHub Actions / build
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.
🛠️ Refactor suggestion
Consider maintaining dependency injection pattern
The constructor change removes dependency injection capability by directly instantiating
KeyboardNavigationHandler
. This creates tight coupling and makes the class harder to test. Consider keeping the previous pattern:This maintains flexibility while providing a default implementation if needed.
📝 Committable suggestion
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.
Could you please explain why is it null? May be in new version we need to register the handler somewhere? I mean may be in older version it was registered by something else which is not in the game anymore?
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.
The old Consololnia code base used to instantiate a KeyboardHandler in the ConsoloniaApplication and then pass it into the ArrowsAndKeyboardHandler instance as a singleton. Thus both the keyboard handler and the ArrowsAndKeyboard handler were singletons.
The new Avalonia code base has KeyboardHandler as a transient in the AvaloniaLocator, which means it is created on demand. According to documentation and discussions AvaloniaLocator is an internal resource resolver and not meant for general purpose dependency resolution. We are attempting to replace the one that is registered with the one that we have which wraps it. Soo...I looked at the registration of the dependency in the Avalonia side of things and it's just newing up a new instance, making it transient. I was not able to create a new registration in AvaloniaLocator for the outer navigator without creating a recursive dependency, so I elected to just instantiate it in the class.
If you have a cleaner way of doing that I'm open to it.