Reline 0.5.0.pre #614

tompng · 2023-12-01T14:49:13Z

This is a branch to release #595 as 0.5.0.pre.x
See description of #595

Instance variables of cursor and screens

before this pull request

after this pull request

lib/reline/line_editor.rb

lib/reline.rb

lib/reline/line_editor.rb

st0012 · 2024-01-22T11:53:04Z

lib/reline/line_editor.rb

+    end
+
+    if new_lines != rendered_lines
+      Reline::IOGate.hide_cursor


For my own learning: when do we need to hide cursor and why? I see there are other operations involving moving cursor around, but this seems to be the only place to hide it?

When lines to be rendered changed, reline needs to move cursor and write texts many times. We need to hide cursor because we don't want to show the cursor moving while rendering.
Example of not hiding cursor (Repeating Insert ) and delete)

On the other hand, if we always hide cursor in render_differential, cursor sometimes blink when just moving cursor.
Example of pressing arrow keys with hiding cursor

We need this for comfortable editing, so we don't have to hide cursor in exceptional case like clear screen with C-l.

Can we have a comment for this?

st0012 · 2024-01-22T11:56:33Z

lib/reline/line_editor.rb

+  end
+
+  def rerender
+    handle_cleared


Since this is the only caller of handle_cleared, how about we just move its content here and remove the method?

It also looks like we could be calling render_differential twice under the current implementation?

It also looks like we could be calling render_differential twice under the current implementation?

Nice catch. I remove render_differential from handle_cleared. Finally when pasting is complete, rerender will be called with @in_pasting == false from reline.rb.

I want to leave handle_cleared because I want to remove handle_cleared from rerender in the future.
But changing it now needs test_key_actor_emacs.rb that depends on instance variable @cleared to be changed.

Plan and reason

private def ed_clear_screen(key) @cleared = true end def rerender handle_cleared render_differential unless @in_pasting end

↓

private def ed_clear_screen(key) @rendered_screen_cache = nil Reline::IOGate.clear_screen @screen_size = Reline::IOGate.get_screen_size @cursor_base_y = 0 @cursor_y = 0 end # We need handle_resize(`def resize` is implemented) and handle_interrupt(not implemented, causing deadlock and thread problem) to handle signals correctly # We don't need handle_cleared. clear is not a signal def rerender render_differential unless @in_pasting end

lib/reline/line_editor.rb

ima1zumi · 2024-03-18T16:12:50Z

lib/reline.rb

+      line_editor.print_nomultiline_prompt(prompt)
+      line_editor.update_dialogs


Please let me know for study purposes. Why do I need to update dialogs before read_io?

Reline has an ability to always show dialog. Reline needs to setup and render dialog on initial render, before first read_io.

irb(main):001> Reline.add_dialog_proc(:FOO,->{Reline::DialogRenderInfo.new contents:['DIALOG'], pos: cursor_pos}) irb(main):002> DIALOG

This is tested in test_dialog_narrower_than_screen and test_dialog_with_fullwidth_scrollbar

lib/reline.rb

ima1zumi · 2024-03-18T16:10:45Z

lib/reline/line_editor.rb

    else
-      @line = Reline::HISTORY[@history_pointer]
+      @buffer_of_lines = [Reline::HISTORY[@history_pointer]]
+      calculate_nearest_cursor(cursor)


For my own learning: Why is it necessary to calculate the cursor position here?

When @buffer_of_lines[@line_index] or @buffer_of_lines itself is directly modified, we need set byte_pointer to 0 or line.bytesize, or re-calculate byte_position based on previous cursor position to avoid byte_position being in a middle of grapheme cluster.

For this case, I just found a bug like this which is fixed by this calculate_nearest_cursor.

# prepare history irb(main):001> "1️⃣" => "1️⃣" irb(main):002> "1 # press PGUP and 2 irb(main):002> "12️⃣" # press DELETE DELETE irb(main):002> "(garbled text)"

