Extend Win32 Console #241
Extend Win32 Console #241
Conversation
tomlm
commented
Jan 2, 2025
tomlm
commented
- replaced hand coded partial and incomplete PInvoke signatures with Vanara.Kernel32 signatures.
- Rewrote WindowsConsole to use Vanara signatures
- Extended and fixed console flags so all buttons are properly implemented with up/down signatures, double click, etc.
📝 WalkthroughWalkthroughThis pull request introduces updates to the Consolonia project's Windows console handling components. A new Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/Consolonia.GuiCS/WindowsDriver.cs (1)
45-62: Null return caution
Returningnullinstead of an empty array could lead toNullReferenceExceptionif callers assume a valid array. Consider returning an empty array instead to simplify downstream logic.- return numberEventsRead == 0 - ? null - : records; + return numberEventsRead == 0 + ? Array.Empty<INPUT_RECORD>() + : records;src/Consolonia.PlatformSupport/WindowsConsole.cs (3)
122-124: WindowBufferSize event block
The commented-out logic for resizing is left in place. If the resizing logic is not needed, consider safely removing it to avoid confusion.
222-222: Horizontal wheel
Currently unhandled. If horizontal scroll is needed, consider implementing a corresponding vector update.
245-245: Character extraction
Readingchar character = keyEvent.uChar;is straightforward. Consider verifying ifuCharcan be non-ASCII.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Consolonia.GuiCS/Consolonia.GuiCS.csproj(1 hunks)src/Consolonia.GuiCS/WindowsDriver.cs(1 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs(3 hunks)
🔇 Additional comments (32)
src/Consolonia.GuiCS/WindowsDriver.cs (5)
5-7: Imports alignment looks good
These imports fromVanara.PInvokeand the static usage ofKernel32are consistent with the new PInvoke approach in this PR. No issues spotted here.
11-14: Appropriate platform targeting
Applying[SupportedOSPlatform("windows")]is a good practice to indicate that this class is intended for Windows-specific functionalities.
15-17: Improved type safety
Switching fromIntPtrtoHFILEand fromuinttoCONSOLE_INPUT_MODEenhances clarity and reduces the risk of misusing handle or mode types.
18-28: Verify disabling of certain console modes
Disabling quick-edit mode and processed input, and enabling mouse input might limit typical console interactions (e.g. Ctrl+C). If this is intentional, proceed. Otherwise, you may want to selectively re-enable certain flags.
30-43: Straightforward console mode property
Capturing errors fromGetConsoleModeandSetConsoleModeby throwing exceptions is a sensible approach. Ensure higher-level logic handles these errors gracefully, especially in production environments.src/Consolonia.PlatformSupport/WindowsConsole.cs (25)
15-16: Vanara import usage
The static import ofVanara.PInvoke.Kernel32is consistent with the rest of this PR’s approach. No issues noted.
22-22: Clear platform specification
Applying[SupportedOSPlatform("windows")]toWin32Consoleclarifies that this class is not cross-platform. Well done.
25-68: Comprehensive translator mappings
The newFlagTranslatorusage for both keyboard and mouse states is clean and easy to understand. This mapping approach improves readability and maintainability compared to multiple conditional checks.
73-73: Initial mouse state
Initializing_mouseButtonsStatetoMOUSE_BUTTON_STATE.NONEensures a known default. No issues here.
93-93: Focus event injection
Writing a focus event into the console input stream is an interesting approach to trigger internal logic. This can be fragile if used too extensively, but is acceptable for specialized console scenarios.
118-118: Validate read result
The code throwsNotImplementedExceptionon an empty read, which might abruptly terminate the loop. Confirm whether empty reads require graceful handling or if they are truly unexpected.
127-128: Focus event dispatch
Raising focus events onFOCUS_EVENTaligns with your injection approach. No issues here.
131-132: Key event
InvokingHandleKeyInputforKEY_EVENTis consistent. Straightforward approach.
134-136: Mouse event
Delegating handling toHandleMouseInputis sound. The method is sufficiently encapsulated for reading or modifying mouse input logic.
143-143: Mouse input function
HandleMouseInputis the central dispatch for translating raw mouse records into Avalonia pointer events. Good structure.
145-145: Use of Avalonia's Point
Creating aPointfromdwMousePositionis straightforward. No issues here.
147-148: Combined modifiers
Combining recognized control-key states and current mouse-button states intoRawInputModifiersis a good approach for consistency.
150-150: Default event type
Defaulting toRawPointerEventType.Moveis a safe fallback.
153-153: Event flags switch
Handling differentdwEventFlagsin aswitchis clear and maintainable.
155-172: Double-click event generation
Generating two Down/Up sequences is a conventional approach to simulate double-clicks. Confirm that consumers do not misinterpret these repeated events as two single clicks.
175-209: Button state transitions
Looping over eachMOUSE_BUTTON_STATEto detect transitions is robust. Ensure the default path (lines 204–209) does not trigger spurious Move events for unchanged states.
213-216: Vertical mouse wheel
Raising theWheelevent with aVector(0, velocity)is logically correct.
218-220: Wheel event lines
This chunk is altogether part of theMOUSE_EVENT_FLAG.MOUSE_WHEELEDhandling. Looks fine.
224-225: Mouse moved event
A directMoveevent is raised for anyMOUSE_EVENT_FLAG.MOUSE_MOVED. This is standard usage.
229-230: Update state after Move
Updating_mouseButtonsStateensures the local state remains in sync.
231-235: Combination flag
This block attempts to handle a scenario where bothMOUSE_MOVEDandDOUBLE_CLICKflags appear together. Verify correctness, since it only callsLeftButtonDownandMove, omittingLeftButtonUp.
236-237: Unknown event
ThrowingInvalidOperationExceptionfor unrecognized mouse flags is a safe fallback to detect unexpected input conditions.
239-240: Finalize state
Returningtruefrom the method after updating_mouseButtonsStatecompletes the cycle. No issues.
243-243: Dedicated KeyInput handler
SeparatingHandleKeyInputclarifies the event loop logic.
247-247: Consolidated modifiers
Translating thedwControlKeyStateintoRawInputModifiersmerges seamlessly with the existing logic to detect Shift, Ctrl, and Alt.src/Consolonia.GuiCS/Consolonia.GuiCS.csproj (1)
8-10: Vanara PInvoke dependency
AddingVanara.PInvoke.Kernel32version4.0.4is aligned with the updated console logic.src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (1)
3-5: Shared dependency
Including the sameVanara.PInvoke.Kernel32reference here ensures consistent usage across both projects for improved console handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
25-32: Selective suppression of platform compatibility warnings.Disabling CA1416 around Windows-specific code is a good practice when the code path is guaranteed to run on Windows only. Just remember to confirm that calls to Windows-specific APIs cannot be reached on non-Windows platforms at runtime.
src/Consolonia.PlatformSupport/WindowsConsole.cs (4)
36-45: Mouse modifiers mapping.By mapping
MOUSE_BUTTON_STATEtoRawInputModifiers, you ensure consistent decoding of mouse button states. This is a neat approach that centralizes the logic. Watch for boundary cases, such as multi-button presses.
48-58: Distinct translators for mouse button down events.Having separate translators for down and up events helps keep the logic straightforward. Consider adding references or comments clarifying the rationale for the “LeaveWindow” default scenario, which might confuse new contributors.
73-73: Track mouse state across events.Using a private
_mouseButtonsStateeases stateful mouse event handling. For clarity, confirm this field’s usage in multi-threading scenarios, as race conditions can occur if it’s accessed from different threads.
143-239: Enhanced mouse event handling logic.The comprehensive switch handling for different mouse events (e.g., double-click, press, release, wheel, etc.) appears robust. A few observations:
• Double-click logic triggers two down-up cycles—test thoroughly for race conditions or unintended repeated clicks.
• Large switch-statement can grow complex; consider extracting smaller methods if more event flags are introduced.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs(2 hunks)src/Consolonia.PlatformSupport/WindowsConsole.cs(3 hunks)
🔇 Additional comments (8)
src/Consolonia.PlatformSupport/PlatformSupportExtensions.cs (1)
11-14: Use of pragmas to suppress IDE warnings.These pragmas to disable and restore specific IDE warnings are acceptable in cases where they address known false positives. However, ensure proper justification is documented to prevent confusion for future contributors.
src/Consolonia.PlatformSupport/WindowsConsole.cs (7)
15-16: Adopting Vanara Kernel32 API.Bringing in
Vanara.PInvoke.Kernel32and theSystem.Runtime.Versioningattributes is aligned with modern Windows-specific development. This library use should increase code clarity and maintainability.
22-22: Marking class as Windows-specific.Using [SupportedOSPlatform("windows")] clarifies the operating system dependencies of this code, which is particularly helpful for cross-platform scenarios.
25-34: FlagTranslator usage for key modifiers.Defining a static
FlagTranslatorfor mappingCONTROL_KEY_STATEtoRawInputModifiersis a clean way to manage bit-flag mappings, limiting duplication elsewhere in the code. Ensure tests verify all relevant key modifier scenarios (e.g., SHIFT + CTRL).
59-68: Ensuring symmetrical mapping for button up events.This symmetrical translator for button-up events is consistent with the button-down translator. Validate that XButton1 and XButton2 interactions are tested if the target environment utilizes them.
93-93: Triggering synthetic focus events.Writing a FOCUS_EVENT record to the console input stream is a creative way to simulate focus changes. This can help unify event flow across the codebase.
118-136: Additional event routes in loop.Handling
WINDOW_BUFFER_SIZE_EVENT,FOCUS_EVENT,KEY_EVENT, andMOUSE_EVENTdemonstrates improved coverage of console input scenarios. Double check if ignoring all other event types is intentional or if fallback handling is needed.
242-249: Refined key input interpretation.Forwarding key events with
KeyModifiersTranslatorand character data is a good practice. Ensure that special characters (e.g., function keys) and combined keys (CTRL + ALT + SHIFT) are processed consistently.
