Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify DrawingContextImpl iteration logic #114

Merged
merged 3 commits into from
Jan 5, 2024
Merged

Simplify DrawingContextImpl iteration logic #114

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
270a018
Simplify DrawingContextImpl iteration logic
jinek Jan 5, 2024
339b15d
Fix coordinate assignment in pixel buffer
jinek Jan 5, 2024
ac84182
Automated JetBrains cleanup
github-actions[bot] Jan 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 29 additions & 28 deletions src/Consolonia.Core/Drawing/DrawingContextImpl.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -387,35 +387,33 @@
for (int i = 0; i < str.Length; i++)
{
Point characterPoint = whereToDraw.Transform(Matrix.CreateTranslation(currentXPosition++, 0));
// ReSharper disable AccessToModifiedClosure
CurrentClip.ExecuteWithClipping(characterPoint, () =>
ConsoleColor foregroundColor = consoleColorBrush.Color;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The declaration of foregroundColor is redundant if additionalBrushes is not null and brush is of type FourBitColorBrush with PixelBackgroundMode.Colored, as it gets reassigned in line 410.

- ConsoleColor foregroundColor = consoleColorBrush.Color;
+ ConsoleColor foregroundColor;
Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
ConsoleColor foregroundColor = consoleColorBrush.Color;
ConsoleColor foregroundColor;


if (additionalBrushes != null)
{
ConsoleColor foregroundColor = consoleColorBrush.Color;
if (additionalBrushes != null)
(FormattedText.FBrushRange _, IBrush brush) = additionalBrushes.FirstOrDefault(pair =>
{
(FormattedText.FBrushRange _, IBrush brush) = additionalBrushes.FirstOrDefault(pair =>
{
// ReSharper disable once AccessToModifiedClosure //todo: pass as a parameter
int globalIndex = i + startIndex;
(FormattedText.FBrushRange key, _) = pair;
return key.StartIndex <= globalIndex && globalIndex < key.EndIndex;
});
// ReSharper disable once AccessToModifiedClosure //todo: pass as a parameter
int globalIndex = i + startIndex;
(FormattedText.FBrushRange key, _) = pair;
return key.StartIndex <= globalIndex && globalIndex < key.EndIndex;
});

if (brush != null)
if (brush != null)
{
if (brush is not FourBitColorBrush { Mode: PixelBackgroundMode.Colored } additionalBrush)
{
if (brush is not FourBitColorBrush { Mode: PixelBackgroundMode.Colored } additionalBrush)
{
ConsoloniaPlatform.RaiseNotSupported(11);
return;
}

foregroundColor = additionalBrush.Color;
ConsoloniaPlatform.RaiseNotSupported(11);
return;
}

foregroundColor = additionalBrush.Color;
}
}
Comment on lines +392 to +412
Copy link
Contributor

@coderabbitai coderabbitai bot Jan 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for determining the foregroundColor based on additionalBrushes is correct, but it could be more efficient. The current implementation uses FirstOrDefault, which iterates over the collection even after a match is found. Consider refactoring to a loop that breaks once a match is found to improve performance.

- (FormattedText.FBrushRange _, IBrush brush) = additionalBrushes.FirstOrDefault(pair =>
- {
-     int globalIndex = i + startIndex;
-     (FormattedText.FBrushRange key, _) = pair;
-     return key.StartIndex <= globalIndex && globalIndex < key.EndIndex;
- });
- if (brush != null)
+ IBrush brush = null;
+ int globalIndex = i + startIndex;
+ foreach (var pair in additionalBrushes)
+ {
+     (FormattedText.FBrushRange key, IBrush currentBrush) = pair;
+     if (key.StartIndex <= globalIndex && globalIndex < key.EndIndex)
+     {
+         brush = currentBrush;
+         break;
+     }
+ }
+ if (brush is FourBitColorBrush { Mode: PixelBackgroundMode.Colored } additionalBrush)
Committable suggestion

❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
if (additionalBrushes != null)
{
ConsoleColor foregroundColor = consoleColorBrush.Color;
if (additionalBrushes != null)
(FormattedText.FBrushRange _, IBrush brush) = additionalBrushes.FirstOrDefault(pair =>
{
(FormattedText.FBrushRange _, IBrush brush) = additionalBrushes.FirstOrDefault(pair =>
{
// ReSharper disable once AccessToModifiedClosure //todo: pass as a parameter
int globalIndex = i + startIndex;
(FormattedText.FBrushRange key, _) = pair;
return key.StartIndex <= globalIndex && globalIndex < key.EndIndex;
});
// ReSharper disable once AccessToModifiedClosure //todo: pass as a parameter
int globalIndex = i + startIndex;
(FormattedText.FBrushRange key, _) = pair;
return key.StartIndex <= globalIndex && globalIndex < key.EndIndex;
});
if (brush != null)
if (brush != null)
{
if (brush is not FourBitColorBrush { Mode: PixelBackgroundMode.Colored } additionalBrush)
{
if (brush is not FourBitColorBrush { Mode: PixelBackgroundMode.Colored } additionalBrush)
{
ConsoloniaPlatform.RaiseNotSupported(11);
return;
}
foregroundColor = additionalBrush.Color;
ConsoloniaPlatform.RaiseNotSupported(11);
return;
}
foregroundColor = additionalBrush.Color;
}
}
if (additionalBrushes != null)
{
IBrush brush = null;
int globalIndex = i + startIndex;
foreach (var pair in additionalBrushes)
{
(FormattedText.FBrushRange key, IBrush currentBrush) = pair;
if (key.StartIndex <= globalIndex && globalIndex < key.EndIndex)
{
brush = currentBrush;
break;
}
}
if (brush is FourBitColorBrush { Mode: PixelBackgroundMode.Colored } additionalBrush)
{
foregroundColor = additionalBrush.Color;
}
}

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FirstOrDefault does not iterate over the collection after match found.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jinek You're absolutely right; FirstOrDefault stops iterating after finding a match. My apologies for the oversight. Thank you for the clarification.

New Learnings Added 
User: jinek
PR: jinek/Consolonia#114
File: src/Consolonia.Core/Drawing/DrawingContextImpl.cs:392-412
Timestamp: 2024-01-05T19:02:39.703Z
Learning: The `FirstOrDefault` method in LINQ stops iterating over the collection once a match is found. This was clarified by the user in the context of optimizing the search for a matching brush in the `additionalBrushes` collection.

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!


char character = str[i];
char character = str[i];

switch (character)
switch (character)
{
case '\t':
{
Expand All @@ -426,12 +424,11 @@
Point newCharacterPoint = characterPoint.WithX(characterPoint.X + j);
CurrentClip.ExecuteWithClipping(newCharacterPoint, () =>
{
_pixelBuffer.Set((PixelBufferCoordinate)characterPoint.WithX(characterPoint.X + j),

Check warning on line 427 in src/Consolonia.Core/Drawing/DrawingContextImpl.cs

View workflow job for this annotation

GitHub Actions / build 

"[AccessToModifiedClosure] Captured variable is modified in the outer scope" on /home/runner/work/Consolonia/Consolonia/src/Consolonia.Core/Drawing/DrawingContextImpl.cs(427,15595)
(oldPixel, cp) => oldPixel.Blend(cp), consolePixel);
});
}



currentXPosition += tabSize - 1;
}
break;
Expand All @@ -444,17 +441,21 @@
(oldPixel, cp) => oldPixel.Blend(cp), consolePixel);*/
}
break;
case '\u200B':
currentXPosition--;
break;
default:
{
var consolePixel = new Pixel(character, foregroundColor);

_pixelBuffer.Set((PixelBufferCoordinate)characterPoint,
(oldPixel, cp) => oldPixel.Blend(cp), consolePixel);
CurrentClip.ExecuteWithClipping(characterPoint, () =>
{
_pixelBuffer.Set((PixelBufferCoordinate)characterPoint,
(oldPixel, cp) => oldPixel.Blend(cp), consolePixel);
}
);
}
break;
}
});
// ReSharper restore AccessToModifiedClosure
}
}
}
Expand Down
Loading