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

Completely support full-width characters in differential rendering #654

Merged
merged 2 commits into from
Apr 29, 2024

Conversation

tompng
Copy link
Member

@tompng tompng commented Mar 19, 2024

When dialog is cleared, we need to restore the text that was hidden behind dialogs.

To implement restoring full width characters hidden behind dialog, Reline::Unicode.take_range lacks of feature.
It does not provide where to render the string. column information is missing.

Reline::Unicode.take_range('一二三四五a', 2, 5) #=> '二三' # should render at column=2
Reline::Unicode.take_range('a一二三四五', 2, 5) #=> '二三' # should render at column=3

Sometimes we need other cut variation depending on adjacent dialog position.
The image below shows a variation of take_range('一二三四五六七八九', 5, 8, option)
dialog_restore

So I add Reline::Unicode.take_mbchar_range that has option cover_begin, cover_end, padding and make it return the actual cut position.

take_mbchar_range('一二三', 1, 4) #=> ['二', 2, 2]
take_mbchar_range('一二三', 1, 4, cover_begin: true) #=> ['一二', 0, 4]
take_mbchar_range('一二三', 1, 4, cover_end: true) #=> ['二三', 2, 4]
take_mbchar_range('一二三', 1, 4, padding: true) #=> [' 二 ', 1, 4]
take_mbchar_range('一二三', 1, 4, cover_begin: true, padding: true) #=> ['一二 ', 0, 5]
take_mbchar_range('一二三', 1, 4, cover_end: true, padding: true) #=> [' 二三', 1, 5]

# Padding space can have different background color
take_mbchar_range("#{RED_BACKGROUND}#{BLUE_BACKGROUND}#{GREEN_BACKGROUND}三", 1, 4, padding: true)
#=> "#{RED_BACKGROUND} #{BLUE_BACKGROUND}二#{GREEN_BACKGROUND} "

@tompng tompng force-pushed the differential_rendering_fullwidth branch from 95220ed to 75ca084 Compare April 1, 2024 17:57
total_width += mbchar_width
break if (start_col + max_width) < total_width
chunk << gc if start_col < total_width
break if !cover_end && total_width > start_col + width
Copy link
Member

Choose a reason for hiding this comment

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

Could you add comments to these conditional branches?

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot to implement this.

# Red background あ
Reline::Unicode.take_mbchar_range "\e[41mあ", 1, 2, padding: true
# Red background whitespace + No background whitespace
["\e[41m \e[0m ", 1, 2]

I've updated the implementation and add comments

@@ -692,13 +697,6 @@ def add_dialog_proc(name, p, context = nil)

DIALOG_DEFAULT_HEIGHT = 20

private def padding_space_with_escape_sequences(str, width)
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious, why did you combine calculating padding with take_mbchar_range?

Copy link
Member Author

@tompng tompng Apr 25, 2024

Choose a reason for hiding this comment

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

Padding sometimes have background colors.
The last testcase I added is a good example.

assert_equal ["\e[41m \e[42mい\e[43m ", 1, 4], Reline::Unicode.take_mbchar_range("\e[41mあ\e[42mい\e[43mう\e[0m", 1, 4, padding: true)

colored padding

So we can't add non-colored space as padding. Padding logic cannot be separated from take_mbchar_range

@tompng tompng force-pushed the differential_rendering_fullwidth branch from 75ca084 to a7c4064 Compare April 27, 2024 09:35
Copy link
Member

@ima1zumi ima1zumi left a comment

Choose a reason for hiding this comment

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

LGTM!

@ima1zumi ima1zumi merged commit 29714df into ruby:master Apr 29, 2024
40 checks passed
matzbot pushed a commit to ruby/ruby that referenced this pull request Apr 29, 2024
differential rendering
(ruby/reline#654)

* Add a cut variation of Reline::Unicode.take_range method take_mbchar_range

* Consider fullwidth take_range in differential rendering

ruby/reline@29714df09f
@tompng tompng deleted the differential_rendering_fullwidth branch April 29, 2024 12:17
@ima1zumi ima1zumi added the enhancement New feature or request label Apr 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

2 participants