ima1zumi · 2024-03-18T16:07:34Z

test/reline/yamatanooroti/test_rendering.rb

-          prompt>   5
-          prompt>   6 Ruby is...
+          prompt>   5 Ruby is...
+          prompt>   6 A dynamic, open source programming


Why is this change needed?

It's because it was testing wrong behavior.

This bug is fixed in this pull request, so the expected state needs to be updated.

ima1zumi · 2024-03-18T16:09:18Z

lib/reline/line_editor.rb

+
+  # Calculate cursor position in word wrapped content.
+  def wrapped_cursor_position
+    line = ' ' * calculate_width(prompt_list[@line_index], true) + whole_lines[@line_index].byteslice(0, @byte_pointer)


How about a local variable name of line_before_cursor? I thought line here is not the whole line.

test/reline/test_line_editor.rb

ima1zumi · 2024-03-18T12:13:35Z

lib/reline/line_editor.rb

+    end
+
+    if new_lines != rendered_lines
+      Reline::IOGate.hide_cursor


Can we have a comment for this?

ima1zumi · 2024-03-18T16:17:51Z

PROMPT_LIST_CACHE_TIMEOUT in line_editor.rb can also be removed.

…r flickering.

It only need to clear screen. line_editor.rerender will be called later.

…n_buffer

Co-authored-by: Stan Lo <stan001212@gmail.com>

Fix bug updating `@cursor_y`. Do not use `@cursor_y` while updating dialog because it is not current cursor position but cursor position at last rendered time.

These instance variables are already removed from LineEditor

…e unused instance variable

…d_screen`

It represents cursor position of word wrapped whole content

ima1zumi

Thank you for the excellent refactoring! 🌟
This PR has made the rendering much easier to understand and use.

(ruby/reline#614) * Re-architecture LineEditor's internal state and rendering * Fix test related to LineEditor re-architecture * Bump to 0.5.0.pre.1 * Hide cursor only when updating screen. Frequent hide&show makes cursor flickering. * Simplify rerender call from reline.rb * Simplify handle_cleared It only need to clear screen. line_editor.rerender will be called later. * Add description of updating pasting_state inserts continuous_insertion_buffer * Use meaningful block parameter Co-authored-by: Stan Lo <stan001212@gmail.com> * Fix use of `@cursor_y` Fix bug updating `@cursor_y`. Do not use `@cursor_y` while updating dialog because it is not current cursor position but cursor position at last rendered time. * Remove useless instance_variable_set in test These instance variables are already removed from LineEditor * Always initialize instance variables to avoid ruby 2.7 warning, remove unused instance variable * Call update_dialogs from reline.rb before first render * Combine state representing rendered screen information into `@rendered_screen` * Rename editor_cursor_ to wrapped_cursor It represents cursor position of word wrapped whole content * Remove unused code, tweak, add comment --------- ruby/reline@3fa376217d Co-authored-by: Stan Lo <stan001212@gmail.com>

tompng added the Don't merge label Dec 1, 2023

tompng marked this pull request as draft December 1, 2023 15:05

tompng force-pushed the reline_0.5.0.pre branch from bff6012 to aac06dc Compare December 1, 2023 15:15

tompng mentioned this pull request Dec 1, 2023

LineEditor Re-architecture #595

Closed

tompng force-pushed the reline_0.5.0.pre branch from 080869f to 0b421c5 Compare January 4, 2024 15:56

st0012 reviewed Jan 6, 2024

View reviewed changes

lib/reline/line_editor.rb Outdated Show resolved Hide resolved

tompng removed the Don't merge label Jan 21, 2024

tompng force-pushed the reline_0.5.0.pre branch from 961c294 to 3cfc6f5 Compare January 21, 2024 05:11

tompng changed the title ~~[DO NOT MERGE] Reline 0.5.0.pre release~~ Reline 0.5.0.pre Jan 21, 2024

tompng marked this pull request as ready for review January 21, 2024 05:40

st0012 reviewed Jan 22, 2024

View reviewed changes

ima1zumi reviewed Jan 24, 2024

View reviewed changes

lib/reline/line_editor.rb Show resolved Hide resolved

lib/reline/line_editor.rb Outdated Show resolved Hide resolved

lib/reline/line_editor.rb Show resolved Hide resolved

tompng force-pushed the reline_0.5.0.pre branch from 2740c66 to b541d30 Compare February 24, 2024 17:35

ima1zumi reviewed Mar 18, 2024

View reviewed changes

tompng and others added 14 commits March 19, 2024 21:48

Re-architecture LineEditor's internal state and rendering

44706f6

Fix test related to LineEditor re-architecture

f0860b7

Bump to 0.5.0.pre.1

821f0c3

Hide cursor only when updating screen. Frequent hide&show makes curso…

57e1106

…r flickering.

Simplify rerender call from reline.rb

389337a

Simplify handle_cleared

b54424a

It only need to clear screen. line_editor.rerender will be called later.

Add description of updating pasting_state inserts continuous_insertio…

934b840

…n_buffer

Use meaningful block parameter

83b6e09

Co-authored-by: Stan Lo <stan001212@gmail.com>

Fix use of @cursor_y

7cc44c0

Fix bug updating `@cursor_y`. Do not use `@cursor_y` while updating dialog because it is not current cursor position but cursor position at last rendered time.

Remove useless instance_variable_set in test

dcd1c9d

These instance variables are already removed from LineEditor

Always initialize instance variables to avoid ruby 2.7 warning, remov…

3e30a59

…e unused instance variable

Call update_dialogs from reline.rb before first render

049c632

Combine state representing rendered screen information into `@rendere…

