Add ability to define linestyle for each edge when drawing a linebrush. #246
Conversation
Example: ```LineStyle="SingleLine DoubleLine SingleLine DoubleLine"``` Will produce single line on left and right side and double line on top bottom. Pattern is same as Thickness definitions: Left Top Right Bottom If you define partial then last definition will apply.
…into tomlm/lineStyles
📝 WalkthroughWalkthroughThis pull request modifies the Possibly related PRs
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 (
|
|
Warning Rate limit exceeded@tomlm has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 28 minutes and 8 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
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 (
|
Co-authored-by: <[email protected]>
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive enhancement to line style handling in the Consolonia framework. The changes primarily revolve around creating a new Possibly related PRs
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: 1
🧹 Nitpick comments (7)
src/Consolonia.Core/Controls/LineStyles.cs (2)
12-15: Consider validating empty or invalid input inParse.
Currently,Parsedirectly calls the constructor that expects valid strings. Adding minimal validation (e.g.,if (string.IsNullOrWhiteSpace(s))) could help avoid cryptic exceptions when receiving incorrect input from external sources.
42-50: Implicit operators enhance ease of use.
These implicit conversions fromstringandLineStyletoLineStylesreduce boilerplate code. Keep an eye out for ambiguous call scenarios when multiple conversions might apply.src/Tests/Consolonia.Core.Tests/WithLifetimeFixture/LineStylesTests.cs (1)
11-19: Robust test ensures single-style strings apply to all edges.
The test coverage for"SingleLine"is good. It might be beneficial to also test an empty string scenario if theLineStyles.Parsemethod will handle it in the future.src/Consolonia.Core/Drawing/DrawingContextImpl.cs (4)
154-178: Stream geometry handling is neatly separated for fills and strokes.
This block usesInferStrokePositionsto decide which line to draw. The approach is clear. Just confirm that shapes with more complex geometry (e.g., non-rectangular shapes) don’t produce unexpected stroke outcomes.
210-214: UsingsceneBrushContentfor rendering is straightforward but lacks error handling.
Consider verifyingsceneBrushContent.Sizeor other properties to ensure the content is valid before rendering.
477-501: Efficient brush-based rectangle filling with fallback brush logic.
The nested loops andExecuteWithClipping(...)calls handle partial coverage. Ensure performance remains acceptable for large rectangles, as each pixel calls a lambda.
722-735: Tab handling is thorough but could be user-configurable.
The constanttabSize = 8might not suit all use cases. You might consider a method parameter or global setting if your app deals with text editors or text-based UIs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Consolonia.Core/Controls/LineBrush.cs(1 hunks)src/Consolonia.Core/Controls/LineStyles.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(9 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml(2 hunks)src/Tests/Consolonia.Core.Tests/WithLifetimeFixture/LineStylesTests.cs(1 hunks)
🔇 Additional comments (14)
src/Consolonia.Core/Controls/LineBrush.cs (2)
16-17: Use ofLineStylesforLineStylePropertyis well-aligned with the new feature.
This update correctly shifts from a singleLineStyleto the more granularLineStyles. Make sure to confirm that any existing bindings in XAML (or code) referencingLineStylePropertyare updated accordingly.
25-25: Property rename fromLineStyletoLineStylesis consistent with the property type change.
The new property type and name match usage patterns in the rest of the codebase. Ensure the property usage is thoroughly tested, especially with partial style definitions (e.g., only specifying two edges).src/Consolonia.Core/Controls/LineStyles.cs (2)
22-32: Parsing flexible tokens is helpful; consider stricter error checks.
The code splits on spaces and commas, then parses each token intoLineStylewithout catching potentialArgumentExceptionfromEnum.Parse. If user input contains invalid tokens, it will throw. You may want to catch or rethrow a more meaningful error.
34-40: Smart fallback logic for partial constructor arguments.
Using default values (??) to progressively assignTop,Right, andBottomis a neat approach. Ensure there's a test scenario that specifically verifies each fallback scenario.src/Tests/Consolonia.Core.Tests/WithLifetimeFixture/LineStylesTests.cs (3)
41-49: Quad-style test covers all four edges.
Great way to confirm the fallback logic doesn’t kick in. The usage of spaces vs. commas is well-tested.
51-56: Exception test ensures robust error handling.
Verifies that too many style tokens result in anArgumentException. Consider testing invalid token text (e.g., "SingleLine,NotAStyle") to confirmEnum.Parseexceptions are also as expected.
150-157: JSON serialization test ensures long-term maintainability.
Serialization coverage is often overlooked, so it’s great to see it tested here. If backward compatibility is needed, you might confirm stable JSON property names or consider a custom contract resolver.src/Consolonia.Core/Drawing/DrawingContextImpl.cs (5)
99-129: Quad-pixel approach for bitmap drawing is well-structured but watch for indexing issues.
The loops skip in increments of two, then reindex by /2 for the finalpxandpy. Consider adding upper-bound checks or logging to prevent accidental out-of-bounds errors ifbitmap.Info.Widthor.Heightis odd.
216-225: Caret reposition logic is direct.
Nice approach. If future expansions require multiple caret positions or multi-line caret movement, consider factoring this logic into a dedicated class or method.
518-531: Segmented line logic for top, right, bottom, left is consistent with newLineStyles.
Good alignment with the new enumerations. For unknown or diagonal lines, the fallback toSingleLineis a reasonable default.
617-622: Signature change toExtractColorOrNullWithPlatformCheck(IPen pen, out LineStyles lineStyles)is a good step forward.
You replacedLineStyle?withLineStyles, ensuring a default always exists. Make sure consumers ofExtractColorOrNullWithPlatformCheckhandle the possibility that the returnedColor?can benull.
745-803: Wide glyph handling logic is comprehensive.
You carefully reset overlapped characters and handle multi-width emojis properly. Testing with complex scripts and combining characters can help ensure no edge cases remain.src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml (2)
58-65: Excellent implementation of the mixed line styles feature!The new Border element effectively demonstrates the ability to define different line styles for each edge. The syntax "SingleLine SingleLine DoubleLine DoubleLine" follows a clear left-top-right-bottom pattern, making it intuitive for users. The accompanying TextBlock provides clear documentation of the feature.
82-82: LGTM: Color change improves visual distinctionChanging the brush color to "Red" helps in better distinguishing this border example from others in the gallery.
|
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
166-177: Use helper method or dictionary to condense repeated block checks.This block handles each rectangle line position with multiple if-else statements. Consider replacing the chain of conditionals with a more concise approach (e.g., a lookup table) to reduce repetition and improve readability.
src/Consolonia.Core/Controls/LineStyles.cs (1)
17-27: Parsing logic is robust for partial/complete definitions.Splitting the string and assigning the leftover style to subsequent sides is a straightforward approach. Consider validating the tokens to gracefully handle invalid enums or trailing whitespace.
src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml (1)
82-82: Potential styling consistency check.Changing the brush for this border to “Red” might conflict with surrounding examples. Ensure it’s intentional if you want consistent theming across the gallery.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/Consolonia.Core/Controls/LineBrush.cs(1 hunks)src/Consolonia.Core/Controls/LineStyles.cs(1 hunks)src/Consolonia.Core/Drawing/DrawingContextImpl.cs(4 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml(2 hunks)src/Tests/Consolonia.Core.Tests/WithLifetimeFixture/LineStylesTests.cs(1 hunks)
🔇 Additional comments (30)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (5)
518-519: Ensure we handle null pen scenarios gracefully.The
ExtractColorOrNullWithPlatformCheckcall is made unconditionally. Ifpenmight be null, you could encounter null references outside of the null check. Verify that upstream logic prevents a null pen from reaching here.
525-532: Switch expression usage is clear and efficient.The new switch expression picking the correct side’s style is well-structured. Great job keeping it readable.
536-536: Separate logic for edge lines vs. normal lines.Calling
DrawEdgeLineis a good approach to keep specialized logic separate. No issues spotted.
541-547: Inline comments improve clarity.The inline comments (“//beginning”, “//line”, “//ending”) clarify the line-drawing segments. This is good for maintainability.
Line range hint
618-647: Leverage consistent defaults for missing brush references.When no suitable brush is found (or pen doesn't match expected types), the method returns null or uses a default
LineStyles. Confirm that callers handle a null return gracefully to avoid silent rendering failures.src/Consolonia.Core/Controls/LineBrush.cs (2)
16-17: Property name aligns with multi-style usage.Renaming to
LineStylePropertyof typeLineStylesmakes sense given the new multi-side feature.
25-25: Expanding the property to multiple sides.Switching from a single
LineStyletoLineStylesis consistent with the new design. Good job integrating the updated type.src/Consolonia.Core/Controls/LineStyles.cs (7)
1-9: Namespace and directives are clean.Imports and namespace structure look consistent with the rest of the codebase.
12-15: Default constructor establishing reasonable defaults.All sides default to
SingleLine, which is intuitive for minimal configuration.
29-36: Param-based constructor handles null values gracefully.Falling back to the preceding side’s style if a parameter is not provided is a neat approach. Well done.
38-41: Properties are straightforward.Exposing each side (Left, Top, Right, Bottom) is essential for fine-grained control.
43-46:Parsemethod convenient for direct string usage.Encapsulation of parsing logic behind
Parsehelps standardize line style creation.
48-51: Implicit operator from string.This makes calling code more succinct, allowing direct assignment from a string.
53-56: Implicit operator from singleLineStyle.Automatically converting a single
LineStyleto aLineStylesobject is consistent with the other usage patterns.src/Consolonia.Gallery/Gallery/GalleryViews/GalleryBorders.axaml (1)
58-65: Demonstrates multiple line styles on different edges.Using
"SingleLine SingleLine DoubleLine DoubleLine"is a helpful example. This clarifies how the new multi-side feature can be configured in XAML.src/Tests/Consolonia.Core.Tests/WithLifetimeFixture/LineStylesTests.cs (15)
11-19: Single style test coverage.Validates that a single token sets all sides consistently to SingleLine.
21-29: Double style test ensures fallback behavior.Confirms that the second style is applied to remaining edges.
31-39: Triple style test extends fallback logic.Checks partial override for sides, verifying correct inheritance rules.
41-49: Quad style test ensures all sides can be distinct.Demonstrates full coverage for Left, Top, Right, Bottom. Good scenario coverage.
51-55: Validation of invalid input.Confirming an
ArgumentExceptionfor too many tokens is an important negative test.
57-65: Empty constructor test.Verifies default values are all
SingleLine. Straightforward and vital.
67-75: Single constructor param test.Checks if all sides match the single provided style. This is an essential scenario.
77-85: Two-parameter constructor test.Ensures leftover sides inherit from the last non-null style. Nicely tested.
87-95: Three-parameter constructor test.Similarly checks partial definition logic. Good thorough coverage.
97-105: Four-parameter constructor test.Verifies each side can be independently set. No issues here.
107-115: Constructor with final null parameter.Ensures the bottom style defaults to the third parameter correctly.
117-125: Constructor with all null.Asserts that all sides remain
SingleLine. Good boundary test.
127-135: Implicit conversion from string test.Verifies that string-based initialization yields correct side assignments via the implicit operator.
137-145: Implicit conversion from enum test.Checks that a single
LineStyleautomatically populates all sides, as intended.
147-157: JSON serialization round-trip.Ensures that
LineStylescan be serialized and deserialized without losing data. Perfect approach to maintain state across application boundaries.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (2)
165-176: Consider using a switch expression for better readability.The repeated if-else chain can be simplified using a switch expression for a more concise and maintainable code.
- if (strokePosition == RectangleLinePosition.Left) - DrawBoxLineInternal(pen, stroke, RectangleLinePosition.Left); - else if (strokePosition == RectangleLinePosition.Right) - DrawBoxLineInternal(pen, stroke, RectangleLinePosition.Right); - else if (strokePosition == RectangleLinePosition.Top) - DrawBoxLineInternal(pen, stroke, RectangleLinePosition.Top); - else if (strokePosition == RectangleLinePosition.Bottom) - DrawBoxLineInternal(pen, stroke, RectangleLinePosition.Bottom); + DrawBoxLineInternal(pen, stroke, strokePosition switch + { + RectangleLinePosition.Left => RectangleLinePosition.Left, + RectangleLinePosition.Right => RectangleLinePosition.Right, + RectangleLinePosition.Top => RectangleLinePosition.Top, + RectangleLinePosition.Bottom => RectangleLinePosition.Bottom, + _ => strokePosition + });
Line range hint
620-646: Remove unnecessary initialization of lineStyles.The initialization of
lineStylesto null on line 622 is unnecessary as it's always assigned a value before being returned.- private static Color? ExtractColorOrNullWithPlatformCheck(IPen pen, out LineStyles lineStyles) - { - lineStyles = null; + private static Color? ExtractColorOrNullWithPlatformCheck(IPen pen, out LineStyles lineStyles) + {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs(4 hunks)
🔇 Additional comments (1)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
517-531: LGTM! Clean implementation of multi-edge line styles.The implementation elegantly handles different line styles for each edge position with a clear switch expression and sensible default.
Example:
LineStyle="SingleLine SingleLine DoubleLine DoubleLine"Will produce single line on left and top side and double line on right bottom.
Pattern is same as Thickness definitions: Left Top Right Bottom
If you define partial then last definition will apply.
Added unit tests, assignment operators etc.