Text-based UI support #503

st0012 · 2022-01-27T10:34:12Z

This PR adds the TUI support proposed in #305.

Usage

tui opens TUI with a default top window (locals)
tui on [type] opens TUI with a type top window (current has locals and src)
- tui on src,locals can open 2 windows
tui off closes TUI

Demo

Single-window
(The top window shows the current frame's local variables)

TUI.mov

Multi-window

TUI.-.stacked.view.mov

Specs

These specs will be improved in the future.

When disabled (default)

It doesn't affect any existing funcionality.

When enabled with tui command

The screen will be separated into top frame and the REPL
- The top frame
  - Is not scrollable
  - Has one window to display current frame's source code (src) or local variables (locals).
  - Has a fixed height of 15 lines (13 lines for content)
    - I'll make it configurable once this has been accepted
- The REPL
  - Is not scrollable either
Can be disabled with tui off

Future Plans

More window types

breakpoints - the list of registered breakpoints
tracers - the list of activated tracers
trace - tracers' output will be directed here
threads
frames - nearby frames (it's unlikely we'll be able to show all frames)

Multi-window support

tui on [w1:h1,w2:h2] opens windows with specified types. We'll start from stack layout. Example: tui on src:20,locals:10

----------
|  src   |
|        |
----------
| locals |
----------
|  REPL  |
----------

RUBY_DEBUG_TUI_WINDOWS=w1:h1,w2,h2 - configures default windows.
RUBY_DEBUG_TUI_HEIGHT=15 - configures the default window height.

Scroll support

We can add scroll support to both the REPL and the top frame. But probably only command or key-binding scrolling at first.

Implementation notes

Additional data channel

TUI requires frequent data exchange between the Session and ThreadClient. More specifically:

Session needs to tell TC what type of the data it needs with additional information like how many lines is expected.
TC needs to give the Session requested TUI data, like local variables.

For 2), we already pass @internal_info in every TC -> Session events so we can use it directly.

However, we currently don't have anything for 1). So in this PR I added change to let every Session -> TC request also carry a metadata payload. It's a Hash that can carry some general information, like TUI info. I think this can also help simplify the implementation of #422.

Separate frame data's collection/display logic

Currently, frame data like file source, local variables...etc. are printed during collection. This is enough for the current local console but is not convenient when we need to process the data differently, like DAP server or the TUI. So I also extracted get_* methods (return collections) from some of the show_* methods (show collections). Part of this change was done in #465 and merged here and can be merged it separately.

(I know technically puts in ThreadClient doesn't "print" the data immediately. But it's hard to get the collection back from the @output variable as a collection anyway.)

TODO: Reduce screen refreshing

Currently, screen is refreshed on each UI#puts call. So lines.each {|l| @ui.puts(l)} will refresh screen multiple times. If we can change that into @ui.puts(lines), we can avoid unnecessary refreshes. It won't be a huge change (mainly here and maybe a few other places), but I want to wait for the general design is approved before making further changes.

st0012 · 2022-01-27T10:39:13Z

@ko1 this implementation requires some extractions made in #465, so it'd be great if we can finish its discussion.

ono-max · 2022-02-01T07:13:43Z

Because I've never seen Expected the debuggee program to finish. as random errors, this PR's changes may cause failing tests.

I apologize for giving you detailed informations.

https://github.com/ruby/debug/runs/5010975029?check_suite_focus=true

st0012 · 2022-02-01T09:33:25Z

@ono-max Yeah it's weird but I don't see how this PR can cause the detaching test failure. The DAP-related changes are:

Method extractions like get_frame and collet_locals, which are taken from Unify frame and locals collection implementation between ThreadClient and servers #465. But I don't see that PR's tests failed. And it'd be weird for them to affect detaching as well.
Replacing tc << with the new request_tc interface. Since this is a low level change, if it has any issue it'll fail all DAP tests instead of just the one.

ko1 · 2022-02-05T19:06:51Z

I read your patch.

we can introduce metadata or some more communication path between Session <-> TC
Also we can accept to introduce getters instead of printers (show_... methods).
Now your patch hard codes TUI mode into sources, and I don't think it is good idea for maintenance. So current patch is difficult to accept. Could you design extended general (extensible) mechanism at first?

