Live preview while editing AXAML files #150
Conversation
* Added material and fluent themes * made ConsolePreview correctly find files
cleanup background look and feel
WalkthroughThe pull request introduces significant changes across multiple files in the Consolonia project, including the addition of new classes, modifications to existing methods, and updates to project configurations. Key changes include the introduction of JSON serialization for various classes, updates to the handling of text decorations, and enhancements to the drawing context. Additionally, new project files for the Changes
Possibly related PRs
Suggested reviewers
Poem
Warning Rate limit exceeded@tomlm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 (
|
I need to actually treat Symbol Empty and Symbol space different, as Symbol space is for drawing background, and Symbol.Empty represents pixel that is invisible because of prior double-wide char overlapping with it.
remove test code I didn't want to check in.
…nto tomlm/preview
Co-authored-by: <[email protected]>
![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
🧹 Outside diff range and nitpick comments (1)
src/Consolonia.Designer/ConsolePreview.cs (1)
271-304: Consider optimizing pixel buffer rendering for performanceThe
RenderPixelBuffermethod creates aStackPanelwith nestedTextBlockelements for each line and character, which may have performance implications for large buffers due to the UI elements' overhead.Consider using a more efficient approach, such as rendering the entire text content in a single
TextBlockor leveraging aDrawingContextto draw text directly. This can improve rendering performance and reduce memory usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Designer/ConsolePreview.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Designer/ConsolePreview.cs (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/ConsolePreview.cs:161-173
Timestamp: 2024-11-29T19:09:22.038Z
Learning: In C#, the `File.ReadAllText` method throws an `IOException`, not a `FileNotFoundException`. Therefore, catching `IOException` is sufficient to handle scenarios where a file is not found or cannot be accessed.
🔇 Additional comments (6)
src/Consolonia.Designer/ConsolePreview.cs (6)
331-345: Guard against division by zero when computing character dimensions
In the ComputeCharWidth method, if _charWidth and _charHeight remain zero after attempting to measure text, default values are assigned. However, if the measurement fails and the defaults are also zero (though currently they are set to 10 and 20), this could lead to division by zero elsewhere in the code.
Ensure that _charWidth and _charHeight are never zero after this method to prevent potential runtime errors.
Verify that the default values ensure non-zero dimensions:
if (_charWidth == 0)
- _charWidth = 10;
+ _charWidth = 10; // Ensure this is a non-zero default
if (_charHeight == 0)
- _charHeight = 20;
+ _charHeight = 20; // Ensure this is a non-zero default48-57:
Avoid wrapping essential functionality within #if DEBUG directives
The initialization code, including the assignment of _process, font settings, and event subscriptions to Initialized and PropertyChanged, is enclosed within #if DEBUG directives. This means that in Release builds, critical functionality will be omitted, potentially leading to a non-functional control outside of Debug configurations.
Consider removing the #if DEBUG directives to ensure that the control works correctly in all build configurations.
Apply this diff to adjust the code:
-#if DEBUG
_process = null;
FontFamily = FontFamily.Parse("Cascadia Mono");
Initialized += (_, _) => LoadXaml();
PropertyChanged += (_, e) =>
{
if (e.Property == FileNameProperty) LoadXaml();
};
-#endif98-119:
Ensure process resources are properly released in all build configurations
In the Dispose(bool disposing) method, the code that disposes of _process is wrapped inside #if DEBUG directives. This means that in Release builds, any process started by ConsolePreview may not be properly terminated and disposed of, potentially leading to resource leaks.
Consider removing the #if DEBUG directives around the disposal code to ensure _process is properly disposed in all build configurations.
Apply this diff to adjust the code:
protected void Dispose(bool disposing)
{
if (!_disposedValue)
{
if (disposing)
{
// Dispose managed state (managed objects)
-#if DEBUG
#pragma warning disable CA1416 // Validate platform compatibility
if (_process != null)
{
_process.Kill();
_process.Dispose();
_process = null;
}
#pragma warning restore CA1416 // Validate platform compatibility
-#endif
}
_disposedValue = true;
}
}347-366:
Add validation to prevent exceptions when parsing design width and height
In the GetDesignWidthAndHeight method, if the expected patterns are not found, IndexOf may return -1, leading to invalid indices and potential exceptions when using Substring. It's important to validate iStart and iEnd before attempting to extract substrings.
Apply this diff to add validation:
int iStart = xaml.IndexOf("d:DesignWidth=\"", StringComparison.Ordinal);
if (iStart > -1)
{
iStart += 15;
int iEnd = xaml.IndexOf("\"", iStart, StringComparison.Ordinal);
+ if (iEnd > iStart)
+ {
string num = xaml.Substring(iStart, iEnd - iStart);
_ = ushort.TryParse(num, out designWidth);
+ }
}
iStart = xaml.IndexOf("d:DesignHeight=\"", StringComparison.Ordinal);
+if (iStart > -1)
{
iStart += 16;
int iEnd = xaml.IndexOf("\"", iStart, StringComparison.Ordinal);
+ if (iEnd > iStart)
+ {
string num = xaml.Substring(iStart, iEnd - iStart);
_ = ushort.TryParse(num, out designHeight);
+ }
}228-230:
Add null-check for _process before writing to StandardInput
When sending a request to the preview host to load the new XAML file, ensure that _process is not null and has not exited to prevent a potential NullReferenceException or writing to a closed stream.
Apply this diff to add a null-check:
// Send request to previewHost to load the new xaml file.
+if (_process != null && !_process.HasExited)
+{
_process.StandardInput.WriteLine(xamlPath);
+}
+else
+{
+ Debug.WriteLine("Process is not running. Cannot send XAML path.");
+}Likely invalid or redundant comment.
122-126:
Implement the IDisposable interface explicitly
The ConsolePreview class contains Dispose methods but does not implement the IDisposable interface. This may lead to unexpected behavior when disposing of the object, as the dispose pattern relies on the interface to signal that resources should be released.
Implement the IDisposable interface to follow .NET best practices for resource management.
Apply this diff to implement IDisposable:
-public class ConsolePreview : UserControl
+public class ConsolePreview : UserControl, IDisposable
{
// Class implementation
}
public void Dispose()
{
// Dispose of managed resources.
Dispose(true);
+ GC.SuppressFinalize(this);
}Likely invalid or redundant comment.
|
Lets disable CA1303? @tomlm |
# Conflicts: # src/Consolonia.Core/Consolonia.Core.csproj # src/Consolonia.Core/Infrastructure/ConsoleWindow.cs # src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml # src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (1)
79-84: Add parameter validation for text shapingWhile the implementation is correct, it would benefit from parameter validation to ensure robust error handling.
Consider adding these validations:
+if (string.IsNullOrEmpty(value)) +{ + _shapedText = null; + return; +} + ShapedBuffer glyphs = textShaper.ShapeText(value.AsMemory(), new TextShaperOptions(typeface, typeface.Metrics.DesignEmHeight)); +if (glyphs.Length == 0) +{ + throw new InvalidOperationException("Text shaping produced no glyphs."); +}src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (2)
21-21: Consider adding null checks and documentation for the public APIThe class is now public and introduces a new
PixelBufferproperty. Since this is part of the public API:
- Consider adding XML documentation to describe the class's purpose and the property's usage
- Add null checks for the
PixelBufferinitialization to prevent potential null reference exceptions+ /// <summary> + /// Implements window functionality for console-based applications. + /// </summary> public class ConsoleWindow : IWindowImpl { public ConsoleWindow() { // ... existing code ... - PixelBuffer = new PixelBuffer(Console.Size); + PixelBuffer = new PixelBuffer(Console.Size ?? throw new InvalidOperationException("Console size cannot be null")); } + /// <summary> + /// Gets or sets the pixel buffer used for console window rendering. + /// </summary> public PixelBuffer PixelBuffer { get; set; }Also applies to: 40-40, 43-43
Line range hint
243-254: Consider using pattern matching for feature type checksThe current implementation could be more maintainable using pattern matching and a dictionary of known types.
public object TryGetFeature(Type featureType) { - if (featureType == typeof(ISystemNavigationManagerImpl)) - return null; - if (featureType == typeof(ITextInputMethodImpl)) return null; - if (featureType == typeof(IStorageProvider)) - return AvaloniaLocator.Current.GetService<IStorageProvider>(); - throw new NotImplementedException("Consider this"); + return featureType switch + { + Type t when t == typeof(ISystemNavigationManagerImpl) => null, + Type t when t == typeof(ITextInputMethodImpl) => null, + Type t when t == typeof(IStorageProvider) => AvaloniaLocator.Current.GetService<IStorageProvider>(), + _ => throw new NotImplementedException($"Feature type {featureType.Name} is not supported") + }; }src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (1)
Line range hint
107-131: Optimize string concatenation performanceThe current implementation creates multiple string concatenations through StringBuilder. Consider pre-calculating the capacity and chaining the append calls.
- var sb = new StringBuilder(); + var sb = new StringBuilder(str.Length + 50); // Estimate for escape sequences if (textDecoration == TextDecorationLocation.Underline) - sb.Append(Esc.Underline); + sb.Append(Esc.Underline) + .Append(Esc.Background(background)) + .Append(Esc.Foreground(weight switch + { + FontWeight.Medium or FontWeight.SemiBold or FontWeight.Bold or FontWeight.ExtraBold or FontWeight.Black + or FontWeight.ExtraBlack + => foreground.Brighten(background), + FontWeight.Thin or FontWeight.ExtraLight or FontWeight.Light + => foreground.Shade(background), + _ => foreground + })) + .Append(str) + .Append(Esc.Reset);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs(8 hunks)src/Consolonia.Gallery/App.cs(3 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml.cs(1 hunks)src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs(3 hunks)src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj(1 hunks)src/Tools/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj(1 hunks)src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml
- src/Tests/Consolonia.Core.Tests/Consolonia.Core.Tests.csproj
- src/Consolonia.Gallery/Gallery/GalleryViews/GalleryWelcome.axaml.cs
- src/Consolonia.Gallery/App.cs
- src/Consolonia.Core/Consolonia.Core.csproj
- src/Consolonia.Gallery/View/ControlsListView.axaml.cs
- src/Tools/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj
🔇 Additional comments (8)
src/Consolonia.Themes.TurboVision/Templates/Controls/Helpers/SymbolsControl.cs (4)
103-103: LGTM: Pattern matching improves readability
The use of pattern matching with is not { Length: > 0 } makes the code more expressive and maintainable.
72-74:
Add null checks for Avalonia services
Services obtained from AvaloniaLocator.Current.GetService<T>() might return null if not registered. This could lead to runtime exceptions.
Apply this diff to add null checks:
var platformRender = AvaloniaLocator.Current.GetService<IPlatformRenderInterface>();
+ if (platformRender == null)
+ throw new InvalidOperationException("IPlatformRenderInterface service is not available.");
var textShaper = AvaloniaLocator.Current.GetService<ITextShaperImpl>();
+ if (textShaper == null)
+ throw new InvalidOperationException("ITextShaperImpl service is not available.");
var fontManager = AvaloniaLocator.Current.GetService<IFontManagerImpl>();
+ if (fontManager == null)
+ throw new InvalidOperationException("IFontManagerImpl service is not available.");75-77:
Improve GlyphTypeface creation error handling
The TryCreateGlyphTypeface method's boolean result is ignored, and the error message doesn't clearly indicate the actual issue when the font creation fails.
Apply this diff to improve error handling:
-fontManager.TryCreateGlyphTypeface("Cascadia Mono", FontStyle.Normal, FontWeight.Normal,
- FontStretch.Normal, out IGlyphTypeface typeface);
-ArgumentNullException.ThrowIfNull(typeface);
+bool success = fontManager.TryCreateGlyphTypeface("Cascadia Mono", FontStyle.Normal, FontWeight.Normal,
+ FontStretch.Normal, out IGlyphTypeface typeface);
+if (!success || typeface == null)
+{
+ throw new InvalidOperationException("Failed to create GlyphTypeface for 'Cascadia Mono'. Ensure the font is installed.");
+}105-112: 🛠️ Refactor suggestion
Optimize text rendering performance
The current implementation creates a new FormattedText instance for each line, which could be inefficient for large heights.
Consider caching the FormattedText:
-for (int y = 0; y < Bounds.Height; y++)
- context.DrawText(formattedText, new Point(0, y));
+var formattedTextCache = formattedText;
+for (int y = 0; y < Bounds.Height; y++)
+ context.DrawText(formattedTextCache, new Point(0, y));Also, verify the MeasureText extension method:
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (1)
390-409:
Complete the implementation of resource disposal
The Dispose pattern implementation still contains TODO comments and might miss disposing of some managed resources. Consider:
- Disposing of
_resizeCancellationTokenSource - Disposing of
PixelBufferif it implements IDisposable - Removing the TODO comments once implemented
protected virtual void Dispose(bool disposing)
{
if (!_disposedValue)
{
if (disposing)
{
- // TODO: dispose managed state (managed objects)
Closed?.Invoke();
Console.Resized -= OnConsoleOnResized;
Console.KeyEvent -= ConsoleOnKeyEvent;
Console.MouseEvent -= ConsoleOnMouseEvent;
Console.FocusEvent -= ConsoleOnFocusEvent;
Console.Dispose();
+ _resizeCancellationTokenSource?.Dispose();
+ (PixelBuffer as IDisposable)?.Dispose();
}
- // TODO: free unmanaged resources (unmanaged objects) and override finalizer
- // TODO: set large fields to null
+ PixelBuffer = null;
_disposedValue = true;
}
}src/Consolonia.Core/Infrastructure/InputLessDefaultNetConsole.cs (3)
50-72:
Improve reliability of emoji support detection
The current implementation has several potential issues:
- Writing to console during detection may cause visual artifacts
- Using cursor position for detection might be unreliable across different terminal emulators
- Defaulting to
trueon failure could lead to incorrect behavior - The TODO comment suggests incomplete implementation
Consider this alternative approach that uses buffer width checking:
public bool SupportsComplexEmoji
{
get
{
if (_supportEmoji == null)
{
- // Detect complex emoji support by writing a complex emoji and checking cursor position.
- // If the cursor moves 2 positions, it indicates proper rendering of composite surrogate pairs.
- (int left, int top) = Console.GetCursorPosition();
- WriteText(
- $"{Esc.Foreground(Colors.Transparent)}{Esc.Background(Colors.Transparent)}{TestEmoji}");
-
- // TODO, escape sequence
- (int left2, _) = Console.GetCursorPosition();
- _supportEmoji = left2 - left == 2;
- Console.SetCursorPosition(left, top);
-
- WriteText(Esc.ClearScreen);
+ try
+ {
+ var testStr = TestEmoji;
+ _supportEmoji = Console.BufferWidth >= testStr.Length * 2;
+ }
+ catch (Exception)
+ {
+ _supportEmoji = false; // Safer default
+ }
}
- return _supportEmoji ?? true;
+ return _supportEmoji ?? false; // Safer default
}
}164-165: 🛠️ Refactor suggestion
Ensure reliable console state restoration
The current implementation might leave the console in an inconsistent state if an exception occurs during cleanup.
Wrap the console state restoration in a try-finally block:
- WriteText(Esc.DisableAlternateBuffer);
- WriteText(Esc.ShowCursor);
+ try
+ {
+ WriteText(Esc.DisableAlternateBuffer);
+ WriteText(Esc.ShowCursor);
+ }
+ finally
+ {
+ // Ensure we never leave the console in an inconsistent state
+ }141-145:
Avoid virtual method in constructor chain
The WriteText method is called from PrepareConsole which is invoked in the constructor. This is dangerous as derived classes may override WriteText before their constructors are called.
Consider either:
- Making
WriteTextnon-virtual - Moving console preparation to an
Initializemethod called after construction
- public virtual void WriteText(string str)
+ protected internal void WriteText(string str)
{
PauseTask?.Wait();
Console.Write(str);
}Likely invalid or redundant comment.
Co-authored-by: <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Designer/ConsolePreview.cs (3)
253-254: Fix typo in commentThere's a minor typo in the comment: "we continue to check the proces." The word "proces" should be "process".
269-269: Address the TODO: Use theme background brushThe comment indicates that the
Backgroundshould use the theme's background brush instead of a hardcoded color. This improves consistency with the application's theme and user settings.Would you like assistance in implementing this change?
371-371: Address the TODO: MoveTextBlockComposerclass outThe
TextBlockComposerclass has a TODO comment suggesting it should be moved out. To improve code organization and maintainability, consider moving this nested class to a separate file or refactoring it as appropriate.Would you like assistance in refactoring this class?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Designer/ConsolePreview.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Designer/ConsolePreview.cs (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/ConsolePreview.cs:161-173
Timestamp: 2024-11-29T19:09:22.038Z
Learning: In C#, the `File.ReadAllText` method throws an `IOException`, not a `FileNotFoundException`. Therefore, catching `IOException` is sufficient to handle scenarios where a file is not found or cannot be accessed.
🔇 Additional comments (4)
src/Consolonia.Designer/ConsolePreview.cs (4)
5-455: Reconsider extensive use of #if DEBUG directives affecting functionality
A significant portion of the code, including essential methods like LoadXaml(), ListenForChanges(), and the _process field, is wrapped within #if DEBUG directives. This means that in Release builds, critical functionality of the ConsolePreview control will be excluded, potentially leading to runtime issues or a non-functional control outside of Debug configurations.
Consider the following actions:
- Refactor the code to minimize the use of
#if DEBUGdirectives, ensuring that necessary functionality is included in all build configurations. - Implement design-time checks using
Design.IsDesignModeto distinguish between runtime and design-time environments instead of relying solely on compilation symbols. - Document the intended behavior clearly if the control is meant to function only in Debug mode.
98-126:
Implement the IDisposable interface
The ConsolePreview class defines Dispose() and Dispose(bool disposing) methods but does not implement the IDisposable interface. This may lead to unexpected behavior when the object is disposed, as the dispose pattern relies on the interface to signal that resources should be released.
Apply this diff to implement IDisposable:
-public class ConsolePreview : UserControl
+public class ConsolePreview : UserControl, IDisposable
{
// Class implementation
}
+public void Dispose()
+{
+ Dispose(true);
+ GC.SuppressFinalize(this);
+}161-179: 🛠️ Refactor suggestion
Simplify exception handling by catching only IOException
File.ReadAllText throws IOException for most file access errors, including file not found and unauthorized access. Simplifying the exception handling by catching only IOException makes the code cleaner and more maintainable.
Apply this diff to adjust the exception handling:
try
{
xaml = File.ReadAllText(xamlPath);
ArgumentNullException.ThrowIfNull(xaml);
}
- catch (FileNotFoundException ex)
- {
- Content = new TextBlock { Text = $"XAML file not found. {ex.Message}", Foreground = Brushes.Red };
- return;
- }
- catch (UnauthorizedAccessException ex)
- {
- Content = new TextBlock { Text = $"Unable to access XAML file. {ex.Message}", Foreground = Brushes.Red };
- return;
- }
catch (IOException ex)
{
Content = new TextBlock { Text = $"Unable to load XAML file. {ex.Message}", Foreground = Brushes.Red };
return;
}348-360:
Add validation to prevent exceptions when parsing design dimensions
In the GetDesignWidthAndHeight method, if the expected patterns d:DesignWidth or d:DesignHeight are not found, IndexOf may return -1, leading to ArgumentOutOfRangeException when using Substring. Adding validation ensures that iStart and iEnd are valid before attempting to extract substrings.
Apply this diff to add validation:
int iStart = xaml.IndexOf("d:DesignWidth=\"", StringComparison.Ordinal);
if (iStart > -1)
{
iStart += 15;
int iEnd = xaml.IndexOf("\"", iStart, StringComparison.Ordinal);
+ if (iEnd > iStart)
+ {
string num = xaml.Substring(iStart, iEnd - iStart);
_ = ushort.TryParse(num, out designWidth);
+ }
}
iStart = xaml.IndexOf("d:DesignHeight=\"", StringComparison.Ordinal);
if (iStart > -1)
{
iStart += 16;
int iEnd = xaml.IndexOf("\"", iStart, StringComparison.Ordinal);
+ if (iEnd > iStart)
+ {
string num = xaml.Substring(iStart, iEnd - iStart);
_ = ushort.TryParse(num, out designHeight);
+ }
}
I've opened a separate PR for this. Will cleanup this code after in that same PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Designer/ConsolePreview.cs (3)
253-253: Correct typographical error in commentThere is a typo in the comment: "we continue to check the proces." It should be "we continue to check the process."
Apply this diff to fix the typo:
- // process was probably shut down, we continue to check the proces. + // Process was probably shut down; we continue to check the process.
371-371: Address TODO: MoveTextBlockComposerclass to a separate fileThe
TextBlockComposernested class is marked with a TODO comment to move it out. For better code organization and maintainability, consider moving it to its own file.Would you like assistance in refactoring
TextBlockComposerinto a separate file?
249-249: UseInvokeAsyncinstead ofInvoketo prevent UI blockingWhen updating the UI, using
Dispatcher.UIThread.Invokecan block the UI thread. Consider usingInvokeAsyncto ensure responsive UI.Apply this diff:
- Dispatcher.UIThread.Invoke(() => Content = RenderPixelBuffer(buffer)); + await Dispatcher.UIThread.InvokeAsync(() => Content = RenderPixelBuffer(buffer));This change updates the UI asynchronously, improving application responsiveness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Designer/ConsolePreview.cs(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Designer/ConsolePreview.cs (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/ConsolePreview.cs:161-173
Timestamp: 2024-11-29T19:09:22.038Z
Learning: In C#, the `File.ReadAllText` method throws an `IOException`, not a `FileNotFoundException`. Therefore, catching `IOException` is sufficient to handle scenarios where a file is not found or cannot be accessed.
🔇 Additional comments (6)
src/Consolonia.Designer/ConsolePreview.cs (6)
46-58:
Beware of functionality exclusion due to #if DEBUG directives
The constructor and several critical methods are wrapped within #if DEBUG directives. This means that in Release builds, essential functionalities like process initialization and event subscriptions will be excluded, potentially leading to a non-functional control.
Consider refactoring the code to minimize the use of #if DEBUG directives. Use Design.IsDesignMode to check for design-time environment instead of compilation symbols.
98-118:
Ensure proper disposal by implementing IDisposable interface
The ConsolePreview class defines a Dispose method but does not implement the IDisposable interface. This may lead to improper disposal of resources and unmanaged processes.
Apply this diff to implement the IDisposable interface:
-public class ConsolePreview : UserControl
+public class ConsolePreview : UserControl, IDisposableAlso, ensure to call GC.SuppressFinalize(this); in the Dispose() method to prevent the finalizer from running:
public void Dispose()
{
// Do not change this code. Put cleanup code in 'Dispose(bool disposing)' method
Dispose(true);
+ GC.SuppressFinalize(this);
}164-179: 🛠️ Refactor suggestion
Simplify exception handling by catching only IOException
In the LoadXaml method, catching FileNotFoundException and UnauthorizedAccessException separately is unnecessary because File.ReadAllText throws an IOException for these errors.
Apply this diff to simplify the exception handling:
try
{
xaml = File.ReadAllText(xamlPath);
ArgumentNullException.ThrowIfNull(xaml);
}
-catch (FileNotFoundException ex)
-{
- Content = new TextBlock { Text = $"XAML file not found. {ex.Message}", Foreground = Brushes.Red };
- return;
-}
-catch (UnauthorizedAccessException ex)
-{
- Content = new TextBlock { Text = $"Unable to access XAML file. {ex.Message}", Foreground = Brushes.Red };
- return;
-}
catch (IOException ex)
{
Content = new TextBlock { Text = $"Unable to load XAML file. {ex.Message}", Foreground = Brushes.Red };
return;
}347-361:
Add validation to prevent exceptions when parsing design dimensions
In GetDesignWidthAndHeight, if the expected patterns are not found, IndexOf may return -1, leading to exceptions when using Substring.
Apply this diff to add necessary validation:
int iStart = xaml.IndexOf("d:DesignWidth=\"", StringComparison.Ordinal);
-if (iStart > 0)
+if (iStart >= 0)
{
iStart += 15;
int iEnd = xaml.IndexOf("\"", iStart, StringComparison.Ordinal);
+ if (iEnd > iStart)
+ {
string num = xaml.Substring(iStart, iEnd - iStart);
_ = ushort.TryParse(num, out designWidth);
+ }
}
+iStart = xaml.IndexOf("d:DesignHeight=\"", StringComparison.Ordinal);
+if (iStart >= 0)
+{
+ iStart += 16;
+ int iEnd = xaml.IndexOf("\"", iStart, StringComparison.Ordinal);
+ if (iEnd > iStart)
+ {
+ string num = xaml.Substring(iStart, iEnd - iStart);
+ _ = ushort.TryParse(num, out designHeight);
+ }
+}This adds checks to ensure indices are valid before attempting to parse substrings.
236-262:
Handle potential exceptions when reading from standard output
In the ListenForChanges method, reading from _process.StandardOutput may throw exceptions if the process exits unexpectedly.
Apply this diff to add exception handling:
while (true)
{
if (_process == null || _process.HasExited)
return;
- string? line = await _process!.StandardOutput.ReadLineAsync().ConfigureAwait(false);
+ string? line;
+ try
+ {
+ line = await _process!.StandardOutput.ReadLineAsync().ConfigureAwait(false);
+ }
+ catch (Exception ex)
+ {
+ Debug.WriteLine($"Error reading from process output: {ex.Message}");
+ return;
+ }
if (string.IsNullOrEmpty(line)) continue;
// Rest of the code...
}This ensures that any exceptions during the read operation are caught and handled gracefully.
Likely invalid or redundant comment.
282-291:
Prevent potential infinite loop due to zero-width symbols
In the loop handling Pixel objects, a zero Symbol.Width could lead to an infinite loop.
Apply this diff to ensure that widthAdjust is always at least 1:
ushort widthAdjust = pixel.Foreground.Symbol.Width == 0 ? (ushort)1 : pixel.Foreground.Symbol.Width;
+if (widthAdjust == 0)
+{
+ widthAdjust = 1;
+}
x += widthAdjust;This ensures that the loop progresses and avoids the risk of an infinite loop.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (2)
21-21: Consider documenting the public API surfaceThe class has been made public, which expands the API surface. Consider adding XML documentation to describe the class's purpose, responsibilities, and usage guidelines.
256-261: Decide on finalizer implementationThe commented-out finalizer suggests uncertainty about unmanaged resource handling. Either:
- Remove the commented code if no unmanaged resources are present
- Implement the finalizer if unmanaged resources need cleanup
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)
🔇 Additional comments (3)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (3)
316-318: Consider thread-safety when writing to System.Console
Direct writes to System.Console might not be thread-safe. Consider using the injected IConsole interface instead.
337-339: Handle resource cleanup during PixelBuffer reassignment
When creating a new PixelBuffer, ensure the old instance is properly disposed if it implements IDisposable.
391-410:
Complete resource cleanup in Dispose method
The Dispose method needs attention in several areas:
- Add disposal of
PixelBufferin the disposing block - Remove TODO comments after implementing resource cleanup
- Consider disposing
_resizeCancellationTokenSource
if (disposing)
{
- // TODO: dispose managed state (managed objects)
Closed?.Invoke();
Console.Resized -= OnConsoleOnResized;
Console.KeyEvent -= ConsoleOnKeyEvent;
Console.MouseEvent -= ConsoleOnMouseEvent;
Console.FocusEvent -= ConsoleOnFocusEvent;
+ (PixelBuffer as IDisposable)?.Dispose();
+ _resizeCancellationTokenSource?.Dispose();
Console.Dispose();
}
- // TODO: free unmanaged resources (unmanaged objects) and override finalizer
- // TODO: set large fields to null
_disposedValue = true;Likely invalid or redundant comment.
This reverts commit ff89176.
|
GAH. I believed coderabbit that it was dispossable. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (2)
21-21: Document public API surfaceThe class has been made public, which exposes it as part of the public API. Consider adding XML documentation to describe its purpose, usage, and any threading considerations.
396-396: Address remaining TODO comments in Dispose methodThe Dispose method contains unaddressed TODO comments. Please:
- Document any managed resources that need disposal
- Document any unmanaged resources that need cleanup
- Remove the TODO comments once addressed
This is important for proper resource management and preventing memory leaks.
Also applies to: 405-406
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs(6 hunks)
🔇 Additional comments (3)
src/Consolonia.Core/Infrastructure/ConsoleWindow.cs (3)
43-43: Protect PixelBuffer from unauthorized modifications
The public setter on PixelBuffer could lead to thread-safety issues and invalid state.
316-318: Consider thread-safety when writing to System.Console
Direct writes to System.Console might not be thread-safe.
337-338: Handle resource cleanup during PixelBuffer reassignment
When creating a new PixelBuffer, ensure the old instance is properly disposed.
|
I need you to approve again. this is what you already approved, |
|
W00t! |
TLDR: This checkin adds ability to preview consolonia .axaml files while editing them.
Backstory
I ended up implementing multiple layered approaches to try and acheive this goal.
Attempt 1 - try and not have the built-avalonia viewer puke
Basically, the reason the previewer pukes was because of a variety of issues.
Changes Made:
The effect of this is that Text and colors basically render correctly, but issues are:
So...basically, it's not as shitty as having a broken pane, but not really useful at all.
Attempt 2 - Avalonia PreviewControl displayed via Design.Preview
I had noticed that Avalonia has the following markup:
That got me thinking that I could create Avalonia control which talks to a consolonia app, and if you simply dropped that control in there it would give you the preview.
Changes made
The Consolonia.PreviewHost and Avalonia.PreviewHost look and work great.
Here's some clips:
Consolonia.PreviewHost
Avalonia.PreviewHost
So this is looking super promising. There are no real issues yet except to hook it up.
Well.
😠😭💔
The avalonia preview window only honors the Design.WithPreview tag for Style and Resources, not for user controls. It's a couple line change in avalonia to add the support, but it doesn't work out of the box.
ConsolePreview with Design.WithPreview markup
To show what it looks like, here's clip where I used ConsolePreview control as the WithPreview for a Resource Dictionary.
This highlights several pretty serious issues:
This was a pretty despairing point for me, as I was starting to think this was a no-go. I started to think about creating my own VS integration (Which I have done in the past, but it's a painful thing to do.) If only I could just get the console app to work directly in vs.......hey.....wait a minute.....
Attempt 3 - Host the console preview app in a terminal window.
With no more code changes, I tried just running the Consolonia.PreviewHost app in a terminal window. The result is actually pretty magic, and it works in both Visual Studio and Visual Studio Code.
Visual Studio
With single file preview
With project preview
Visual Studio Code
With single file preview
With project preview
Pros:
Cons:
Changes made
none! 🤪
The only issues are:
At any rate, I think this is pretty respectable solution, just use Consolonia.PreviewHost.exe for live previews, and why not? We should be dog fooded our own tool to make tools, no?
I may at a future point try and create an integration that tightly bounds the active file to the preview window, but for now I'm going to declare victory.
I'm keeping the Avalonia.PreviewHost and PreivewControl because I spent a lot of time to get them working, and they don't hurt anything.
The stuff I did to make it so the built-in previewer doesn't barf I think is useful, but again very easy to not use if you don't want it. It's all isolated in the Consolonia.Designer assembly, and so you can ignore it if you don't like it.
BTW: I made Consolonia.PreviewHost a dotnet tool, so you can install it with
OBTW: I also added 2 new color themes: Fluent and Material. They look very similar but I think it looks way better.
Summary by CodeRabbit
Release Notes
New Features
DummyConsolefor basic console functionality.ConsolePreviewfor rendering console AXAML outputs in Visual Studio.DrawingBoxSymbol,Pixel,PixelForeground, andSimpleSymbol.FluentandMaterialthemes added for enhanced UI styling.Improvements
DrawingContextImpl.Bug Fixes
Documentation