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

Rewrite dialog rendering #492

Merged
merged 4 commits into from
May 1, 2023
Merged

Conversation

tompng
Copy link
Member

@tompng tompng commented Jan 1, 2023

Reline can have multiple dialog in random position as an API design, but failed to render it correctly.
This pull-request will enable rendering multiple dialogs correctly.

Left: before, Right: after

dialogs.mp4

Fixes #489
Fixes #458 in another way.
No more remaining whitespaces. Before this pullrequest, white space remained where dialog exist after dialog closes.

Change dialog rendering

Dialog#contents is now used as an offscreen canvas for each dialog.

Dialogs cannot be clear and re-rendered independently in render_each_dialog, especially when they are overlapped.
I added render_dialog_changes. It can be used in both dialog rendering and dialog clearing.

  • Takes all dialog's previous position and next position as argument
  • Calculates where to restore the input text under dialogs
  • Clear and render all dialogs at once

Delete Dialog#lines_backup

Dialog#lines_backup is not needed. It contains line buffers and base dialog_y coordinate of previous rendering but these are not dialog dependant.
Line buffers(visual_lines) can be calculated from whole_lines() when we need to restore underlying text.
All we need is @previous_rendered_dialog_y stored in LineEditor.

Handling fullwidth unicode characters

I made a separate pullrequest for it #519

Other

I used this code to capture the movie above

srand 0
[[47,30],[42,30],[43,30],[44,37],[45,37]].each_with_index do |(bg,fg),index|
  pointClass = Data.define :x, :y
  Reline.add_dialog_proc :"dialog#{index}", ->{
    contents = 6.times.map do
      '-' * _1 + 'abcdef' + '-' * (5 - _1)
    end
    Reline::DialogRenderInfo.new(
      pos: pointClass.new(x: rand(40), y: rand(20)),
      contents: contents,
      height: 10,
      bg_color: bg,
      fg_color: fg,
    )
  }
end

@tompng
Copy link
Member Author

tompng commented Jan 1, 2023

Dialog rendering with fullwidth characters looks working in macOS Terminal.app but not in other terminal emulators because of the difference shown below. (I split the fullwidth character diff to #519)

printf "あいうえお\e[7Dかき\n"
printf "あいうえお\e[7Dkaki\n"
printf "aaiiuueeoo\e[7Dかき\n"

# Terminal.app, Terminal in VSCode
あ かき お
あ kaki お
aaiかきeoo

# iTerm2
あ  かきお
あ kaki お
aaiかきeoo

# yamatanooroti(vterm 0.3)
あいかきお
あいkakiお
aaiかきeoo

To handle this correctly, we should write "\e[7Dかき" or "\e[8D かき" depending on what character was there on the screen before. I think it's difficult and sometimes almost impossible(image below).
Almost impossible to handle fullwidth sample

@tompng tompng marked this pull request as draft January 2, 2023 15:08
@tompng tompng marked this pull request as ready for review January 2, 2023 18:27
@tompng tompng force-pushed the rewrite_dialog_rendering branch from d288fe0 to 9a1cc0c Compare January 12, 2023 12:43
@hasumikin
Copy link
Collaborator

I think test_yamatanooroti will be fixed after merging #498 (and rebasing this branch)

@tompng tompng force-pushed the rewrite_dialog_rendering branch from 9a1cc0c to a4a598d Compare January 14, 2023 11:13
@tompng
Copy link
Member Author

tompng commented Jan 14, 2023

I rebased but it still fails test_yamatanooroti. I opend a pull request #502

@tompng tompng force-pushed the rewrite_dialog_rendering branch 2 times, most recently from e9e636f to db22275 Compare January 18, 2023 16:49
@tompng tompng force-pushed the rewrite_dialog_rendering branch from db22275 to f9c6dd0 Compare February 7, 2023 14:28
@tompng tompng force-pushed the rewrite_dialog_rendering branch from f9c6dd0 to b37d971 Compare February 27, 2023 11:20
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

The unicode change seems relatively independent from the rest. If that's true, can you open a separate PR for it?

lib/reline/line_editor.rb Outdated Show resolved Hide resolved
lib/reline/line_editor.rb Outdated Show resolved Hide resolved
@tompng tompng marked this pull request as draft March 4, 2023 20:08
@tompng tompng force-pushed the rewrite_dialog_rendering branch from b37d971 to 549117a Compare March 4, 2023 20:17
@tompng tompng force-pushed the rewrite_dialog_rendering branch from 549117a to 2290ce7 Compare March 15, 2023 14:22
@tompng tompng marked this pull request as ready for review March 15, 2023 14:51
@tompng
Copy link
Member Author

tompng commented Mar 17, 2023

The unicode change seems relatively independent from the rest. If that's true, can you open a separate PR for it?

I separated the unicode full-width part to #519

@tompng tompng force-pushed the rewrite_dialog_rendering branch from 10e7d64 to ec3c334 Compare April 25, 2023 12:14
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Tested it with IRB a bit and I think it worked great. Thank you for the amazing rewrite 🙏

Just some questions and to check my understanding:

  • update_each_dialog method only update dialogs' content, but not writing to the output
  • render_dialog_changes writes changes to the output

Are they correct?

lib/reline/line_editor.rb Show resolved Hide resolved
lib/reline/line_editor.rb Show resolved Hide resolved
lib/reline/line_editor.rb Show resolved Hide resolved
@tompng
Copy link
Member Author

tompng commented Apr 27, 2023

  • update_each_dialog method only update dialogs' content, but not writing to the output
  • render_dialog_changes writes changes to the output

Yes, that's correct

@tompng
Copy link
Member Author

tompng commented Apr 28, 2023

This is a slow-motion video (with cursor visible) of two algorithms

on master branch

clear and render by each dialog

dialog_before.mp4

on this pull request

clear and render by each line from up to down

dialog_after.mp4

Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

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

Still can't say that I understand all the computation 😅 But based on the video I think the new approach should be more robust and enables future fixes too. Also didn't find any issue when tested locally, so let's give it a try 👍

@tompng tompng merged commit 6cee9fa into ruby:master May 1, 2023
@tompng tompng deleted the rewrite_dialog_rendering branch May 1, 2023 12:20
matzbot pushed a commit to ruby/ruby that referenced this pull request May 1, 2023
(ruby/reline#492)

* Rewrite dialog rendering

* Fix failing test of dialog with small screen

* Add multiple-dialog rendering test

* Add description comments for each part of render_dialog_changes
@ima1zumi ima1zumi added the bug Something isn't working label May 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Development

Successfully merging this pull request may close these issues.

Hole in completion dialog
4 participants