2fcf837

…d_screen`

Rename editor_cursor_ to wrapped_cursor

f0f0afc

It represents cursor position of word wrapped whole content

tompng force-pushed the reline_0.5.0.pre branch from b541d30 to d02b844 Compare March 19, 2024 12:50

tompng force-pushed the reline_0.5.0.pre branch from d02b844 to 83232d0 Compare March 19, 2024 12:51

Remove unused code, tweak, add comment

97d76d8

tompng force-pushed the reline_0.5.0.pre branch from 83232d0 to 97d76d8 Compare March 19, 2024 13:12

ima1zumi approved these changes Mar 19, 2024

View reviewed changes

ima1zumi merged commit 3fa3762 into master Mar 19, 2024
79 checks passed

ima1zumi deleted the reline_0.5.0.pre branch March 19, 2024 14:17

ima1zumi mentioned this pull request Apr 13, 2024

Newline characters are inserted into wordwrapped text #610

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reline 0.5.0.pre #614

Reline 0.5.0.pre #614

tompng commented Dec 1, 2023 •

edited

Loading

st0012 Jan 22, 2024

tompng Jan 22, 2024

ima1zumi Mar 18, 2024

st0012 Jan 22, 2024

tompng Jan 22, 2024

tompng Jan 22, 2024

ima1zumi Mar 18, 2024

tompng Mar 19, 2024

ima1zumi Mar 18, 2024

tompng Mar 19, 2024

ima1zumi Mar 18, 2024

tompng Mar 19, 2024

ima1zumi Mar 18, 2024

ima1zumi Mar 18, 2024

ima1zumi commented Mar 18, 2024

ima1zumi left a comment

		line_editor.print_nomultiline_prompt(prompt)
		line_editor.update_dialogs

Reline 0.5.0.pre #614

Reline 0.5.0.pre #614

Conversation

tompng commented Dec 1, 2023 • edited Loading

Instance variables of cursor and screens

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ima1zumi commented Mar 18, 2024

ima1zumi left a comment

Choose a reason for hiding this comment

tompng commented Dec 1, 2023 •

edited

Loading