Others:

it is only for locals. Can we extend it on remote mode? I don't want to introduce difference between local and remote REPL as possible.
Where the stdout/err outputs?

st0012 · 2022-02-06T00:07:59Z

it is only for locals. Can we extend it on remote mode? I don't want to introduce difference between local and remote REPL as possible.

I can explore it and it should be possible. But because it requires redrawing the screen repeatedly, the result won't be good in remote consoles under current implementation (with bigger overhead to transfer data). I prefer to wait until optimizations like Reduce screen refreshing is implemented.

Another reason to wait would be to have a more complete test framework (or helpers) for TUI.

Where the stdout/err outputs?

Do you mean we should preserve the stdout/err output after the debug session is finished?

ko1 · 2022-02-21T18:49:56Z

Do you mean we should preserve the stdout/err output after the debug session is finished?

My question is, what happens when the program prints something to stdout/err?

st0012 · 2022-03-02T18:37:05Z

My question is, what happens when the program prints something to stdout/err?

Currently, program's output to stdout/err won't be displayed because they don't trigger the screen refresh. I'm still finding the least impactful way to pipe program output to the console screen.

st0012 · 2022-03-19T22:38:46Z

@ko1 I've added stderr/stdout support. I've tested these scenarios in the demo:

stdout from user program
stdout from debugger console
stderr from debugger console
stdout resumed after TUI is turned off

2022-03-19.22.34.35.mov

st0012 · 2022-03-19T22:41:50Z

lib/debug/local.rb

+    def deactivate
+      super
+      clear_screen!
+      $stdout.reopen(@original_stdout)


This will reconnect the $stdout object to the original stdout object's stream. So both STDOUT and $stdout will be resumed.

st0012 · 2022-03-19T22:43:40Z

lib/debug/local.rb

+      $stdout.define_singleton_method(:write) do |*args|
+        original_stdout.write(*args)
+        tui_console.puts(args.join)
+      end


This is to listen to stdout's printing events and reflect the message on the TUI window as well. The patch will be cleared after #deactivate is called (see below).

st0012 · 2022-03-19T22:44:50Z

lib/debug/local.rb

+
+      tui_console = self
+      original_stdout = @original_stdout
+      original_stderr = @original_stderr


Locals are for referencing from define_singleton_method (can't use ivar there), and ivars are for referencing from #deactivate.

st0012 · 2022-03-19T22:45:22Z

lib/debug/local.rb

+      true
+    end
+
+    def store_prev_line(line)


This is mainly for storing the user input line.

lib/debug/session.rb

lib/debug/thread_client.rb

ko1 · 2022-03-20T22:06:35Z

what the plan do you want to extend it with more information?
do you plan to extend to accept key combination like gdb's TUI mode?
Could you squash commits if the commits can be merged semantically? It will help me to review them.
I couldn't review all code, so I need to more time for it.

Minor comments:

I tried with the script:

# target.rb

def rec n
  rec n - 1 if n > 0
  a = b = c = d = e = f = g = h = i = j = k = l = m = n = o = p = q = r = s = t = u = nil
  bb
end

rec 100

I can't see the src pane. any setting is needed? I can see the lvars pane and prompt
When there are many local variables, it seems to show vars as possible.

when the window size is changed, the output seems broken.

I didn't use get_xxx terminology in thread_client.rb so if we have another good name, it seems better (but I couldn't find out another name).

st0012 · 2022-03-20T22:18:05Z

First of all, thank you for taking the time reviewing this PR 🙏

what the plan do you want to extend it with more information?

at the very least, I hope we support these:

breakpoints - the list of registered breakpoints
tracers - the list of activated tracers
trace - tracers' output will be directed here
threads
frames - nearby frames (it's unlikely we'll be able to show all frames)

