[Lldb-commits] [PATCH] D104395: [LLDB][GUI] Add initial forms support

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

> did you consider implementing each field as a Window?

My first implementation was actually done completely with fields as sub windows.
However, two things deterred me from taking that approach.

- While sub-windows in ncurses are mostly sub-spaces with their own local coordinate system and cursor, which would be a good abstraction to use for fields, their implementation in the Window class seemed more involved than needed, with panels for drawing even. So I thought maybe they are not supposed to be used in that manner. I also though about introducing a type that is more lightweight and suitable for this kind of thing, but that didn't seem worth it at the moment. I will come back to this in a future patch though.
- Field construction doesn't have access to the possible parent window needed to create the sub-window, which means we will either have to pass the window around during construction or we will have to conditionally construct it the first window draw. Neither of those sounded like a good method to me.

The field drawing code used some special coordinate computation anyways, so
sub-spacing there was natural to do. Moreover, I don't think the duplicated code
with the Window is a lot, a non-border box drawing function is still useful to
have so I wouldn't think of it as a duplicate code.

> how complex it the scrolling code going to be?

I will try to answer this question now to make it easier to decide what we need to
do. But I basically wanted full flexibility when it comes to drawing, with possibly
multiple row fields like the choices field and multi-column forms. The forms ncurses
library uses pages instead of scrolling to make this easier, so they would put the
most important fields in the first page and more fields in later pages. But I will let
you know what I think after I look into scrolling first.



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:962
+
+  int GetContentLength() { return (int)m_content.length(); }
+
----------------
clayborg wrote:
> Does this need to be an integer? Can we return "size_t"?
The compiler kept throwing sign comparison warnings in expressions like `m_cursor_position < m_content.length()`, so I just casted to an integer. How should I properly handle this?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1120
+    switch (key) {
+    case ' ':
+      ToggleContent();
----------------
clayborg wrote:
> enter or return key as well? or is that saved for submit in the main window?
Sure, we can do that. Enter is only handled if the button is selected.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1442
+  // The index of the selected field. This can be equal to the number of fields,
+  // in which case, it denotes that the button is selected.
+  int m_selected_field_index;
----------------
clayborg wrote:
> The submit button?
Yes.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104395/new/

https://reviews.llvm.org/D104395