Prepare build and packages for publishing #175
Conversation
**PublishNuget** - builds release and publishes nuget and symbol nuget for the current checked in version **PublishIncrementNuget** - Bumps incremental build number (11.2.X) - builds release and publishes nuget and symbol nuget for the - Checks in updated build number NOTE: This needs a NUGET_KEY secret added
…he right dependencies
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces significant changes across multiple files, primarily focusing on the addition of new GitHub Actions workflows for publishing NuGet packages. Two new workflow files, Additionally, the project file 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: 12
🧹 Outside diff range and nitpick comments (15)
src/Consolonia.GuiCS/Consolonia.GuiCS.csproj (1)
1-1: Remove BOM character from project fileThe file starts with a BOM (Byte Order Mark) character. While this doesn't affect functionality, it's best practice to avoid BOM in project files for consistency.
-<Project Sdk="Microsoft.NET.Sdk"> +<Project Sdk="Microsoft.NET.Sdk">.github/workflows/PublishNuget.yml (1)
28-34: Enhance package publishing security and reliabilityConsider these improvements for the package publishing step:
- Add condition to prevent publishing if build fails
- Upload packages as artifacts for audit trail
- Add package signature verification
+ - name: Upload packages as artifacts + uses: actions/upload-artifact@v4 + with: + name: nuget-packages + path: packages/ + retention-days: 30 + - name: Publish NuGet and symbols + if: success() id: nuget-push uses: edumserrano/nuget-push@v1 with: api-key: '${{ secrets.NUGET_KEY }}' working-directory: 'packages' fail-if-exists: false🧰 Tools
🪛 yamllint (1.35.1)
[error] 32-32: trailing spaces
(trailing-spaces)
src/Consolonia.Core/Consolonia.Core.csproj (1)
Line range hint
5-12: Consider standardizing package version referencesWhile not directly related to the packaging changes, it would be good practice to move common package versions to a central location.
Consider moving version numbers for packages like
CommunityToolkit.Mvvm,SkiaSharp,Unicode.net, etc., to the Directory.Build.props file, similar to how$(AvaloniaVersion)is handled.src/Tools/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj (1)
Line range hint
16-24: Consider moving debug-only package reference to a conditional ItemGroupFor better organization, consider moving debug-only package references to a separate conditional ItemGroup.
<ItemGroup> <PackageReference Include="Avalonia" Version="$(AvaloniaVersion)" /> <PackageReference Include="Avalonia.Desktop" Version="$(AvaloniaVersion)" /> <PackageReference Include="Avalonia.Themes.Fluent" Version="$(AvaloniaVersion)" /> <PackageReference Include="Avalonia.Fonts.Inter" Version="$(AvaloniaVersion)" /> <PackageReference Include="CommunityToolkit.Mvvm" Version="8.3.2" /> <PackageReference Include="Newtonsoft.Json" Version="13.0.3" /> + </ItemGroup> - <!--Condition below is needed to remove Avalonia.Diagnostics package from build output in Release configuration.--> - <PackageReference Condition="'$(Configuration)' == 'Debug'" Include="Avalonia.Diagnostics" Version="$(AvaloniaVersion)" /> + <ItemGroup Condition="'$(Configuration)' == 'Debug'"> + <PackageReference Include="Avalonia.Diagnostics" Version="$(AvaloniaVersion)" /> </ItemGroup>.github/workflows/PublishIncrementalNuget.yml (2)
18-24: Consider adding version bump validationThe automatic version bumping could cause conflicts if multiple PRs are merged in quick succession.
Consider:
- Adding a validation step to ensure the version hasn't been bumped by another PR
- Using semantic versioning with major.minor.patch format
- Adding a manual input parameter to specify the version bump type (patch/minor/major)
32-34: Add package validation stepConsider validating the NuGet packages before publishing.
Add a validation step:
- name: Validate packages run: | for package in packages/*.nupkg; do dotnet nuget verify $package done🧰 Tools
🪛 yamllint (1.35.1)
[error] 32-32: trailing spaces
(trailing-spaces)
src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (1)
25-32: Consider adding package validation for copied assetsThe custom target for copying project references looks good, but consider adding validation.
Add a validation target:
<Target Name="ValidatePackageAssets" AfterTargets="CopyProjectReferencesToPackage"> <Error Condition="!Exists('$(OutputPath)Consolonia.GuiCS.dll')" Text="Required asset Consolonia.GuiCS.dll was not found in the output directory." /> </Target>src/Consolonia.NUnit/README.md (3)
7-7: Fix grammar in project status noteThe project status note is missing an article.
-> Project is in proof of concept state and is looking for collaboration. +> The project is in a proof of concept state and is looking for collaboration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: A determiner appears to be missing. Consider inserting it.
Context: ... styling and the rest from Avalonia. > Project is in proof of concept state and is loo...(AI_EN_LECTOR_MISSING_DETERMINER)
12-12: Format bare URL as a proper markdown linkConvert the bare URL to a proper markdown link for better readability and maintainability.
-Example of usage: https://github.com/jinek/ToolUI +Example of usage: [ToolUI Repository](https://github.com/jinek/ToolUI)🧰 Tools
🪛 Markdownlint (0.35.0)
12-12: null
Bare URL used(MD034, no-bare-urls)
14-14: Improve reference to additional documentationThe reference to additional documentation is vague and could be more helpful to users.
-Solution contains one more readme file with coding information. +For detailed development guidelines, see [Coding Guidelines](../docs/coding-guidelines.md).src/Consolonia.Themes.TurboVision/README.md (2)
1-5: Enhance the introduction section with more detailsWhile the introduction provides a good overview, it could be more informative by:
- Adding a brief description of what TurboVision theme specifically offers
- Including installation instructions or package information
- Mentioning key features specific to this theme
# Consolonia UI -TUI (Text User Interface) (GUI Framework) implementation for [Avalonia UI](https://github.com/AvaloniaUI) +TUI (Text User Interface) implementation for [Avalonia UI](https://github.com/AvaloniaUI), featuring a TurboVision-inspired theme. -Supports XAML, data bindings, animation, styling and the rest from Avalonia. +## Features +- Full Avalonia UI support including XAML, data bindings, animations, and styling +- Classic TurboVision-style appearance + +## Installation +```bash +dotnet add package Consolonia.Themes.TurboVision +```
7-7: Improve the project status noticeThe current status notice could be more informative about what kind of collaboration is needed.
-> Project is in proof of concept state and is looking for collaboration. +> ⚠️ This project is in a proof-of-concept state. We welcome contributions in the following areas: +> - Bug reports and fixes +> - Feature implementations +> - Documentation improvements +> - Testing and feedback🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: Possible missing article found.
Context: ... styling and the rest from Avalonia. > Project is in proof of concept state and is loo...(AI_HYDRA_LEO_MISSING_THE)
src/Consolonia.Blazor/README.md (3)
7-7: Improve grammar in project status noteConsider revising the project status note for better clarity.
-> Project is in proof of concept state and is looking for collaboration. +> This project is in a proof-of-concept state and is looking for collaboration.🧰 Tools
🪛 LanguageTool
[uncategorized] ~7-~7: Possible missing article found.
Context: ... styling and the rest from Avalonia. > Project is in proof of concept state and is loo...(AI_HYDRA_LEO_MISSING_THE)
12-12: Format bare URL as a proper markdown linkUse proper markdown link syntax for better readability and compliance with markdown best practices.
-Example of usage: https://github.com/jinek/ToolUI +Example of usage: [ToolUI Repository](https://github.com/jinek/ToolUI)🧰 Tools
🪛 Markdownlint (0.35.0)
12-12: null
Bare URL used(MD034, no-bare-urls)
14-14: Clarify documentation referenceThe reference to another readme file is vague. Consider specifying the location or purpose of the additional documentation.
-Solution contains one more readme file with coding information. +For development and coding guidelines, please refer to the [Developer Documentation](../README.md).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
.github/workflows/PublishIncrementalNuget.yml(1 hunks).github/workflows/PublishNuget.yml(1 hunks)Directory.Build.props(1 hunks)foo.cmd(1 hunks)src/Consolonia.Blazor/README.md(1 hunks)src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs(0 hunks)src/Consolonia.Core/README.md(1 hunks)src/Consolonia.Designer/Consolonia.Designer.csproj(1 hunks)src/Consolonia.Designer/README.md(1 hunks)src/Consolonia.GuiCS/Consolonia.GuiCS.csproj(1 hunks)src/Consolonia.GuiCS/readme.md(0 hunks)src/Consolonia.NUnit/Consolonia.NUnit.csproj(1 hunks)src/Consolonia.NUnit/README.md(1 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Consolonia.PlatformSupport/README.md(1 hunks)src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj(1 hunks)src/Consolonia.Themes.TurboVision/README.md(1 hunks)src/Tools/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj(1 hunks)src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj(1 hunks)
💤 Files with no reviewable changes (2)
- src/Consolonia.GuiCS/readme.md
- src/Consolonia.Core/Infrastructure/ConsoloniaNotSupportedException.cs
✅ Files skipped from review due to trivial changes (5)
- Directory.Build.props
- src/Consolonia.Designer/README.md
- src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj
- src/Consolonia.Core/README.md
- foo.cmd
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/PublishNuget.yml
14-14: the runner of "actions/setup-dotnet@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/PublishIncrementalNuget.yml
14-14: the runner of "actions/setup-dotnet@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/PublishNuget.yml
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 22-22: trailing spaces
(trailing-spaces)
[error] 23-23: trailing spaces
(trailing-spaces)
[error] 24-24: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 35-35: trailing spaces
(trailing-spaces)
.github/workflows/PublishIncrementalNuget.yml
[error] 10-10: trailing spaces
(trailing-spaces)
[error] 12-12: trailing spaces
(trailing-spaces)
[error] 17-17: trailing spaces
(trailing-spaces)
[error] 25-25: trailing spaces
(trailing-spaces)
[error] 30-30: trailing spaces
(trailing-spaces)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 32-32: trailing spaces
(trailing-spaces)
[error] 40-40: trailing spaces
(trailing-spaces)
[error] 43-43: trailing spaces
(trailing-spaces)
🪛 LanguageTool
src/Consolonia.Blazor/README.md
[uncategorized] ~7-~7: Possible missing article found.
Context: ... styling and the rest from Avalonia. > Project is in proof of concept state and is loo...
(AI_HYDRA_LEO_MISSING_THE)
src/Consolonia.NUnit/README.md
[uncategorized] ~7-~7: A determiner appears to be missing. Consider inserting it.
Context: ... styling and the rest from Avalonia. > Project is in proof of concept state and is loo...
(AI_EN_LECTOR_MISSING_DETERMINER)
src/Consolonia.PlatformSupport/README.md
[uncategorized] ~7-~7: Possible missing article found.
Context: ... styling and the rest from Avalonia. > Project is in proof of concept state and is loo...
(AI_HYDRA_LEO_MISSING_THE)
src/Consolonia.Themes.TurboVision/README.md
[uncategorized] ~7-~7: Possible missing article found.
Context: ... styling and the rest from Avalonia. > Project is in proof of concept state and is loo...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~13-~13: A determiner appears to be missing. Consider inserting it.
Context: ...usage: https://github.com/jinek/ToolUI Solution contains one more readme file with codi...
(AI_EN_LECTOR_MISSING_DETERMINER)
🪛 Markdownlint (0.35.0)
src/Consolonia.Blazor/README.md
12-12: null
Bare URL used
(MD034, no-bare-urls)
src/Consolonia.NUnit/README.md
12-12: null
Bare URL used
(MD034, no-bare-urls)
src/Consolonia.PlatformSupport/README.md
12-12: null
Bare URL used
(MD034, no-bare-urls)
src/Consolonia.Themes.TurboVision/README.md
12-12: null
Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (5)
src/Consolonia.Designer/Consolonia.Designer.csproj (1)
14-16: LGTM! Project references are well organized
The project references are properly formatted and the conditional reference for PreviewHost in Debug configuration is appropriate.
src/Tools/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj (1)
8-8: LGTM! Appropriate packaging configuration
Setting IsPackable to false is correct for this tool project, as it's not intended for distribution via NuGet. This aligns with the PR's objective of organizing package publishing.
src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj (1)
26-26: LGTM: Package reference and metadata changes look good!
The addition of the Avalonia.Markup.Xaml.Loader package and the handling of package metadata through ItemGroup entries follow best practices.
src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (1)
6-9: Verify GuiCS consolidation impact
The GuiCS consolidation looks correct, but we should verify its impact on dependent packages.
✅ Verification successful
GuiCS consolidation is correctly isolated
The GuiCS project reference exists only in PlatformSupport.csproj, confirming that the consolidation is properly contained and won't impact other packages directly. The PrivateAssets="All" configuration ensures the dependency remains internal to the platform package.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any direct references to Consolonia.GuiCS in other projects
rg -l "Include=\".*Consolonia\.GuiCS\.csproj\"" --type csproj
Length of output: 92
Script:
#!/bin/bash
# Find all csproj files and check for GuiCS references
fd '\.csproj$' | xargs grep -l "Consolonia\.GuiCS\.csproj"
Length of output: 125
src/Consolonia.Themes.TurboVision/README.md (1)
9-10: LGTM! Well-structured showcase section
The showcase section effectively uses a clickable image linking to a video demonstration, which is a good practice for documentation.
Add ConsoloniaApplication<MainWindow> Cleaned up turbovision stuff
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (21)
src/Tests/Consolonia.Core.Tests/MiscTests.cs (2)
13-13: Fix typo in test method nameThe method name contains a typo: "TestExectpionRequest" should be "TestExceptionRequest"
- public void TestExectpionRequest() + public void TestExceptionRequest()
1-5: Remove unused importsThe following imports appear to be unused in this test file:
Avalonia.MediaConsolonia.Core.Drawing.PixelBufferImplementationusing System; -using Avalonia.Media; -using Consolonia.Core.Drawing.PixelBufferImplementation; using Consolonia.Core.Infrastructure; using NUnit.Framework;src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (1)
25-32: LGTM! Custom build target implementation is solid.The target correctly handles project reference copying while preserving the package structure. Consider documenting this target in the repository's build documentation for future maintainers.
To improve maintainability:
- Consider adding this as a reusable target in Directory.Build.targets if other projects need similar functionality
- Document the target's purpose and usage in the build documentation
src/Consolonia.Themes.TurboVision/Themes/TurboVisionTheme.cs (1)
Line range hint
10-24: Consider documenting the constructor choicesThe class now has three constructors with different initialization paths. Consider adding XML documentation to explain when to use each constructor variant.
Add documentation like this:
+ /// <summary> + /// Initializes a new instance with the default TurboVision theme URI. + /// </summary> public TurboVisionTheme() : base(Uri) { } + /// <summary> + /// Initializes a new instance with a custom base URI. + /// </summary> + /// <param name="baseUri">The custom URI to use for resource loading.</param> public TurboVisionTheme(Uri baseUri) : base(baseUri) { } + /// <summary> + /// Initializes a new instance with a service provider for dependency injection. + /// </summary> + /// <param name="serviceProvider">The service provider for resolving dependencies.</param> public TurboVisionTheme(IServiceProvider serviceProvider) : base(serviceProvider) { }src/Consolonia.Designer/Extensions.cs (1)
Line range hint
13-17: Update documentation to reflect new behaviorThe XML documentation states "In release mode it will load consolonia, removing the dependency on the desktop avalonia subsystem." This is no longer accurate since the design mode check is now performed in all builds, not just debug builds.
- /// In Design mode it will load up avalonia instead of consolonia so that avalonia - /// previewer will attempt to render it. This mostly only works for text layout. - /// In release mode it will load consolonia, removing the dependency on the desktop avalonia subsystem. - /// NOTE: The package references for + /// When in design mode, it loads Avalonia for preview rendering (primarily supporting text layout). + /// Otherwise, it loads Consolonia for normal operation. + /// This check occurs in all build configurations.src/Example/Program.cs (1)
17-21: Consider documenting configuration optionsThe BuildAvaloniaApp method improves testability by extracting configuration. Consider adding XML documentation to explain available options and their effects.
+ /// <summary> + /// Configures and builds the Avalonia application with Consolonia support. + /// </summary> + /// <returns>Configured AppBuilder instance</returns> public static AppBuilder BuildAvaloniaApp()src/Example/App.cs (2)
12-15: Remove or document commented theme optionsConsider either:
- Removing the commented theme options if they're not needed
- Converting them into configurable options if they represent valid alternatives
- Adding a comment explaining why they're kept for reference
8-8: Consider making theme selection configurableThe MaterialTheme is hardcoded. Consider making theme selection configurable through app settings or environment variables to improve flexibility.
Also applies to: 16-16
src/Consolonia.Themes.TurboVision/Themes/FluentTheme.cs (1)
10-12: Consider reducing URI duplicationThe same URI is specified in both the constructor and the Uri property. Consider centralizing this to a single constant to improve maintainability.
public class FluentTheme : ResourceIncludeBase { + private static readonly Uri ThemeUri = new("avares://Consolonia.Themes.TurboVision/Themes/Fluent/Fluent.axaml"); - public FluentTheme() : base(new Uri("avares://Consolonia.Themes.TurboVision/Themes/Fluent/Fluent.axaml")) + public FluentTheme() : base(ThemeUri) { } // ... other constructors ... - protected override Uri Uri => - new("avares://Consolonia.Themes.TurboVision/Themes/Fluent/Fluent.axaml"); + protected override Uri Uri => ThemeUri;Also applies to: 21-22
src/Consolonia.Themes.TurboVision/Themes/TurboVisionDarkTheme.cs (1)
7-7: Standardize comment styleThe ReSharper suppression comment uses inline style while other files use attribute style. Consider standardizing across all theme files.
- // ReSharper disable once UnusedType.Global + [SuppressMessage("ReSharper", "UnusedType.Global")] public class TurboVisionDarkTheme : ResourceIncludeBasesrc/Consolonia.Themes.TurboVision/Themes/TurboVisionBlackTheme.cs (1)
10-12: Consider a base theme implementationGiven the identical pattern across all theme classes, consider introducing a base theme class to handle common initialization logic.
Example implementation:
public abstract class ConsoloniaThemeBase : ResourceIncludeBase { private readonly string _themeUri; protected ConsoloniaThemeBase(string themeUri) { _themeUri = themeUri; } protected override Uri Uri => new(_themeUri); } public class TurboVisionBlackTheme : ConsoloniaThemeBase { private const string ThemeUri = "avares://Consolonia.Themes.TurboVision/Themes/TurboVisionBlack/TurboVisionBlack.axaml"; public TurboVisionBlackTheme() : base(ThemeUri) { } // Other constructors... }Also applies to: 21-22
src/Consolonia.PlatformSupport/README.md (2)
2-2: Fix macOS spellingChange "MacOS" to "macOS" to follow Apple's official naming convention.
-This package provides Consolonia IConsole platform support for adapting to consoles on Windows, Linux and MacOS environments. +This package provides Consolonia IConsole platform support for adapting to consoles on Windows, Linux and macOS environments.🧰 Tools
🪛 LanguageTool
[grammar] ~2-~2: The operating system from Apple is written “macOS”.
Context: ...pting to consoles on Windows, Linux and MacOS environments. ## Background Consolonia...(MAC_OS)
12-20: Enhance usage documentationThe usage section could benefit from:
- Prerequisites or installation instructions
- Explanation of the configuration options (
UseConsolonia()andUseAutoDetectedConsole())- Platform-specific considerations for Windows, Linux, and macOS
Would you like me to help expand this section with more detailed documentation?
src/Consolonia.Designer/README.md (1)
9-9: Improve experimental warning formattingThe experimental warning should be more prominent and better formatted.
-This package **EXPERIMENTAL** Consolonia support for design time previews. +> ⚠️ **EXPERIMENTAL**: This package provides Consolonia support for design time previews.src/Consolonia.Themes.TurboVision/README.md (2)
6-6: Fix heading hierarchyMaintain consistent heading levels throughout the document.
-# Showcase (click picture to see video) +## Showcase (click picture to see video) -# Usage +## UsageAlso applies to: 16-16
9-14: Add theme previewsConsider adding screenshots or ASCII art examples for each theme to help users visualize the differences between themes.
Would you like me to help create a theme preview section with example screenshots or ASCII art representations?
src/Consolonia.NUnit/README.md (1)
13-13: Improve readability with proper punctuation and articlesConsider these grammatical improvements:
- Line 13: Add a comma: "...against your consolonia application, your test class..."
- Line 15: Add articles: "...by passing a size parameter to the base class."
-To create a unit test against your consolonia application your test class should derive from ConsoloniaAppTestBase<App$gt;. +To create a unit test against your consolonia application, your test class should derive from ConsoloniaAppTestBase<App$gt;. -You can define the size of your console by passing size parameter to base class. +You can define the size of your console by passing a size parameter to the base class.Also applies to: 15-15
🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: A comma might be missing here.
Context: ...ate a unit test against your consolonia application your test class should derive from Cons...(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
src/Consolonia.Core/README.md (1)
30-39: Enhance code example with additional documentationConsider adding more comments to explain:
- The purpose of
ConsoloniaApplication<T>- Why MaterialTheme is being used
- What other theme options are available
// use HelloWorldWindow as the MainWindow for the application public class HelloWorldApp : ConsoloniaApplication<HelloWorldWindow> { public override void Initialize() { - // set the theme + // Set the MaterialTheme for consistent UI styling + // Other available themes can be found in Consolonia.Themes.TurboVision Styles.Add(new MaterialTheme()); } }src/Tools/Consolonia.PreviewHost/App.cs (2)
Line range hint
89-124: Remove or document commented-out codeThe file contains a large block of commented-out event handling code. If this code is no longer needed, it should be removed. If it's kept for future reference, it should be properly documented with the reason for keeping it.
Line range hint
11-15: Document culture setting in static constructorThe static constructor sets the culture to InvariantCulture without explaining why this is necessary.
static App() { - // we want tests and UI to be executed with same culture + // Set InvariantCulture for consistent formatting and parsing across different system locales + // This ensures that tests and UI behave consistently regardless of the user's system settings Thread.CurrentThread.CurrentUICulture = Thread.CurrentThread.CurrentCulture = CultureInfo.InvariantCulture; }src/Consolonia.Gallery/App.cs (1)
20-22: Remove commented-out theme code for clarityThe application now uses
MaterialThemefor styling, whileTurboVisionThemeandFluentThemeare commented out. Consider removing the commented-out theme code if it's no longer needed to enhance code readability and maintainability.Apply this diff to remove the unnecessary code:
-// Styles.Add(new TurboVisionTheme()); Styles.Add(new MaterialTheme()); -// Styles.Add(new FluentTheme());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (30)
Directory.Build.props(1 hunks)src/Consolonia.Core/Consolonia.Core.csproj(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Core/README.md(1 hunks)src/Consolonia.Designer/ConsolePreview.cs(0 hunks)src/Consolonia.Designer/Consolonia.Designer.csproj(2 hunks)src/Consolonia.Designer/Extensions.cs(1 hunks)src/Consolonia.Designer/README.md(1 hunks)src/Consolonia.Gallery/App.cs(2 hunks)src/Consolonia.NUnit/Consolonia.NUnit.csproj(1 hunks)src/Consolonia.NUnit/README.md(1 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Consolonia.PlatformSupport/README.md(1 hunks)src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj(1 hunks)src/Consolonia.Themes.TurboVision/README.md(1 hunks)src/Consolonia.Themes.TurboVision/Themes/FluentTheme.cs(1 hunks)src/Consolonia.Themes.TurboVision/Themes/MaterialTheme.cs(1 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlackTheme.cs(1 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionDarkTheme.cs(1 hunks)src/Consolonia.Themes.TurboVision/Themes/TurboVisionTheme.cs(1 hunks)src/Example/App.axaml(0 hunks)src/Example/App.axaml.cs(0 hunks)src/Example/App.cs(1 hunks)src/Example/Program.cs(1 hunks)src/Tests/Consolonia.Core.Tests/MiscTests.cs(1 hunks)src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs(1 hunks)src/Tools/Consolonia.PreviewHost/App.axaml(0 hunks)src/Tools/Consolonia.PreviewHost/App.cs(2 hunks)src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj(2 hunks)src/Tools/Consolonia.PreviewHost/readme.md(1 hunks)
💤 Files with no reviewable changes (4)
- src/Tools/Consolonia.PreviewHost/App.axaml
- src/Example/App.axaml.cs
- src/Example/App.axaml
- src/Consolonia.Designer/ConsolePreview.cs
✅ Files skipped from review due to trivial changes (1)
- src/Tools/Consolonia.PreviewHost/readme.md
🚧 Files skipped from review as they are similar to previous changes (6)
- Directory.Build.props
- src/Consolonia.Designer/Consolonia.Designer.csproj
- src/Consolonia.Core/Consolonia.Core.csproj
- src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj
- src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj
- src/Consolonia.NUnit/Consolonia.NUnit.csproj
🧰 Additional context used
📓 Learnings (1)
src/Consolonia.Designer/README.md (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/Consolonia.Designer.csproj:14-15
Timestamp: 2024-11-19T07:19:33.261Z
Learning: In the Consolonia project, circular dependencies between `Consolonia.Designer` and `Consolonia.PreviewHost` are intentional and by design. The `ConsolePreview` control consumes `Consolonia.PreviewHost`, and `Avalonia.PreviewHost` consumes `ConsolePreview` control in the designer.
🪛 LanguageTool
src/Consolonia.Themes.TurboVision/README.md
[uncategorized] ~16-~16: Possible missing preposition found.
Context: ...me** - TurboVision Black theme # Usage Define an application with a theme ## Define...
(AI_HYDRA_LEO_MISSING_TO)
src/Consolonia.PlatformSupport/README.md
[grammar] ~2-~2: The operating system from Apple is written “macOS”.
Context: ...pting to consoles on Windows, Linux and MacOS environments. ## Background Consolonia...
(MAC_OS)
src/Consolonia.NUnit/README.md
[uncategorized] ~13-~13: A comma might be missing here.
Context: ...ate a unit test against your consolonia application your test class should derive from Cons...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~15-~15: You might be missing the article “a” here.
Context: ...ine the size of your console by passing size parameter to base class. Then you can ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~15-~15: You might be missing the article “the” here.
Context: ...ur console by passing size parameter to base class. Then you can use UITest to inte...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/Consolonia.Core/README.md
[uncategorized] ~12-~12: Possible missing preposition found.
Context: ...(https://youtu.be/ttgZmbruk3Y) # Usage Define an application with a theme (See Consol...
(AI_HYDRA_LEO_MISSING_TO)
🔇 Additional comments (19)
src/Tests/Consolonia.Core.Tests/MiscTests.cs (2)
15-23: LGTM: Exception property validation
The test correctly validates that the Request property of the exception maintains the reference to the original request object.
12-24: Consider adding serialization tests
Given that the [Serializable] attribute was removed from ConsoloniaNotSupportedException, we should verify if this change was intentional and if we need additional tests to validate the new behavior.
src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj (3)
17-23: LGTM! Well-documented build configuration.
The PropertyGroup configuration correctly sets up the custom target for package building, and the included documentation link is helpful for future maintenance.
12-15: Verify the existence of package assets.
The package asset configuration looks good, but let's ensure the referenced files exist.
✅ Verification successful
Package assets are correctly configured and present
Both Icon.png and README.md files exist at their specified locations, confirming that the package asset configuration is valid.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Icon.png exists in repository root
if [ -f "Icon.png" ]; then
echo "✓ Icon.png exists"
else
echo "✗ Icon.png missing"
fi
# Check if README.md exists in package directory
if [ -f "src/Consolonia.PlatformSupport/README.md" ]; then
echo "✓ README.md exists"
else
echo "✗ README.md missing"
fi
Length of output: 167
6-9: LGTM! Project reference configuration looks correct.
The configuration properly embeds GuiCS.dll while preventing it from becoming a package dependency, which aligns with the consolidation objective.
Let's verify no other GuiCS references exist:
✅ Verification successful
Project reference configuration is correctly isolated to PlatformSupport
The verification confirms that Consolonia.GuiCS is only referenced in Consolonia.PlatformSupport.csproj with the correct private assets configuration. No other project references exist that would need similar treatment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other GuiCS package references that might need similar treatment
rg -g "*.csproj" "Consolonia\.GuiCS" --type xml
Length of output: 496
src/Consolonia.Themes.TurboVision/Themes/TurboVisionTheme.cs (1)
5-5: Verify namespace change impact across the codebase
The class has been moved from Consolonia.Themes.TurboVision.Templates to Consolonia.Themes. While this change improves organization, we should verify that all references to this class have been updated.
✅ Verification successful
No impact from namespace change - references are properly scoped
The namespace change from Consolonia.Themes.TurboVision.Templates to Consolonia.Themes appears safe because:
- All references to the old namespace are only in XAML files and are specifically for the
Controls.Helpersnamespace, not forTurboVisionThemeclass - All usages of
TurboVisionThemefound in the codebase are either:- Documentation references in README.md
- Commented out code in App.cs files
- The class definition itself
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for any remaining references to the old namespace
# and verify all usages of TurboVisionTheme have been updated
# Check for old namespace references
echo "Checking for old namespace references..."
rg "Consolonia\.Themes\.TurboVision\.Templates"
# Check all usages of TurboVisionTheme
echo "Checking TurboVisionTheme usages..."
rg "TurboVisionTheme" -A 2 -B 2
Length of output: 9336
src/Consolonia.Designer/Extensions.cs (1)
22-26: Verify performance impact of unconditional design mode checks
The removal of DEBUG conditional compilation means Design.IsDesignMode is now checked in all builds, including production. While this simplifies the code, we should verify that this doesn't impact production performance.
src/Example/Program.cs (2)
9-9: LGTM: Visibility change aligns with package goals
The change from internal to public and updated method signature align well with the PR's objective of preparing for package publishing.
Also applies to: 11-11
13-14: Verify error handling in console lifetime
While the configuration looks correct, ensure proper error handling exists when starting with console lifetime.
✅ Verification successful
Let me check the implementation of error handling in the console lifetime by looking at the core implementation.
Let me try a different approach to check the implementation.
Let me check one more thing about the ConsoloniaLifetime implementation since it's the actual class handling the lifetime.
Based on the implementation review, I can now provide the final response:
Error handling is properly implemented in the console lifetime
The console lifetime implementation inherits from ClassicDesktopStyleApplicationLifetime and includes proper error propagation:
- The
StartWithConsoleLifetimemethod returns an integer status code fromlifetime.Start() - The
ConsoloniaLifetimeclass includes proper cancellation token handling and task-based asynchronous operations - Error conditions are properly propagated through the standard Avalonia lifetime mechanisms
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error handling patterns in console lifetime implementation
rg -A 5 "StartWithConsoleLifetime"
Length of output: 2189
Script:
#!/bin/bash
# Check the implementation of CreateLifetime and related error handling
ast-grep --pattern 'CreateLifetime($_) { $$$ }'
# Also check if there's any try-catch blocks around StartWithConsoleLifetime
ast-grep --pattern 'try { $$$ StartWithConsoleLifetime($$$) $$$ } catch'
# Look for error handling in lifetime.Start implementation
ast-grep --pattern 'Start($_) { $$$ }'
Length of output: 165
Script:
#!/bin/bash
# Check the content of ApplicationStartup.cs which contains the StartWithConsoleLifetime implementation
cat src/Consolonia.Core/ApplicationStartup.cs
# Also check if there are any error handling patterns in the codebase
rg -A 3 "try\s*{.*StartWithConsoleLifetime"
Length of output: 3096
Script:
#!/bin/bash
# Check the implementation of ConsoloniaLifetime
rg -l "class ConsoloniaLifetime"
cat $(rg -l "class ConsoloniaLifetime")
Length of output: 1416
src/Consolonia.Themes.TurboVision/Themes/FluentTheme.cs (2)
5-5: LGTM: Namespace simplification
The namespace simplification from Consolonia.Themes.TurboVision.Themes.Fluent to Consolonia.Themes improves organization and usability.
10-10: Verify resource path resolution
The hardcoded URI path assumes a specific package structure. Ensure this works correctly across different deployment scenarios and package versions.
Also applies to: 21-21
✅ Verification successful
Let me verify if the resource files exist at the specified paths.
Resource path resolution is correctly configured
The avares:// URI scheme is consistently used across all theme implementations, and the resource files exist at their expected locations. The Fluent.axaml file is present at the specified path src/Consolonia.Themes.TurboVision/Themes/Fluent/, and the project follows a consistent structure for all themes (Fluent, Material, TurboVision variants). Additionally, the theme class provides an overloaded constructor that accepts a custom URI, allowing for deployment flexibility if needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for resource handling and path resolution patterns
rg -A 5 "avares://"
Length of output: 9983
Script:
#!/bin/bash
# Check if the Fluent theme files exist
fd -t f "Fluent.axaml" src/Consolonia.Themes.TurboVision/Themes/Fluent/
# List all .axaml files in the themes directory to verify structure
fd -t f ".axaml$" src/Consolonia.Themes.TurboVision/Themes/
Length of output: 1034
src/Consolonia.Themes.TurboVision/Themes/MaterialTheme.cs (1)
5-5: Verify impact of namespace change
The namespace has been simplified from Consolonia.Themes.TurboVision.Themes.Material to Consolonia.Themes. While this improves organization, it could break existing consumers.
✅ Verification successful
Namespace change appears safe to proceed
The namespace simplification from Consolonia.Themes.TurboVision.Themes.Material to Consolonia.Themes appears safe as:
- All current usages of
MaterialThemeare through direct instantiation (new MaterialTheme()) - No explicit namespace references or qualified names were found in the codebase
- The file exists in the default branch and the change only affects its internal organization
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing references to the old namespace
rg "Consolonia\.Themes\.TurboVision\.Themes\.Material"
Length of output: 56
Script:
#!/bin/bash
# Let's try a broader search to find any references to MaterialTheme class
rg "MaterialTheme" -l
# Also check git history for the file to see if this is a new file or modified
git log --oneline -- src/Consolonia.Themes.TurboVision/Themes/MaterialTheme.cs
# Search for any using statements or type references that might be affected
rg "using.*Consolonia\.Themes\.TurboVision\.Themes"
rg "using.*Consolonia\.Themes\.TurboVision"
Length of output: 342
Script:
#!/bin/bash
# Let's check the actual content of files that reference MaterialTheme to see how they use it
rg "MaterialTheme" -C 2 src/Example/App.cs
rg "MaterialTheme" -C 2 src/Tools/Consolonia.PreviewHost/App.cs
rg "MaterialTheme" -C 2 src/Consolonia.Gallery/App.cs
# Check if this is a new file by looking for its presence in the default branch
git ls-tree -r HEAD | grep MaterialTheme.cs
Length of output: 731
src/Consolonia.Themes.TurboVision/Themes/TurboVisionDarkTheme.cs (1)
9-11: 🛠️ Refactor suggestion
Eliminate URI string duplication and verify resource
Similar to MaterialTheme, the URI string is duplicated. Additionally, let's verify the existence of the theme resource file.
Also applies to: 21-22
✅ Verification successful
Resource file exists, but URI constant pattern needs implementation
The theme resource file TurboVisionDark.axaml exists at the correct location. However, looking at both MaterialTheme and TurboVisionDarkTheme, neither currently implements a constant pattern for URIs. Suggest implementing a constant for both themes:
private const string ThemeUri = "avares://Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDark.axaml";
public TurboVisionDarkTheme() : base(new Uri(ThemeUri))
{
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the existence of the theme resource file
fd -e axaml "TurboVisionDark.axaml$"
# Suggest using a constant for the URI
echo "Apply the same URI constant pattern as suggested for MaterialTheme"
Length of output: 260
Script:
#!/bin/bash
# Check if MaterialTheme has a constant pattern for URI
rg -A 5 "class MaterialTheme"
Length of output: 579
src/Consolonia.Themes.TurboVision/Themes/TurboVisionBlackTheme.cs (1)
5-5: Document theme usage in README
Since this PR focuses on package publishing, ensure the README includes clear documentation about available themes and their usage.
src/Consolonia.NUnit/README.md (1)
1-41: Consider creating a central README with component-specific extensions
The identical README content across multiple components could lead to maintenance challenges. Consider:
- Creating a central README in the repository root with common information
- Having component-specific READMEs that focus on their unique features
🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: A comma might be missing here.
Context: ...ate a unit test against your consolonia application your test class should derive from Cons...
(AI_EN_LECTOR_MISSING_PUNCTUATION_COMMA)
[uncategorized] ~15-~15: You might be missing the article “a” here.
Context: ...ine the size of your console by passing size parameter to base class. Then you can ...
(AI_EN_LECTOR_MISSING_DETERMINER_A)
[uncategorized] ~15-~15: You might be missing the article “the” here.
Context: ...ur console by passing size parameter to base class. Then you can use UITest to inte...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
src/Consolonia.Gallery/App.cs (2)
6-6: Import statement necessary for theme usage
The addition of using Consolonia.Themes; is required to access the MaterialTheme class utilized in the application.
10-10: Ensure correct initialization with generic application class
By inheriting from ConsoloniaApplication<ControlsListView>, the application delegates the main window initialization to the generic base class. Verify that ControlsListView is the intended main window for the application and that all necessary initialization logic is appropriately managed by the base class.
src/Tests/Consolonia.Gallery.Tests/Base/GalleryTestsBaseBase.cs (1)
12-12: Confirm intention to make GalleryTestsBaseBase non-abstract
The GalleryTestsBaseBase class is now declared as internal instead of internal abstract, allowing it to be instantiated. If the class is not meant to be instantiated directly and is intended to serve solely as a base class for other tests, consider keeping it abstract to prevent direct instantiation.
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (1)
1-1: Necessary imports added for new functionality
The addition of using System; and using Avalonia.Controls.ApplicationLifetimes; is required for implementing the generic application class and managing application lifetimes.
Also applies to: 4-4
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/Consolonia.Gallery/View/ControlsListView.axaml.cs (1)
95-123: Refactor theme change event handlers to reduce code duplicationThe methods
MaterialTheme_Click,FluentTheme_Click,TurboVisionTheme_Click, etc., contain duplicated code patterns. Consider refactoring these methods into a single method to improve maintainability and reduce redundancy.For example, you could create a method that accepts the selected theme as a parameter:
private void ChangeTheme(Themes theme, Styles style) { Model.SelectedTheme = theme; this.Styles[0] = style; }And update the event handlers:
private void MaterialTheme_Click(object sender, Avalonia.Interactivity.RoutedEventArgs e) { ChangeTheme(Themes.Material, new MaterialTheme()); } // Repeat for other themes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
src/Consolonia.Gallery/App.cs(1 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml.cs(4 hunks)src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj(1 hunks)src/Example/App.cs(1 hunks)src/Tools/Consolonia.PreviewHost/App.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Example/App.cs
- src/Consolonia.Themes.TurboVision/Consolonia.Themes.TurboVision.csproj
🔇 Additional comments (4)
src/Consolonia.Gallery/App.cs (1)
10-10: Ensure application initialization is consistent with new inheritance
The App class now inherits from ConsoloniaApplication<ControlsListView>, and the OnFrameworkInitializationCompleted method has been removed. Verify that the application initializes correctly and that the main window is properly set to ControlsListView as expected.
src/Consolonia.Gallery/View/ControlsListView.axaml (1)
13-24: LGTM!
The addition of the menu for theme selection is well-implemented, and the bindings correspond correctly with the ViewModel properties.
src/Tools/Consolonia.PreviewHost/App.cs (1)
6-6: Verify application components initialize correctly without InitializeComponent()
The InitializeComponent() method, which loads XAML components, has been removed from the constructor. Ensure that all necessary components, styles, and resources are properly initialized when the application starts, and that there are no runtime issues due to this change.
Also applies to: 20-20
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryMenu.axaml (1)
40-42: Verify the use of IsChecked with ToggleType="None"
The MenuItem with ToggleType="None" has IsChecked="True". Since ToggleType="None" indicates that the menu item is neither checkable nor radio, the IsChecked property may have no effect. Confirm that this is intended and that the menu item behaves as expected.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…into tomlm/packages
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
src/Consolonia.Core/README.md (3)
1-8: Consider adding version and prerequisites information.The introduction provides a good overview, but consider adding:
- Supported .NET versions
- Prerequisites for development
- Link to full documentation
9-10: Consider using relative path for the showcase image.The image URL points to a user-content URL which might not be permanent. Consider:
- Including the image in the repository
- Using a relative path to reference it
59-61: Consider expanding the requirements section.The notes section could be more prominent and detailed:
- Move it higher in the document, possibly after the Background section
- Add version requirements for each package
- Explain why each package is needed
- Add links to the package repositories or documentation
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (1)
65-75: Consider documenting migration path for existing applicationsThis new generic application class introduces a structured way to initialize windows, which is good. However, since this is a core infrastructure change, consider:
- Documenting the migration path for existing applications
- Adding XML documentation comments to explain the purpose and usage of this class
- Creating examples of proper usage in the README
src/Consolonia.Gallery/View/ControlsListView.axaml.cs (1)
Line range hint
1-1: Enable nullable reference type annotationsThe code uses nullable reference type annotations but the nullable context is not enabled.
Add the following directive at the top of the file:
+ #nullable enable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Core/README.md(1 hunks)src/Consolonia.Designer/Consolonia.Designer.csproj(2 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml(1 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml.cs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Gallery/View/ControlsListView.axaml
- src/Consolonia.Designer/Consolonia.Designer.csproj
🧰 Additional context used
🪛 GitHub Check: build
src/Consolonia.Gallery/View/ControlsListView.axaml.cs
[warning] 95-95:
The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
[warning] 95-95:
The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
[warning] 95-95:
The annotation for nullable reference types should only be used in code within a '#nullable' annotations context.
🔇 Additional comments (4)
src/Consolonia.Core/README.md (1)
17-23: LGTM! The XAML syntax is correct.
The window definition example is clear and follows proper XAML syntax.
src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs (2)
4-4: LGTM: Required using directive added
The added using directive is necessary for the IClassicDesktopStyleApplicationLifetime type and is correctly placed.
65-66: LGTM: Constraints implemented as suggested
The new() constraint has been correctly added alongside the Window constraint, ensuring type safety at compile-time.
src/Consolonia.Gallery/View/ControlsListView.axaml.cs (1)
7-24: LGTM: Clean enum definition with proper imports
The imports and enum definition are well-organized and follow C# naming conventions.
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
|
|
||
| - name: Setup .NET |
There was a problem hiding this comment.
Can we extract repeated things to somewhere? Would be nice to re-use same things in different workflows
There was a problem hiding this comment.
I have no idea. I created these a long time ago and I just keep copy-pasta'ing them everywhere because they work! :)
There was a problem hiding this comment.
can we do this for a later checkin? I want to work more in themes area and this delta renamed everything...
|
Just small clarification. Directory.Build.props is automatically included to all projects. Directory.Core.Build.props must be included manually to projects where needed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
Directory.Build.props (1)
Line range hint
3-3: Consider addressing the mono TODO before publishingThere's an existing TODO comment about adding mono support. Since this PR is preparing for package publishing, it might be worth addressing cross-platform support requirements first.
Would you like me to help create an issue to track the mono support requirement?
src/Consolonia.Themes/Fluent/FluentTheme.axaml (1)
5-7: Consider removing empty Design.PreviewWith section.The empty
Design.PreviewWithsection doesn't serve any purpose and could be removed to improve code cleanliness.- <Design.PreviewWith> - - </Design.PreviewWith>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (19)
Directory.Build.props(1 hunks)src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs(2 hunks)src/Consolonia.Designer/Consolonia.Designer.csproj(2 hunks)src/Consolonia.Gallery/Program.cs(0 hunks)src/Consolonia.Gallery/View/ControlsListView.axaml.cs(4 hunks)src/Consolonia.NUnit/Consolonia.NUnit.csproj(1 hunks)src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj(1 hunks)src/Consolonia.Themes/Fluent/FluentTheme.axaml(1 hunks)src/Consolonia.Themes/FluentTheme.cs(1 hunks)src/Consolonia.Themes/Material/MaterialTheme.axaml(1 hunks)src/Consolonia.Themes/MaterialTheme.cs(1 hunks)src/Consolonia.Themes/TurboVisionBlack/Controls/Button.axaml(1 hunks)src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml(1 hunks)src/Consolonia.Themes/TurboVisionBlackTheme.cs(1 hunks)src/Consolonia.Themes/TurboVisionDark/TurboVisionDarkTheme.axaml(1 hunks)src/Consolonia.Themes/TurboVisionDarkTheme.cs(1 hunks)src/Consolonia.Themes/TurboVisionTheme.cs(1 hunks)src/Consolonia/Consolonia.csproj(1 hunks)src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj(1 hunks)
💤 Files with no reviewable changes (1)
- src/Consolonia.Gallery/Program.cs
✅ Files skipped from review due to trivial changes (3)
- src/Consolonia.Themes/FluentTheme.cs
- src/Consolonia.Themes/MaterialTheme.cs
- src/Consolonia.Themes/TurboVisionBlackTheme.cs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/Consolonia.NUnit/Consolonia.NUnit.csproj
- src/Consolonia.Core/Infrastructure/ConsoloniaApplication.cs
- src/Consolonia/Consolonia.csproj
- src/Consolonia.Designer/Consolonia.Designer.csproj
- src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj
- src/Consolonia.PlatformSupport/Consolonia.PlatformSupport.csproj
- src/Consolonia.Gallery/View/ControlsListView.axaml.cs
🧰 Additional context used
📓 Learnings (3)
src/Consolonia.Themes/TurboVisionDarkTheme.cs (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-11-12T04:11:38.190Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
src/Consolonia.Themes/TurboVisionDark/TurboVisionDarkTheme.axaml (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-11-12T04:11:38.190Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml (1)
Learnt from: tomlm
PR: jinek/Consolonia#122
File: src/Consolonia.Themes.TurboVision/Themes/TurboVisionDark/TurboVisionDarkColors.axaml:5-28
Timestamp: 2024-11-12T04:11:38.190Z
Learning: In the project, using `Mode="Shaded"` in `ConsoleBrush` within theme files is intentional and does not cause runtime issues.
🔇 Additional comments (13)
src/Consolonia.Themes/TurboVisionBlack/Controls/Button.axaml (2)
3-3: LGTM: Namespace consolidation aligns with PR objectives.
The namespace change from Consolonia.Themes.TurboVision.Templates.Controls.Helpers to Consolonia.Themes.Templates.Controls.Helpers reflects the intended consolidation of components mentioned in the PR objectives.
3-3: Verify assembly reference updates across the codebase.
The assembly reference has changed from Consolonia.Themes.TurboVision to Consolonia.Themes. Let's verify that all related references have been updated consistently.
✅ Verification successful
Based on the analysis of the XAML files, I can now generate the final response:
Assembly reference update is consistent across the codebase
The change from Consolonia.Themes.TurboVision to Consolonia.Themes is consistent. All helper namespace references in the XAML files are correctly using clr-namespace:Consolonia.Themes.Templates.Controls.Helpers;assembly=Consolonia.Themes. No instances of the old assembly reference were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining references to the old assembly name
# and verify the new assembly reference is used consistently.
echo "Checking for old assembly references..."
rg -l "assembly=Consolonia\.Themes\.TurboVision" --type xml
echo "Verifying new assembly references..."
rg -l "assembly=Consolonia\.Themes" --type xml
echo "Checking for any potential missed helper namespace references..."
rg -l "Consolonia\.Themes\.TurboVision\.Templates\.Controls\.Helpers" --type xml
Length of output: 951
Script:
#!/bin/bash
# Let's first find all XAML files in the repository
echo "Finding all XAML files..."
fd -e axaml -e xaml
# Then let's check for assembly references in these files
echo -e "\nChecking assembly references in XAML files..."
fd -e axaml -e xaml -x cat {} \; -x echo -e "\n---\n"
Length of output: 67376
Directory.Build.props (2)
12-12: Verify Icon.png existence and format
The PackageIcon property has been added, which aligns with the PR objectives.
Let's verify the icon file:
✅ Verification successful
Icon files are present and properly tracked
The verification confirms that:
- Both Icon.png (10KB) and Icon.ico (107KB) exist in the root directory
- Both files are properly tracked in git
- The files have appropriate read permissions
- The Icon.png referenced in PackageIcon property is present at the correct location
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify icon file existence and format
# Check if Icon.png exists in the expected location
if [ -f "Icon.png" ]; then
file Icon.png
echo "Icon exists in root"
fi
# Check for any other icon files that might conflict
fd -e png -e ico --type f icon -i
Length of output: 235
Script:
#!/bin/bash
# Let's verify the size and basic properties of the icon files
ls -l Icon.png Icon.ico
# Check if the PNG file is a valid image by looking at its header bytes
hexdump -n 8 -C Icon.png
# Check if these icons are tracked in git
git ls-files Icon.png Icon.ico
Length of output: 321
8-8: Verify version bump impact
The version change to 11.2.1 aligns with Avalonia's version, which is good. However, this appears to be a significant version bump from 11.0.9.
Let's verify the changelog and dependency changes:
Also applies to: 16-16
✅ Verification successful
Version update is aligned with dependencies
The version bump to 11.2.1 is consistent throughout the codebase:
- The
VersionPrefixandAvaloniaVersionare both set to 11.2.1 in Directory.Build.props - All project files reference Avalonia packages using the
$(AvaloniaVersion)variable - The version change ensures alignment with the Avalonia framework version
This is a coordinated version update that maintains consistency across the solution and its Avalonia dependencies. No breaking changes or version mismatches were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in version history and dependency updates
# Check git history for breaking changes
git log --oneline | grep -i "break"
# Look for version references in solution
rg "11\.0\.9|11\.2\.1" --type xml
Length of output: 184
Script:
#!/bin/bash
# Let's try a more comprehensive search for version-related changes
# Search for any version references in XML files
fd -e props -e csproj | xargs cat
# Check recent commits for version-related changes
git log -n 20 --oneline
# Look for CHANGELOG or similar files
fd -i changelog
Length of output: 13439
src/Consolonia.Themes/TurboVisionTheme.cs (1)
5-5: Verify namespace change impact across the codebase
The namespace has been simplified from Consolonia.Themes.TurboVision.Templates to Consolonia.Themes. While this aligns with the PR's consolidation goals, we should verify all references to this theme are updated.
✅ Verification successful
Namespace change is consistent with other themes and properly referenced
The namespace simplification from Consolonia.Themes.TurboVision.Templates to Consolonia.Themes is correct as:
- All other theme implementations (FluentTheme, MaterialTheme, etc.) are in the same
Consolonia.Themesnamespace - All references to
TurboVisionThemein the codebase are either commented out or using the correct namespace - The theme is properly referenced in
ControlsListView.axaml.csand template files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old namespace
rg "Consolonia\.Themes\.TurboVision\.Templates"
# Search for references to TurboVisionTheme to ensure they're using the new namespace
rg "using.*Consolonia\.Themes.*TurboVisionTheme"
Length of output: 100
Script:
#!/bin/bash
# Let's try a broader search to find any references to TurboVisionTheme
rg "TurboVisionTheme" -A 2
# Also search for any files that might be using this theme
fd ".*\.cs$" --exec grep -l "TurboVisionTheme"
# Check if there are any other theme implementations to understand the namespace pattern
fd "Theme\.cs$"
Length of output: 2616
src/Consolonia.Themes/TurboVisionDarkTheme.cs (1)
5-5: LGTM! Namespace change aligns with project restructuring
The namespace simplification is consistent with the changes in TurboVisionTheme.cs and aligns with the PR's goal of consolidating packages.
src/Consolonia.Themes/TurboVisionBlack/TurboVisionBlackTheme.axaml (2)
3-3: LGTM! Class declaration update aligns with namespace restructuring
The simplified class declaration is consistent with the namespace consolidation effort across the theme files.
7-7: Verify resource paths after prefix removal
The paths have been simplified by removing the redundant Themes/TurboVisionBlack prefix. Let's verify these resources exist at the new locations.
Also applies to: 12-12
✅ Verification successful
Resource paths are correctly referenced after prefix removal
The verification confirms that:
TurboVisionBlackColors.axamlexists at the correct location insrc/Consolonia.Themes/TurboVisionBlack/- No other theme files are using the old path pattern with
/Themes/TurboVisionBlack/prefix
The simplified paths in the resource includes are valid and consistent with the actual file structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced resource files exist
fd -e axaml "TurboVisionBlackColors.axaml|Button.axaml" src/Consolonia.Themes/TurboVisionBlack
# Verify no other theme files still use the old path pattern
rg "/Themes/TurboVisionBlack/" -g "*.axaml"
Length of output: 269
src/Consolonia.Themes/Material/MaterialTheme.axaml (2)
7-7: Verify resource resolution for simplified paths.
Resource paths have been simplified:
/Material/MaterialColors.axaml/TurboVisionBlack/Controls/Button.axaml
While this improves maintainability, we should verify the resource resolution still works correctly.
Also applies to: 13-13
✅ Verification successful
Let me verify the resource resolution by checking the actual content and references in these files.
Resource paths are correctly resolved and functional
The simplified paths are working correctly as:
/Material/MaterialColors.axamlresolves to the file in the same directory, which exists and contains valid resource definitions/TurboVisionBlack/Controls/Button.axamlexists at the correct location with valid style definitions
Both files are properly structured XAML resources that can be included in the theme. The simplified paths are relative to the themes root and maintain correct resolution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the referenced files exist at the new locations
fd -e axaml "MaterialColors.axaml|Button.axaml" --full-path
Length of output: 667
Script:
#!/bin/bash
# Check the content of MaterialTheme.axaml to see all resource includes
cat "src/Consolonia.Themes/Material/MaterialTheme.axaml"
# Check if MaterialColors.axaml exists in the same directory
cat "src/Consolonia.Themes/Material/MaterialColors.axaml"
# Check if Button.axaml exists in TurboVisionBlack
cat "src/Consolonia.Themes/TurboVisionBlack/Controls/Button.axaml"
Length of output: 3405
3-3: Verify namespace change impact across the codebase.
The namespace has been simplified from Consolonia.Themes.TurboVision.Themes.Material.MaterialTheme to Consolonia.Themes.MaterialTheme. While this improves clarity, we should ensure all references are updated.
✅ Verification successful
Namespace change is correctly implemented across the codebase
The namespace simplification from Consolonia.Themes.TurboVision.Themes.Material.MaterialTheme to Consolonia.Themes.MaterialTheme is properly reflected throughout the codebase. All references to MaterialTheme use the new simplified namespace, and there are no lingering references to the old namespace structure. The change appears to be part of a clean namespace reorganization.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old namespace
rg "Consolonia\.Themes\.TurboVision\.Themes\.Material\.MaterialTheme" -t cs -t xaml
Length of output: 114
Script:
#!/bin/bash
# Search for any remaining references to the old namespace pattern, without file type restrictions
rg "Consolonia\.Themes\.TurboVision\.Themes\.Material\.MaterialTheme"
# Also search for references to the new namespace to verify the change
rg "Consolonia\.Themes\.MaterialTheme"
# List all .axaml files to understand the theme structure
fd "\.axaml$"
Length of output: 5581
Script:
#!/bin/bash
# Search for any references to MaterialTheme in C# files
rg "MaterialTheme" -t cs
# Check for any theme-related using statements or references in C# files
rg "using.*Consolonia\.Themes" -t cs
Length of output: 928
src/Consolonia.Themes/Fluent/FluentTheme.axaml (1)
3-3: LGTM! Consistent with MaterialTheme changes.
The namespace simplification and path modifications follow the same pattern as MaterialTheme.axaml, maintaining consistency across theme files.
Also applies to: 10-10, 16-16
src/Consolonia.Themes/TurboVisionDark/TurboVisionDarkTheme.axaml (2)
13-14: LGTM! Consistent path structure for control styles.
The paths for Button.axaml and DialogWindow.axaml follow the established pattern, maintaining consistency with other theme files.
7-7: Verify the use of MergeResourceInclude vs ResourceInclude.
This file uses MergeResourceInclude while other theme files use ResourceInclude. Please verify if this difference is intentional and necessary.
✅ Verification successful
Based on the output, I can now generate the final response as the verification is complete. The shell script results show that MergeResourceInclude is consistently used across different theme files when including color resources within ResourceDictionary.MergedDictionaries. Here are the relevant examples from the output:
- TurboVisionBlackTheme.axaml:
<MergeResourceInclude Source="/TurboVisionBlack/TurboVisionBlackColors.axaml" /> - TurboVisionDarkTheme.axaml:
<MergeResourceInclude Source="/TurboVisionDark/TurboVisionDarkColors.axaml" /> - TurboVisionTheme.axaml:
<MergeResourceInclude Source="/Templates/TurboVisionDefaultColors.axaml" />
The use of MergeResourceInclude is correct and consistent with other theme files.
The codebase shows a clear pattern where MergeResourceInclude is used specifically for merging color resources within ResourceDictionary.MergedDictionaries, while StyleInclude (not ResourceInclude) is used for including control styles.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage patterns of different resource include types
rg -A 1 "<(Resource|MergeResource)Include" -t xaml
Length of output: 81
Script:
#!/bin/bash
# Search for files with .axaml extension and then look for resource include patterns
fd -e axaml -x rg -l "(Resource|MergeResource)Include"
# Then get the content of these files to analyze the usage
fd -e axaml -x cat {} \;
Length of output: 67838
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml (1)
17-17: Consider using theme resources instead of hard-coded colorsWhile the high-contrast combination of dark blue and yellow is visible, consider:
- Using theme resources to maintain consistency and support theme switching:
-<Button Background="DarkBlue" Foreground="Yellow">Custom Colors</Button> +<Button Background="{DynamicResource SystemAccentColorDark1}" + Foreground="{DynamicResource SystemAccentColorLight1}">Custom Colors</Button>
- Ensuring the color contrast meets accessibility standards for all users.
src/Consolonia/Assembly.cs (1)
6-6: Address CLS compliance decision before publishingGiven that this PR prepares the packages for publishing to NuGet, CLS compliance is an important consideration. CLS compliance ensures your library can be consumed from any .NET language, but it comes with some restrictions:
- No unsigned types (byte, ushort, uint, ulong)
- No unsafe code or pointers
- No overloaded operators
- No non-CLS compliant types in public APIs
Consider the following factors for your decision:
- If your library needs to be consumed from other .NET languages (F#, VB.NET), enable CLS compliance
- If you use unsigned types or operator overloading in public APIs, you may need to disable it
- If you're unsure, run a static analyzer to check for CLS compliance violations before making the decision
Would you like me to help analyze the codebase for potential CLS compliance violations?
src/Consolonia.Themes/Fluent/FluentColors.axaml (1)
13-18: Consider consolidating duplicate Communication Blue definitionsBoth ThemeChooserBackgroundBrush and ThemeActionBackgroundBrush use the same Communication Blue color (#0078D4). Consider creating a base color resource that both can reference to improve maintainability.
Example refactor:
+ <!-- Fluent: Communication Blue Base --> + <drawing:ConsoleBrush x:Key="ThemeCommunicationBlueBrush" Color="#0078D4" /> - <!-- Fluent: Communication Blue --> <drawing:ConsoleBrush x:Key="ThemeChooserBackgroundBrush" - Color="#0078D4" /> + Color="{StaticResource ThemeCommunicationBlueBrush}" /> - <!-- Fluent: Communication Blue --> <drawing:ConsoleBrush x:Key="ThemeActionBackgroundBrush" - Color="#0078D4" /> + Color="{StaticResource ThemeCommunicationBlueBrush}" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryButton.axaml(1 hunks)src/Consolonia.Themes/Fluent/FluentColors.axaml(1 hunks)src/Consolonia.Themes/Fluent/FluentTheme.axaml(1 hunks)src/Consolonia/Assembly.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Consolonia.Themes/Fluent/FluentTheme.axaml
🔇 Additional comments (3)
src/Consolonia/Assembly.cs (1)
1-4: LGTM! Test assembly access is properly configured.
The InternalsVisibleTo attribute is correctly set up to allow test access to internal members.
src/Consolonia.Themes/Fluent/FluentColors.axaml (2)
7-44: Well-structured color scheme implementation
The color definitions follow Fluent Design guidelines with clear documentation and appropriate usage of transparency where needed. The organization of colors by their semantic meaning (e.g., error, warning, borders) makes the theme maintainable and consistent.
3-5: Verify the FontSizeNormal value
A font size value of 1 seems unusually small. Please confirm if this is intentional or if it should be using a different scale.
Let's check how this value is used across the codebase:
✅ Verification successful
Based on the evidence gathered, I can now provide a final response:
FontSizeNormal=1 is the correct value for this console-based UI framework
The value of 1 for FontSizeNormal is intentional and correct. This is a console-based UI framework where text rendering is done using console characters, and the font size of 1 represents the base unit for console character cells. This is evident from:
- The DrawingContextImplTests which shows text being rendered in console character cells with unit-based positioning
- The FontManagerImpl which implements a console-specific font system
- The consistent usage of FontSizeNormal=1 across all theme files (Fluent, Material, TurboVision)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for FontSizeNormal usage to understand the context
rg -l "FontSizeNormal"
# Search for other font size resources to compare scales
ast-grep --pattern 'FontSize[A-Za-z]*'
Length of output: 414
Script:
#!/bin/bash
# Let's check the actual usage of FontSizeNormal in these files
rg "FontSizeNormal" -B2 -A2
# Let's also check if there are any scaling or multiplication of this value
rg "FontSize.*\*"
Length of output: 3651
Script:
#!/bin/bash
# Let's check the Window control template for any font size scaling or transformations
cat "src/Consolonia.Themes/Templates/Controls/Window.axaml"
# Let's also check if there are any font-related utilities or scaling factors
rg -l "Font" --type cs
Length of output: 3335
Script:
#!/bin/bash
# Let's check the font-related implementation in Core to understand the scaling
cat "src/Consolonia.Core/Text/FontManagerImpl.cs"
# Also check if there are any font-related tests that might reveal the expected behavior
cat "src/Tests/Consolonia.Core.Tests/DrawingContextImplTests.cs"
Length of output: 28772
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs (1)
Line range hint
13-24: Consider enhancing test coverage for custom button properties.The test verifies the presence of the "Custom" button but doesn't validate its visual properties (DarkBlue background and Yellow foreground).
Consider adding assertions to verify the button's custom colors:
public async Task PerformSingleTest() { await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Enter); await UITest.AssertHasText("Standard _?XAML Button", "Custom", "Disabled", "Toggle Button", "IsTabStop=False"); + // Verify custom button properties + var customButton = await UITest.FindControl<Button>("CustomButton"); + Assert.That(customButton.Background.ToString(), Is.EqualTo("DarkBlue")); + Assert.That(customButton.Foreground.ToString(), Is.EqualTo("Yellow")); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs(1 hunks)
🔇 Additional comments (1)
src/Tests/Consolonia.Gallery.Tests/ButtonTests.cs (1)
18-18: LGTM! Text assertion updated to match UI changes.
The change correctly reflects the replacement of the "Foreground" button with "Custom" in the UI.
Co-authored-by: <+@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/Tools/Consolonia.PreviewHost/App.cs (1)
Line range hint
89-122: Remove or document the commented-out event handlersThere's a significant block of commented-out code containing window event handlers with important cleanup logic for file watchers. If this code is:
- No longer needed: Remove it entirely
- For future use: Move it to a separate branch or issue
- Still relevant: Uncomment and integrate it properly
This will improve code maintainability and prevent confusion for other developers.
Would you like me to help create an issue to track the proper handling of this code?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
Directory.Build.props(1 hunks)src/Consolonia.Designer/ConsolePreview.cs(2 hunks)src/Consolonia.Designer/Extensions.cs(2 hunks)src/Consolonia.Themes/Fluent/FluentColors.axaml(1 hunks)src/Tools/Consolonia.PreviewHost/App.cs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/Consolonia.Designer/Extensions.cs
- src/Consolonia.Themes/Fluent/FluentColors.axaml
- src/Consolonia.Designer/ConsolePreview.cs
- Directory.Build.props
🔇 Additional comments (2)
src/Tools/Consolonia.PreviewHost/App.cs (2)
6-6: LGTM: Import changes align with the new theming approach
The addition of Consolonia.Themes import and removal of Avalonia.Markup.Xaml aligns well with the transition from XAML-based initialization to programmatic theming.
20-20: Verify the impact of removing XAML-based initialization
The transition to programmatic theming looks good, but let's verify:
- That no XAML-based theme configurations are being used elsewhere
- That MaterialTheme is the intended default theme for all use cases
✅ Verification successful
MaterialTheme is consistently used as the default theme, and XAML initialization is properly maintained
The verification shows that:
- MaterialTheme is consistently used as the default theme across multiple files (src/Tools/Consolonia.PreviewHost/App.cs, src/Consolonia.Gallery/View/ControlsListView.axaml.cs, src/Example/App.cs)
- The removal of InitializeComponent() from App.cs is safe because:
- App.cs typically handles theme initialization separately from XAML
- All UI components (windows, controls) still maintain their InitializeComponent() calls
- The XAML-based theme configurations in .axaml files are properly structured with theme resources and styles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any XAML theme configurations that might be affected
# Look for any .axaml or .xaml files that might contain theme configurations
rg -t xaml "Theme|Style|ResourceDictionary"
# Check for any programmatic theme usage in other files
ast-grep --pattern 'new MaterialTheme()'
Length of output: 493
Script:
#!/bin/bash
# Let's check for .axaml files first since rg didn't recognize xaml extension
fd -e axaml -x cat {} \; | grep -i "Theme\|Style\|ResourceDictionary"
# Also check for any other theme types being used
ast-grep --pattern 'new $_Theme()'
# Let's also check if there are any XAML-related initialization calls
ast-grep --pattern 'InitializeComponent()'
Length of output: 50200
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/Experimental/Consolonia.Designer/Extensions.cs (1)
Line range hint
9-19: Complete the incomplete documentation noteThe XML documentation has an unfinished note about package references:
NOTE: The package references forPlease complete this documentation to provide clear guidance about package dependencies.
src/Experimental/Consolonia.Designer/ConsolePreview.cs (3)
42-44: Consider making the typeface configurable.While "Cascadia Mono" is a good default choice, consider making it configurable to support different environments and user preferences.
- private readonly Typeface _typeface = new("Cascadia Mono"); + private static readonly string DefaultTypeface = "Cascadia Mono"; + public static readonly StyledProperty<string> TypefaceProperty = + AvaloniaProperty.Register<ConsolePreview, string>(nameof(Typeface), DefaultTypeface); + private Typeface _typeface => new(GetValue(TypefaceProperty));
Line range hint
234-267: Address potential race condition in ListenForChanges.The async loop in ListenForChanges might continue after disposal. Consider using a cancellation token to properly handle shutdown.
- private void ListenForChanges() + private CancellationTokenSource? _listenCancellation; + private void ListenForChanges() { + _listenCancellation?.Cancel(); + _listenCancellation = new CancellationTokenSource(); + var token = _listenCancellation.Token; Task.Run(async () => { while (true) { + if (token.IsCancellationRequested) + return; if (_process == null || _process.HasExited) return; string? line = await _process!.StandardOutput.ReadLineAsync().ConfigureAwait(false); if (string.IsNullOrEmpty(line)) continue;Also update the Dispose method:
if (disposing) { - // TODO: dispose managed state (managed objects) + _listenCancellation?.Cancel(); + _listenCancellation?.Dispose(); + _listenCancellation = null;
Line range hint
255-267: Improve error handling in ListenForChanges.The catch block for JsonException should log more details about the error and notify the UI about the failure.
catch (JsonException ex) { - // process was probably shut down, we continue to check the proces. - Debug.WriteLine($"Error deserializing pixel buffer: {ex.Message}"); + Debug.WriteLine($"Error deserializing pixel buffer: {ex}"); + Dispatcher.UIThread.Invoke(() => + Content = new TextBlock + { + Text = $"Preview error: {ex.Message}", + Foreground = Brushes.Red + }); if (_process == null) continue; if (!_process.HasExited) _process.Kill(); _process.Dispose(); _process = null; return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/Experimental/Avalonia.PreviewHost/Assets/consolonia-logo.icois excluded by!**/*.ico
📒 Files selected for processing (7)
src/Consolonia.Gallery/Consolonia.Gallery.csproj(1 hunks)src/Consolonia.sln(4 hunks)src/Experimental/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj(2 hunks)src/Experimental/Consolonia.Designer/ConsolePreview.cs(2 hunks)src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj(2 hunks)src/Experimental/Consolonia.Designer/Extensions.cs(2 hunks)src/Experimental/Consolonia.Designer/README.md(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Experimental/Consolonia.Designer/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/Consolonia.Gallery/Consolonia.Gallery.csproj
- src/Consolonia.sln
🧰 Additional context used
📓 Learnings (1)
src/Experimental/Consolonia.Designer/ConsolePreview.cs (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/Consolonia.Designer.csproj:14-15
Timestamp: 2024-11-19T07:19:33.261Z
Learning: In the Consolonia project, circular dependencies between `Consolonia.Designer` and `Consolonia.PreviewHost` are intentional and by design. The `ConsolePreview` control consumes `Consolonia.PreviewHost`, and `Avalonia.PreviewHost` consumes `ConsolePreview` control in the designer.
🔇 Additional comments (9)
src/Experimental/Consolonia.Designer/Extensions.cs (1)
4-5: Verify namespace change impact
The namespace has been changed from Consolonia.Designer to Consolonia. While this aligns with the package consolidation objective, we should verify all references are updated.
✅ Verification successful
Let me verify if these references are properly handled by checking the changes in the PR.
Let me check if there are any other files in the PR that might be affected by this namespace change.
Let me check the content of the other files in the Designer directory to verify if they are also being updated with the new namespace.
Based on the analysis, I can now provide the final response:
Namespace change is properly managed
The namespace change from Consolonia.Designer to Consolonia appears in Extensions.cs with a deliberate // ReSharper disable CheckNamespace directive. The remaining references to Consolonia.Designer are correctly maintained:
- In
ConsolePreview.cswhere the type is defined - In
App.axaml.cswhich uses theConsolePreviewtype - In
Assembly.csof Core project which declares internal visibility
The namespace change is intentionally limited to the Extensions class to make it more accessible while preserving the original namespace for other components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining references to the old namespace
rg "Consolonia\.Designer" --type cs
Length of output: 286
Script:
#!/bin/bash
# Check the content of ConsolePreview.cs to see if it's also being updated
cat src/Experimental/Consolonia.Designer/ConsolePreview.cs
# Check App.axaml.cs to see how the Designer type is used
cat src/Experimental/Avalonia.PreviewHost/App.axaml.cs
Length of output: 17739
Script:
#!/bin/bash
# Get the list of changed files in the PR
git diff --name-only
# Check if there are any other files in the Designer directory
fd . src/Experimental/Consolonia.Designer/
Length of output: 333
Script:
#!/bin/bash
# Check the content of Assembly.cs
cat src/Experimental/Consolonia.Designer/Assembly.cs
# Check if Extensions.cs has any other content beyond the namespace declaration
cat src/Experimental/Consolonia.Designer/Extensions.cs
Length of output: 1249
src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj (2)
18-19: Verify project reference availability
The project references have been updated to use ../../ paths. Ensure these projects exist at the new locations:
../../Consolonia.Core/Consolonia.Core.csproj../../Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj
✅ Verification successful
Project references are correctly configured
Both referenced projects exist at the expected locations relative to the repository root:
src/Consolonia.Core/Consolonia.Core.csprojsrc/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj
The relative paths ../../ in the project references are correct since the file under review is at src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if referenced projects exist
fd -p "Consolonia\.Core\.csproj|Consolonia\.PreviewHost\.csproj" --type f
Length of output: 181
3-3: Verify Directory.Core.Build.props import path
The import path uses $(MSBuildThisFileDirectory)../ but project references now use ../../. Ensure this path is still correct with the new project structure.
✅ Verification successful
Import path for Directory.Core.Build.props is correct
The import path $(MSBuildThisFileDirectory)../ is correct because:
- Directory.Core.Build.props exists at the repository root
- Consolonia.Designer.csproj is located at src/Experimental/Consolonia.Designer/
- Going up one level from the csproj location (src/Experimental/Consolonia.Designer/ -> src/Experimental/) and then using MSBuild's GetPathOfFileAbove will correctly locate the props file at the root
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if Directory.Core.Build.props exists at the expected location
fd -p "Directory.Core.Build.props" --type f
Length of output: 70
Script:
#!/bin/bash
# Let's check the location of the csproj file relative to Directory.Core.Build.props
fd -p "Consolonia.Designer.csproj" --type f
Length of output: 107
src/Experimental/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj (3)
8-9: LGTM: Consistent packaging configuration
The IsPackable=false setting is consistent with other experimental components and aligns with the PR's packaging strategy.
29-29: Verify Designer project reference path
The project reference to Consolonia.Designer has been updated. Ensure it's correctly pointing to the new location.
✅ Verification successful
Project reference path is valid and correctly configured
The project reference path ..\Consolonia.Designer\Consolonia.Designer.csproj in Avalonia.PreviewHost.csproj is correctly pointing to an existing project at src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the Designer project exists at the referenced location
if [ -f "src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj" ]; then
echo "Designer project found at expected location"
else
echo "Designer project not found at expected location"
fi
Length of output: 173
Line range hint 16-24: Verify Avalonia package version consistency
Multiple Avalonia packages are referenced. Ensure $(AvaloniaVersion) is defined consistently across the solution.
✅ Verification successful
Avalonia package version consistency verified
The verification shows that:
AvaloniaVersionis properly defined inDirectory.Build.propsas11.2.1- All examined .csproj files consistently use
$(AvaloniaVersion)variable for Avalonia packages - No hardcoded Avalonia package versions were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for AvaloniaVersion property definition
fd "Directory\..*\.props" --type f --exec grep -l "AvaloniaVersion"
Length of output: 91
Script:
#!/bin/bash
# Check the content of Directory.Build.props for AvaloniaVersion
cat Directory.Build.props
# Search for any other potential Avalonia version references in .csproj files
fd "\.csproj$" --type f --exec grep -l "AvaloniaVersion"
# Also check for any hardcoded Avalonia package versions that might not use the variable
rg --type csproj "Include=\"Avalonia[^\"]*\" Version=\"[^$][^\"]+"
Length of output: 1523
Script:
#!/bin/bash
# Check for any hardcoded Avalonia package versions without the variable
rg "Include=\"Avalonia[^\"]*\" Version=\"[^$][^\"]+" --type xml
# Let's also check the content of some of the .csproj files that reference AvaloniaVersion
cat "./src/Experimental/Avalonia.PreviewHost/Avalonia.PreviewHost.csproj"
cat "./src/Consolonia.Core/Consolonia.Core.csproj"
Length of output: 2674
src/Experimental/Consolonia.Designer/ConsolePreview.cs (3)
8-15: LGTM! Imports are well-organized and necessary.
The imports correctly support the control's functionality as a console XAML previewer.
Line range hint 107-124: LGTM! Proper implementation of the dispose pattern.
The dispose pattern is correctly implemented with proper process cleanup.
48-48: Verify process cleanup in all scenarios.
The _process field is now always available, which is good for consistency. However, ensure proper cleanup in all scenarios.
✅ Verification successful
Let me search for more context about process handling and cleanup.
Let me try a different approach to find process cleanup patterns.
Let me check for the complete Dispose pattern implementation since we see _disposedValue field.
Let me try one final search to find any IDisposable implementation or cleanup methods.
Process cleanup is properly implemented
The code implements proper process cleanup through:
- A dispose pattern with both
Dispose()andDispose(bool)methods - Consistent cleanup of
_processby callingKill()andDispose()in all scenarios - Setting
_process = nullafter cleanup to prevent reuse - Using a
_disposedValueflag to prevent multiple disposals
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential process leaks in error handling
rg -A 5 "Process\." src/Experimental/Consolonia.Designer/ConsolePreview.cs
Length of output: 321
Script:
#!/bin/bash
# Check for process cleanup and disposal patterns
ast-grep --pattern 'class ConsolePreview {
$$$
_process$$$
$$$
}'
Length of output: 73
Script:
#!/bin/bash
# Search for dispose or cleanup related code
rg -U "(?s)class ConsolePreview.*?}$" src/Experimental/Consolonia.Designer/ConsolePreview.cs
# Also check for any Kill or Dispose calls
rg "(_process.*?(Kill|Dispose)|Dispose.*?_process)" src/Experimental/Consolonia.Designer/ConsolePreview.cs
Length of output: 1684
Script:
#!/bin/bash
# Search for the Dispose implementation
rg -A 10 "protected override void Dispose" src/Experimental/Consolonia.Designer/ConsolePreview.cs
Length of output: 99
Script:
#!/bin/bash
# Search for any dispose or cleanup methods
rg -A 10 "(Dispose\(|protected.*Dispose|IDisposable)" src/Experimental/Consolonia.Designer/ConsolePreview.cs
# Also check for any finalizer
rg "~ConsolePreview" src/Experimental/Consolonia.Designer/ConsolePreview.cs
Length of output: 1864
…ished or packaged or anything
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj(1 hunks)
🧰 Additional context used
📓 Learnings (1)
src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj (1)
Learnt from: tomlm
PR: jinek/Consolonia#150
File: src/Consolonia.Designer/Consolonia.Designer.csproj:14-15
Timestamp: 2024-11-19T07:19:33.261Z
Learning: In the Consolonia project, circular dependencies between `Consolonia.Designer` and `Consolonia.PreviewHost` are intentional and by design. The `ConsolePreview` control consumes `Consolonia.PreviewHost`, and `Avalonia.PreviewHost` consumes `ConsolePreview` control in the designer.
🔇 Additional comments (3)
src/Experimental/Consolonia.Designer/Consolonia.Designer.csproj (3)
11-14: Project references look good
The circular dependency between Designer and PreviewHost is intentional and by design, as confirmed by previous learnings. The ConsolePreview control and Avalonia.PreviewHost are designed to consume each other.
✅ Verification successful
Project references are correctly configured
The project references in Consolonia.Designer.csproj are valid and point to existing project files. The circular dependency between Designer and PreviewHost is intentional by design, as confirmed by previous learnings.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify project reference paths exist
test -f "src/Consolonia.Core/Consolonia.Core.csproj" && \
test -f "src/Tools/Consolonia.PreviewHost/Consolonia.PreviewHost.csproj" && \
echo "Project references exist" || echo "Missing project references"
Length of output: 184
8-8: Verify Newtonsoft.Json version security
Check if version 13.0.3 of Newtonsoft.Json has any known security vulnerabilities.
✅ Verification successful
Newtonsoft.Json 13.0.3 is secure and free from known vulnerabilities
The current version (13.0.3) is higher than the patched version (13.0.1) that addressed the last known high-severity vulnerability from 2022. No security issues have been reported for versions >= 13.0.1.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories for Newtonsoft.Json
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NUGET, package: "Newtonsoft.Json") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 823
4-8: Verify Avalonia version variable definition
The project uses $(AvaloniaVersion) for Avalonia packages. Ensure this variable is defined in a parent Directory.Build.props or similar file.
✅ Verification successful
$(AvaloniaVersion) is properly defined and matches package versions
The AvaloniaVersion property is correctly defined in Directory.Build.props with value 11.2.1, which aligns with the project's version prefix. The variable usage in package references is valid and follows good practices for version management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for AvaloniaVersion definition in props files
fd -e props | xargs grep -l "AvaloniaVersion"
Length of output: 67
Script:
#!/bin/bash
# Check the content of Directory.Build.props to verify AvaloniaVersion definition
cat Directory.Build.props
Length of output: 936
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
src/Experimental/Avalonia.PreviewHost/README.md (4)
4-8: Enhance installation instructions with additional details.The installation section could be more comprehensive. Consider adding:
- Specify the language in the code block
- Add version information
- Include a verification command
## Installation To install the tool, you can use the following command: -``` +```shell dotnet tool install -g Consolonia.PreviewHost
+To verify the installation:
+shell +Consolonia.PreviewHost --version +<details> <summary>🧰 Tools</summary> <details> <summary>🪛 Markdownlint (0.35.0)</summary> 6-6: null Fenced code blocks should have a language specified (MD040, fenced-code-language) </details> </details> --- `22-26`: **Fix grammar and enhance the note about single file mode.** Add missing articles and improve the note's clarity. ```diff -Run following command to monitor a given .axaml file. +Run the following command to monitor a given .axaml file: -> NOTE: In single file mode the tool will exit when you hit escape key +> NOTE: In single file mode, the tool will exit when you hit the escape key.🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: You might be missing the article “the” here.
Context: ...ath-to-project-file-or-folder> ``` Run following command to monitor a given .axaml file....(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~26-~26: You might be missing the article “the” here.
Context: ...le mode the tool will exit when you hit escape key ### Running inside Visual Studio 1...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
28-39: Standardize IDE integration instructions and add troubleshooting.The IDE integration sections could be more consistent and comprehensive.
### Running inside Visual Studio 1. Open the solution in Visual Studio. 1. Open a Terminal (View -> Terminal). -3. run the following command -``` Consolonia.PreviewHost . ``` +3. Run the following command: +```shell +Consolonia.PreviewHost . +``` ### Running inside Visual Studio Code -To integrate the tool with Visual Studio Code, you can use the following steps: 1. Open the project directory in Visual Studio Code. -2. Open a terminal inside of visual studio code -3. run the following command -``` Consolonia.PreviewHost . ``` +2. Open a Terminal (View -> Terminal). +3. Run the following command: +```shell +Consolonia.PreviewHost . +``` + +### Troubleshooting +If you encounter any issues: +1. Ensure the tool is installed globally +2. Verify the terminal is running from the correct directory +3. Check if the project contains valid AXAML files
1-2: Add an introduction explaining Consolonia and the preview tool's purpose.Consider adding a brief introduction that explains:
- What Consolonia is
- What problem this preview tool solves
- Key features and benefits
# Consolonia.PreviewHost -This is a tool which enables a live view of a consolonia axaml file, or a consolonia project. +Consolonia.PreviewHost is a development tool for the Consolonia framework, a TUI (Text User Interface) implementation of Avalonia UI. This tool provides: + +- Live preview of Consolonia AXAML files as you edit +- Real-time updates for both single files and entire projects +- Seamless integration with popular IDEs +- Instant feedback for UI changes without rebuilding
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/Experimental/Avalonia.PreviewHost/README.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
src/Experimental/Avalonia.PreviewHost/README.md
[uncategorized] ~22-~22: You might be missing the article “the” here.
Context: ...ath-to-project-file-or-folder> ``` Run following command to monitor a given .axaml file....
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~26-~26: You might be missing the article “the” here.
Context: ...le mode the tool will exit when you hit escape key ### Running inside Visual Studio 1...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🪛 Markdownlint (0.35.0)
src/Experimental/Avalonia.PreviewHost/README.md
6-6: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
12-12: null
Fenced code blocks should have a language specified
(MD040, fenced-code-language)
|
|
||
| [assembly: InternalsVisibleTo("Consolonia.Core.Tests")] | ||
|
|
||
| [assembly: CLSCompliant(true)] //todo: should we make it compliant? |
There was a problem hiding this comment.
Btw, why do we need to make it CLS compliant?
This is to implement #169 and #172