(I've also listed them in the description)

and users should be able to use tui on threads,src to open TUI with the selected windows.

do you plan to extend to accept key combination like gdb's TUI mode?

yes. I'm not sure how to prioritize it with vertical window split though. I think we need some discussion around it.

Could you squash commits if the commits can be merged semantically? It will help me to review them.

of course. will let you know when done.

I tried with the script:

# target.rb

def rec n
  rec n - 1 if n > 0
  a = b = c = d = e = f = g = h = i = j = k = l = m = n = o = p = q = r = s = t = u = nil
  bb
end

rec 100

I can't see the src pane. any setting is needed? I can see the lvars pane and prompt

you can use tui on src to switch to src pane, or tui on locals,src to open both panes.

When there are many local variables, it seems to show vars as possible.

do you mean it doesn't show as many locals as possible? that's because currently the window height is fixed.

I think the best way to address this issue is to have a key-binding for scrolling. I'll write a plan for that.

when the window size is changed, the output seems broken.

yes that's a known issue.

I didn't use get_xxx terminology in thread_client.rb so if we have another good name, it seems better (but I couldn't find out another name).

how about collect_xxx?

st0012 · 2022-03-20T23:01:15Z

@ko1 I've reduced the number of commits to just 4. and the first 2 commits can actually be merged separately (that'll help reducing conflicts).

ko1 · 2022-03-20T23:06:41Z

