-
-
Notifications
You must be signed in to change notification settings - Fork 72
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.
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
Fix Vertical Layout #432
Fix Vertical Layout #432
Conversation
Copilot
AI
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 12 out of 27 changed files in this pull request and generated 1 comment.
Files not reviewed (15)
- samples/DrawWithImageSharp/DrawWithImageSharp.csproj: Language not supported
- src/SixLabors.Fonts/Tables/AdvancedTypographic/GSub/LookupType4SubTable.cs: Evaluated as low risk
- src/SixLabors.Fonts/FontMetrics.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/General/CMap/CMapSubTable.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/Shapers/UniversalShaper.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/Shapers/IndicShaper.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/Shapers/HangulShaper.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/Shapers/DefaultShaper.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/GSub/LookupType8SubTable.cs: Evaluated as low risk
- src/SixLabors.Fonts/FileFontMetrics.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/GSub/LookupType3SubTable.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/GSub/LookupType1SubTable.cs: Evaluated as low risk
- src/SixLabors.Fonts/Tables/AdvancedTypographic/GSub/LookupType2SubTable.cs: Evaluated as low risk
- src/SixLabors.Fonts/GlyphSubstitutionCollection.cs: Evaluated as low risk
- src/SixLabors.Fonts/StreamFontMetrics.cs: Evaluated as low risk
Comments suppressed due to low confidence (2)
src/SixLabors.Fonts/GlyphPositioningCollection.cs:95
- The parameter name isVerticalSubstitution is inconsistent with other parts of the code. It should be renamed to isVertical.
out bool isVerticalSubstitution,
src/SixLabors.Fonts/Tables/General/CMap/Format14SubTable.cs:139
- Ensure that the behavior of TryGetCodePoint always returning false is covered by tests.
public override bool TryGetCodePoint(ushort glyphId, out CodePoint codePoint)
isDecomposed = false; | ||
|
||
Tag vert = FeatureTags.VerticalAlternates; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logic for checking vertical substitutions using FeatureTags.VerticalAlternates, FeatureTags.VerticalAlternatesAndRotation, and FeatureTags.VerticalAlternatesForRotation is duplicated in multiple places. This logic should be refactored into a separate method to avoid duplication and potential inconsistencies.
Tag vert = FeatureTags.VerticalAlternates; | |
isVerticalSubstitution = CheckVerticalSubstitution(glyph.Data.AppliedFeatures); |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic is applied within a loop with other tests. Cannot abstract.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good
Prerequisites
Description
Fixes #429 by ensuring that we test against the substituted codepoint where possible. Also enhances CJK codepoint detection.