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

Fix dialog deleting fullwidth characters #471

Closed
wants to merge 5 commits into from

Conversation

tompng
Copy link
Member

@tompng tompng commented Sep 28, 2022

Reline's dialog will delete fullwidth characters and fails to restore it in reset_dialog. This pull request fixes this.

dialog_on_fullwidth

In reset_dialog, what we want for Reline::Unicode.take_range('一二三', 1, 4) could be

  • 一二三 string covers range, needed for rerender bottom, rerender top in reset_dialog
  • 二三 rerender right in reset_dialog
  • 一二 render left in reset_dialog
  • string inside range
  • ▫️二▫️ string inside range with padding

Changes

take_range will also return position(col and width)

take_range('一二三', 1, 4) #=> ['二', 2, 2]
need to determine where to render the string.
should be rendered at col: 2, width: 2, not at col: 1, width: 4

add an option cover_begin, cover_end to take_range

take_range('一二三', 1, 4, cover_begin: true) #=> ['一二', 0, 4] for render left
take_range('一二三', 1, 4, cover_end: true) #=> ['二三', 2, 4] for render right

add an option padding to take_range

Delete padding_space_with_escape_sequences that only adds spaces at the end of the string.
take_range will properly add spaces around the string.
take_range('一二三', 1, 4, padding: true) #=> ['▫️二▫️', 1, 4]
take_range('一二三', 1, 8, padding: true) #=> ['▫️二三▫️▫️▫️', 1, 8]

Refactored reset_dialog

To make the main change easier, I reduced if branches of render top, left, right and bottom.
This change will fix a potentially-bug in rerender left

  • old_dialog.column is used where it should be dialog.column
  • sign on ydiff looks wrong, compared to rerender right
  • dialog.column usually don't move right, ydiff is usually zero, so it's hard to write failing test

Fixed inconsistency of take_range and calculate_width with special charactors \1 and \2

I Added some take_range tests for newly added option and regression test for escape sequence.
Found \1 and \2 is disappeared that will causes inconsistency of calculate_width and take_range.
calculate_width(take_range("a\1xxxxxxxx\2b", 0, 2), true) should be <=2, got 10
This might solve the root cause of #465 (I'm not sure, I cannot reproduce it).

Limitations

This pull request has a limitation. After document dialog is closed, right side of the completion dialog corrupts if there is full-width character between these dialogs.
dialog_clear
Clearing dialog depends on adjacent dialog positions, and to fix this, dialog rendering and clearing algorithm might need update.

# change clear_each_dialog to use remaining dialog positions
def clear_each_dialog(dialog_to_be_cleared, remaining_dialog_positions)
  # Carefully clear the dialog
end

# or clear multiple dialogs at once
def clear_dialogs(dialogs_to_be_cleared, remaining_dialog_positions)
  # Check all dialogs and collect all ranges to be cleared
  # Clear each calculated ranges
  # In this approach, we might be able to clear trailing spaces created by clear_each_dialog
end

# or just make a space between correction dialog and document dialog

@tompng tompng marked this pull request as draft October 23, 2022 22:45
@tompng tompng force-pushed the fix_dialog_on_fullwidth_chars branch from bcf0c59 to 7a07a1a Compare October 24, 2022 14:11
@tompng tompng marked this pull request as ready for review October 24, 2022 14:22
@tompng tompng mentioned this pull request Jan 1, 2023
@tompng
Copy link
Member Author

tompng commented Jan 1, 2023

I will close this because this pull request is incomplete (see Limitations section) and I opend #492 that solves it

@tompng tompng closed this Jan 1, 2023
@tompng tompng deleted the fix_dialog_on_fullwidth_chars branch May 4, 2023 16:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant