Fix WindowsStartupLocation throwing exception #258
Conversation
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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: 2
🧹 Nitpick comments (2)
src/Consolonia.Core/Infrastructure/ConsoloniaScreen.cs (1)
21-21: Consider implementing the Changed event handler.The
Changedevent is not utilized. Consider implementing screen size change notifications when the console is resized.- public Action Changed { get; set; } + private Action _changed; + public Action Changed + { + get => _changed; + set + { + _changed = value; + _console.Resized += () => + { + UpdateScreenSize(); + _changed?.Invoke(); + }; + } + }src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (1)
219-219: Consider implementing window resize and move operations.The TODO comments suggest potential future improvements. Consider implementing these operations using platform-specific APIs where available:
- Windows: Use
SetConsoleWindowInfoandMoveWindowfrom kernel32.dll- Unix: Various terminal escape sequences might be available
Also applies to: 225-225
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(2 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaScreen.cs(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/Consolonia.Core/Infrastructure/ConsoloniaScreen.cs (1)
25-31: LGTM: Screen lookup methods are implemented correctly.The implementation consistently returns the single available screen, which is appropriate for a console environment.
src/Consolonia.Gallery/View/ControlsListView.axaml (1)
7-7: LGTM: Window centering is now properly supported.The WindowStartupLocation attribute is now supported by the new ConsoloniaScreen implementation.
Co-authored-by: <[email protected]>
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/Consolonia.Core/Infrastructure/ConsoloniaScreen.cs (4)
18-18: Derive ScreenCount from _screens array.Instead of hardcoding the screen count, derive it from the array length for better maintainability.
-public int ScreenCount => 1; +public int ScreenCount => _screens.Length;
29-47: Add XML documentation explaining the single-screen design.The implementation always returns a single screen regardless of the input parameters. This behavior should be documented to explain the design decision.
+/// <summary> +/// Implements a single-screen environment for console applications. +/// All screen-related queries return the same screen instance as console +/// applications operate in a single-screen context. +/// </summary> public class ConsoloniaScreen : IScreenImpl
29-47: Add bounds checking for screen array access.Although we currently have a single screen, it's good practice to add bounds checking to prevent potential issues if the implementation changes.
public Screen ScreenFromPoint(PixelPoint point) { + if (_screens.Length == 0) + throw new InvalidOperationException("No screens available."); return _screens[0]; }
9-48: Consider future extensibility for platform-specific implementations.The current implementation provides a basic screen implementation that should resolve the initialization exception. However, given the PR objectives mentioning potential platform-specific handling (e.g., Windows), consider:
- Making the class abstract or adding virtual members to allow platform-specific overrides
- Adding hooks for console resize events to update screen dimensions
- Implementing platform detection to enable enhanced functionality on supported platforms
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(2 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaScreen.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Core/Infrastructure/ConsoleWindow.cs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
src/Consolonia.Core/Infrastructure/ConsoloniaScreen.cs (2)
15-15: Review the scaling factor of 0.0.The screen is initialized with a scaling factor of 0.0, which might affect UI rendering. Consider using a more appropriate value (typically 1.0 for standard DPI).
13-16: Consider using actual console dimensions.The screen dimensions should adapt to the actual console size rather than using fixed dimensions.
| @@ -4,6 +4,7 @@ | |||
| xmlns:d="http://schemas.microsoft.com/expression/blend/2008" | |||
| xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" | |||
| mc:Ignorable="d" | |||
| WindowStartupLocation="CenterScreen" | |||
There was a problem hiding this comment.
So what does it do eventually, what is the effect?
There was a problem hiding this comment.
it doesn't blow up with my change.
I've been experimenting with adding .Console platform projects to existing avalonia projects. (A UI is shared between Desktop, Console, Android, etc)
WindowStartupLocation is a hint to the platform. On Desktop it positions the window. It's ignored by Android, and ignored by Console app (after my change). More importantly, anything which is expecting there to be a screen definition will get one and not blow up.
There was a problem hiding this comment.
Ah, understood, then add //todo: raise ConsoloniaNotSupportedException
please.
There was a problem hiding this comment.
Where would I leave it? It's Avalonia that throws because there the environment isn't set up correctly. It expects there to be Screens defined by the platform so it can do screen based calculations.
This PR implements the missing platform Screens functionality.
The other place is that it generates a request to Resize() and Move() on the window.
The current code for Resize looks like this:
public void Resize(Size clientSize, WindowResizeReason reason = WindowResizeReason.Application)
{
// todo: can we deny resizing? TODO We could consider resizing the console window
}and I changed Move to be same
public void Move(PixelPoint point)
{
// ignore move request, TODO We could consider moving the console window
}
Bug: #253
When specificing WindowStartupLocation an exception is thrown saying "Rendering system is not intialized"
Avalonia expects there to be a IScreenImpl available so it can calculate things like window position. This delta implements a single
virtual screen for Avalonia to use to calculate.
It ignores request to resize and move the console window, although we could do something like that using platform APIs on some platforms (like windows).