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

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

Looks very nice.

A few things to consider here, and I am open to everyone's opinion here, did you consider implementing each field as a Window? It seems that a lot of the window code is duplicated, DrawBox for one. I know we would need to make modifications to the Window class if this, but just something to think about. It might be more trouble that it is worth.

Another question I have is if we want a box around each field and do we want the user to be able to select the exact location of each item? Right now we can make a nice UI with almost window like fields, but I worry that if we have a lot of fields and they don't fit in the window, how complex it the scrolling code going to be? Another way would be to make a list of fields. And you add them in the order you want them to show up in the list. There could be no boxes around each field in this case and the window could look more like a list in this case. I will try and show with some ASCII art what I was thinking:

  /--------------------------------------------------------------------\
  | Path: /tmp/a.out                                                   |
  | Number: 1234                                                       |
  | Boolean: true                                                      |
  | Choices: Choice 1                                                  |
  |--------------------------------------------------------------------|
  |                              [ SUBMIT ]                            |
  \--------------------------------------------------------------------/

This might make it easier to implement scrolling if there are too many entries to fit on the screen.

Future things we can add would include:

- a field grouping field that would not take any input, but it would allow fields to be grouped into sections
- subclasses of the text field that allow for things like paths which might auto complete and allow only files or directories to be selected based on constructor args
- an array of a specific kind of fields, like for program arguments that would take a FieldDelegate and allow for a list to be created



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:432-440
+  void PutCStringTruncatedWidth(int right_pad, const char *s, int width,
+                                int len = -1) {
+    int bytes_left = width - GetCursorX();
     if (bytes_left > right_pad) {
       bytes_left -= right_pad;
       ::waddnstr(m_window, s, len < 0 ? bytes_left : std::min(bytes_left, len));
     }
----------------
move this below function below PutCStringTruncated so that PutCStringTruncated doesn't appear to be edited.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:713
+    PutChar('[');
+    PutCString(title);
+    PutChar(']');
----------------
Do we need to use PutCStringTruncated here just in case?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:935-937
+      : m_label(label), m_width(width), m_origin(origin), m_content(content),
+        m_cursor_position(0), m_first_visibile_char(0) {
+    assert(m_width > 2);
----------------
"m_content(content):" will crash if content is NULL.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:962
+
+  int GetContentLength() { return (int)m_content.length(); }
+
----------------
Does this need to be an integer? Can we return "size_t"?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1060
+  // Returns the text content of the field.
+  std::string GetText() { return m_content; }
+
----------------
Return const reference to avoid copy. If use wants to copy it, they can assign to a local


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1108
+      window.AttributeOn(A_REVERSE);
+    window.PutChar(m_content ? 'X' : ' ');
+    if (is_active)
----------------
We could use ACS_DIAMOND or ACS_BULLET? Check it out and see how it looks?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1120
+    switch (key) {
+    case ' ':
+      ToggleContent();
----------------
enter or return key as well? or is that saved for submit in the main window?


================
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;
----------------
The submit button?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395