(I've also listed them in the description)

Sorry I missed it.

do you plan to extend to accept key combination like gdb's TUI mode?

yes. I'm not sure how to prioritize it with vertical window split though. I think we need some discussion around it.
...
I think the best way to address this issue is to have a key-binding for scrolling. I'll write a plan for that.

Yes I'm considering how to scroll the window to access more information like editors. If the window is small and information are many (for example ivar, frames, srcs) the importance of scroll is high.

@ivar

We should pass tc (ThreadClient)'s data in the Session in a uniformed way: either all with ivar or all with local variable. And since the local variable approach has been rejects (ruby#293), we may want to go with @ivar. This change for unification is needed for later changes.

st0012 · 2022-04-09T11:17:59Z

@ko1 I've checked Ruby TUI applications like ruby_jard and Ruco to see how they handle keyboard input. I think ruby_jard implements input capturing directly from IO, while Ruco is built on top of curses.

So I think to support key-binding, we can't use reline's readline method anymore and need to implement a lower level of input handling. This can affect how features like autocompletion are supported under different modes.

IMO, the principles should be

We need to support the same set of input features (e.g. hints and autocompletion) in normal/TUI mode.
They can't have on different implementations (reline vs a completely custom one) for the sake of maintenance.
For whatever implementation we choose, it shouldn't rely on any 3rd party gem or C-library.
- So curses is off of the table.

That leaves us with these options:

Use custom implementation to replace reline for both normal and TUI mode.
- This clearly requires a lot of work.
Support key-binding detection in reline that also works with the readline method.
- I think it's the ideal option but I'm not sure if that's in the scope of reline.

Do you have any thoughts on this?

ko1 · 2022-04-17T16:36:38Z

Use custom readline on TUI mode? (simple one)

is another idea.

Controlling terminal is very difficult task so I don't recommend you to dive into this area.
Also maybe I can't accept complex solution because I don't want to maintain it.

I think it is more reasonable to make a CLI DAP client in another gem and connect with debug.gem.

st0012 · 2022-04-17T17:35:20Z

I agree that the debugger shouldn't maintain complicated terminal controlling mechanism. I'm not too keen to dive into that now either.

And I just found that GDB uses ncurses to implement TUI instead of building everything from scratch. So perhaps bringing the curses gem in would be a possible solution here.

But if that's not acceptable, I don't think building another UI on top of DAP is what I'd pursuit. My goal for this TUI are:

Convenient to use: just a tui command can spin it up.
Operations are still performed from REPL and can directly see stdout/stderr.

Which would be hard to achieve with DAP's architecture.

st0012 · 2022-04-17T21:17:37Z

@ko1 Since the chance of this PR getting merged is small, I won't proceed on it anymore. Do you think the first 2 refactoring commits worth merging? Otherwise I'll just close it.

ko1 · 2022-04-21T04:20:07Z

Could you make another PR for that?

st0012 · 2022-04-21T17:55:46Z

@ko1 Of course, here's the PR: #628

st0012 force-pushed the tui branch 3 times, most recently from b66a47c to 3dc8521 Compare January 28, 2022 14:20

st0012 marked this pull request as ready for review January 28, 2022 17:10

st0012 force-pushed the tui branch 3 times, most recently from 605e44d to 3412e69 Compare January 31, 2022 18:59

st0012 force-pushed the tui branch from 3412e69 to 53f3160 Compare February 1, 2022 09:20

st0012 force-pushed the tui branch 2 times, most recently from c7dde90 to cf604c2 Compare February 5, 2022 23:29

st0012 force-pushed the tui branch 2 times, most recently from 5340165 to ba57149 Compare February 6, 2022 22:57

st0012 mentioned this pull request Feb 14, 2022

Add config option to skip confirmation on quit #525

Closed

st0012 force-pushed the tui branch from ba57149 to 32d1527 Compare February 21, 2022 21:52

st0012 force-pushed the tui branch from 32d1527 to c01dcee Compare March 1, 2022 23:33

st0012 force-pushed the tui branch 5 times, most recently from 9f0af2d to 629f1cc Compare March 19, 2022 22:32

st0012 commented Mar 19, 2022

View reviewed changes

ko1 reviewed Mar 20, 2022

View reviewed changes

lib/debug/session.rb Outdated Show resolved Hide resolved

ko1 reviewed Mar 20, 2022

View reviewed changes

lib/debug/thread_client.rb Outdated Show resolved Hide resolved

st0012 force-pushed the tui branch 2 times, most recently from b4dbfe2 to a68c655 Compare March 20, 2022 22:36

st0012 mentioned this pull request Mar 20, 2022

Add special local _ to store previously evaluated value #422

Closed

st0012 force-pushed the tui branch 4 times, most recently from c639929 to cd922a1 Compare March 20, 2022 22:58

st0012 force-pushed the tui branch from cd922a1 to 7556d03 Compare March 27, 2022 13:58

st0012 added 4 commits March 30, 2022 20:53

Extract get_src method

01faab5

Support tui command

d193211

Support printing STDOUT and STDERR to TUI REPL

d733047

st0012 force-pushed the tui branch from 7556d03 to d733047 Compare March 30, 2022 19:53

st0012 mentioned this pull request Apr 21, 2022

Refactor Session and ThreadClient #628

Merged

st0012 closed this Apr 27, 2022

Text-based UI support #503

Text-based UI support #503

Uh oh!

Conversation

st0012 commented Jan 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Usage

Demo

Specs

Future Plans

More window types

Multi-window support

Scroll support

Implementation notes

Additional data channel

Separate frame data's collection/display logic

TODO: Reduce screen refreshing

Uh oh!

st0012 commented Jan 27, 2022

Uh oh!

ono-max commented Feb 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 commented Feb 1, 2022

Uh oh!

ko1 commented Feb 5, 2022

Uh oh!

st0012 commented Feb 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ko1 commented Feb 21, 2022

Uh oh!

st0012 commented Mar 2, 2022

Uh oh!

st0012 commented Mar 19, 2022

Uh oh!

st0012 Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

st0012 Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

st0012 Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

st0012 Mar 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ko1 commented Mar 20, 2022

Uh oh!

st0012 commented Mar 20, 2022

Uh oh!

st0012 commented Mar 20, 2022

Uh oh!

ko1 commented Mar 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ko1 commented Apr 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

st0012 commented Apr 17, 2022

Uh oh!

st0012 commented Apr 17, 2022

Uh oh!

ko1 commented Apr 21, 2022

Uh oh!

st0012 commented Apr 21, 2022

Uh oh!

Uh oh!

st0012 commented Jan 27, 2022 •

edited

Loading

ono-max commented Feb 1, 2022 •

edited

Loading

st0012 commented Feb 6, 2022 •

edited

Loading

ko1 commented Mar 20, 2022 •

edited

Loading

st0012 commented Apr 9, 2022 •

edited

Loading

ko1 commented Apr 17, 2022 •

edited

Loading