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.

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

Vertical Writing Direction Support #441

Closed
wants to merge 79 commits into from

Conversation

LEOYoon-Tsaw
Copy link
Member

  • Add vertical writing direction support, toggled by style/vertical in squirrel.yaml.
  • Also add a style/baseOffset to provide flexibility in adjusting font baseline.
  • Should not affect current users, purely additive.

SquirrelPanel.m Outdated
@@ -47,7 +49,7 @@ - (void)setText:(NSAttributedString *)text
}

- (void)setBackgroundColor:(NSColor *)backgroundColor {
_backgroundColor = (backgroundColor != nil ? backgroundColor : [NSColor windowBackgroundColor]);
NSColor * _backgroundColor = (backgroundColor != nil ? backgroundColor : [NSColor windowBackgroundColor]);
Copy link
Member

Choose a reason for hiding this comment

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

nit: name the local variable backgroundColor. and remove the unused property SquirrelView.backgroundColor

Copy link
Member Author

@LEOYoon-Tsaw LEOYoon-Tsaw Aug 15, 2020

Choose a reason for hiding this comment

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

Ok

SquirrelPanel.m Outdated
@@ -128,13 +128,35 @@ @implementation SquirrelPanel {
NSMutableDictionary *_commentHighlightedAttrs;
NSMutableDictionary *_preeditAttrs;
NSMutableDictionary *_preeditHighlightedAttrs;
NSParagraphStyle *_paragraphStyle;
NSParagraphStyle *_preeditParagraphStyle;
NSMutableParagraphStyle *_paragraphStyle;
Copy link
Member

Choose a reason for hiding this comment

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

NSMutableParagraphStyle inherits NSParagraphStyle. No need to change the type.

SquirrelPanel.m Outdated
NSParagraphStyle *_paragraphStyle;
NSParagraphStyle *_preeditParagraphStyle;
NSMutableParagraphStyle *_paragraphStyle;
NSMutableParagraphStyle *_preeditParagraphStyle;
Copy link
Member

Choose a reason for hiding this comment

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

no need to change the type.

SquirrelPanel.m Outdated
@@ -166,20 +188,20 @@ - (void)initializeUIStyle {
_preeditHighlightedAttrs[NSForegroundColorAttributeName] = [NSColor controlTextColor];
_preeditHighlightedAttrs[NSFontAttributeName] = [NSFont userFontOfSize:kDefaultFontSize];

_paragraphStyle = [NSParagraphStyle defaultParagraphStyle];
_preeditParagraphStyle = [NSParagraphStyle defaultParagraphStyle];
_paragraphStyle = [[NSParagraphStyle defaultParagraphStyle] mutableCopy];
Copy link
Member

Choose a reason for hiding this comment

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

no need to copy.

SquirrelPanel.m Outdated
_paragraphStyle = [NSParagraphStyle defaultParagraphStyle];
_preeditParagraphStyle = [NSParagraphStyle defaultParagraphStyle];
_paragraphStyle = [[NSParagraphStyle defaultParagraphStyle] mutableCopy];
_preeditParagraphStyle = [[NSParagraphStyle defaultParagraphStyle] mutableCopy];
Copy link
Member

Choose a reason for hiding this comment

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

no need to copy.

SquirrelPanel.m Outdated
// if the height is too large, it's hard to read, so need to put limit on the height.
if (windowRect.size.height > NSHeight(screenRect) / 3) {
windowRect.size.height = NSHeight(screenRect) / 3;
windowRect.size.width = [_view.text boundingRectWithSize:NSMakeSize(windowRect.size.height - _view.edgeInset.height * 2, windowRect.size.width - _view.edgeInset.width * 2) options:NSStringDrawingUsesLineFragmentOrigin].size.height + _view.edgeInset.height * 2;
Copy link
Member

Choose a reason for hiding this comment

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

too long a line

SquirrelPanel.m Outdated
@@ -225,6 +269,14 @@ - (void)show {
windowRect.origin.y = NSMinY(screenRect);
}
// voila !
Copy link
Member

Choose a reason for hiding this comment

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

nit: voila is for the last step.

SquirrelPanel.m Outdated
// rotate the view, the core in vertical mode!
if (_vertical) {
_view.boundsRotation = -90.0;
[_view setBoundsOrigin:NSMakePoint(_view.contentSize.width + _view.edgeInset.width * 2, _view.edgeInset.height * 2)];
Copy link
Member

Choose a reason for hiding this comment

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

why isn't boundsOrigin.y = 0 but _view.edgeInset.height * 2?

Copy link
Member Author

@LEOYoon-Tsaw LEOYoon-Tsaw Aug 15, 2020

Choose a reason for hiding this comment

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

Right, this is better, I'm going to change it

SquirrelPanel.m Outdated
if (_inlinePreedit) {
windowRect.origin.x -= windowRect.size.width;
} else {
windowRect.origin.x -= windowRect.size.width - preeditSize.height - _view.edgeInset.width;
Copy link
Member

Choose a reason for hiding this comment

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

_view.edgeInset.width or _view.edgeInset.height?

Copy link
Member Author

Choose a reason for hiding this comment

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

It depends on how user interpret borderHeight in vertical mode.. There're more places to look at for the consistency. But this edgeInset.width consistency issue can be dealt after all other gardening's done.

SquirrelPanel.m Outdated
@@ -402,70 +473,116 @@ - (void)showPreedit:(NSString *)preedit
}

if (i > 0) {
NSAttributedString *separator = [[NSAttributedString alloc]
NSMutableAttributedString *separator = [[NSMutableAttributedString alloc]
Copy link
Member

Choose a reason for hiding this comment

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

no need to be mutable.

@lotem
Copy link
Member

lotem commented Aug 15, 2020

This is a great feature. I need more time to understand the changes.
Take notes here of a few bugs I experienced:

  • style/horizontal should be disabled when style/vertical is true.
  • style/base_offset also affects non-vertical mode; prefer applying base offset only to vertical UI so that switching layout direction is as simple as changing one boolean flag.
  • line wrapping within preedit text or candidate causes window to move away from cursor position. maybe due to measuring window size taken place before breaking lines.
  • somehow non-vertical UI is affected by 70ba02c

@LEOYoon-Tsaw
Copy link
Member Author

  • style/horizontal should be disabled when style/vertical is true. This is an obvious conflict, but which takes higher priority? Should Squirrel take the vertical or horizontal if both are true, or fail to deploy?
  • style/base_offset can be useful in other mode, this is the reason I added it for other modes as well, since users might want to vertically align candidates as different fonts have different base line height. But users with out this property will notice no change, it's pure additive.
  • line wrapping, I center the left-most line of preedit to the cursor y position, so that the candidates area expands to the left and preedit area expand to the right. In inline-edit mode, the first candidate is fixed at the cursor y, so people don't need to track where the first candidate is in vertical mode.
  • I'll make minimumLineHeight and maximumLineHeight exclusive to vertical mode'.

@lotem
Copy link
Member

lotem commented Aug 16, 2020

…reedit_back_color set

(cherry picked from commit d2a2859)
Adopts TextStorage to support general linebreak,
a new slim vertical mode (horizontal=true & vertical=true),
more advanced highlighting,
dark mode autoswitching
@LEOYoon-Tsaw
Copy link
Member Author

LEOYoon-Tsaw commented Aug 25, 2020

Will solve #403 #412 #422 #397 #129 #367 #374 #289

@lotem lotem mentioned this pull request Aug 29, 2020
@LEOYoon-Tsaw
Copy link
Member Author

Will refactor the remaining parts for a new pull

@LEOYoon-Tsaw LEOYoon-Tsaw deleted the Vertical branch April 18, 2022 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants