Page MenuHomePhabricator

[LLDB][GUI] Add initial forms support
ClosedPublic

Authored by OmarEmaraDev on Jun 16 2021, 9:13 AM.

Details

Summary

This patch adds initial support for forms for the LLDB GUI. The currently
supported form elements are Text fields, Integer fields, Boolean fields, Choices
fields, File fields, Directory fields, and List fields.

A form can be created by subclassing FormDelegate. In the constructor, field
factory methods can be used to add new fields, storing the returned pointer in a
member variable. One or more actions can be added using the AddAction method.
The method takes a function with an interface void(Window &). This function will
be executed whenever the user executes the action.

Example form definition:

class TestFormDelegate : public FormDelegate {
public:
  TestFormDelegate() {
    m_text_field = AddTextField("Text", "The big brown fox.");
    m_file_field = AddFileField("File", "/tmp/a");
    m_directory_field = AddDirectoryField("Directory", "/tmp/");
    m_integer_field = AddIntegerField("Number", 5);
    std::vector<std::string> choices;
    choices.push_back(std::string("Choice 1"));
    choices.push_back(std::string("Choice 2"));
    choices.push_back(std::string("Choice 3"));
    choices.push_back(std::string("Choice 4"));
    choices.push_back(std::string("Choice 5"));
    m_choices_field = AddChoicesField("Choices", 3, choices);
    m_bool_field = AddBooleanField("Boolean", true);
    TextFieldDelegate default_field =
        TextFieldDelegate("Text", "The big brown fox.");
    m_text_list_field = AddListField("Text List", default_field);

    AddAction("Submit", [this](Window &window) { Submit(window); });
  }

  void Submit(Window &window) { SetError("An example error."); }

protected:
  TextFieldDelegate *m_text_field;
  FileFieldDelegate *m_file_field;
  DirectoryFieldDelegate *m_directory_field;
  IntegerFieldDelegate *m_integer_field;
  BooleanFieldDelegate *m_bool_field;
  ChoicesFieldDelegate *m_choices_field;
  ListFieldDelegate<TextFieldDelegate> *m_text_list_field;
};

Diff Detail

Event Timeline

OmarEmaraDev created this revision.Jun 16 2021, 9:13 AM
OmarEmaraDev requested review of this revision.Jun 16 2021, 9:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2021, 9:13 AM

An example form:

class TestFormDelegate : public FormDelegate {
public:
  TestFormDelegate() {
    m_path_field = AddTextField("Path", 20, Point(4, 2), "/tmp/a.out");
    m_number_field = AddIntegerField("Number", 20, Point(4, 5), 5);
    m_bool_field = AddBooleanField("Boolean", Point(5, 8), true);
    std::vector<std::string> choices;
    choices.push_back(std::string("Choice 1"));
    choices.push_back(std::string("Choice 2"));
    choices.push_back(std::string("Choice 3"));
    choices.push_back(std::string("Choice 4"));
    choices.push_back(std::string("Choice 5"));
    m_choices_field = AddChoicesField("Choices", 20, 5, Point(30, 2), choices);
  }

  bool FormDelegateSubmit() override {
    std::string path = m_path_field->GetText();
    int number = m_number_field->GetInteger();
    bool boolean = m_bool_field->GetBoolean();

    // Do something with the data.

    if (everything_is_correct)
      return true;

    if (there_is_an_error) {
      m_has_error = true;
      m_error =
      std::string("An error occured! Check the validity of the inputs.");
      return false;
    }
  }

protected:
  TextFieldDelegate *m_path_field;
  IntegerFieldDelegate *m_number_field;
  BooleanFieldDelegate *m_bool_field;
  ChoicesFieldDelegate *m_choices_field;
};

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
lldb/source/Core/IOHandlerCursesGUI.cpp
389–395

move this below function below PutCStringTruncated so that PutCStringTruncated doesn't appear to be edited.

2027

Do we need to use PutCStringTruncated here just in case?

2239–2241

"m_content(content):" will crash if content is NULL.

2266

Does this need to be an integer? Can we return "size_t"?

2364

Return const reference to avoid copy. If use wants to copy it, they can assign to a local

2412

We could use ACS_DIAMOND or ACS_BULLET? Check it out and see how it looks?

2424

enter or return key as well? or is that saved for submit in the main window?

2746

The submit button?

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.

lldb/source/Core/IOHandlerCursesGUI.cpp
2266

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?

2424

Sure, we can do that. Enter is only handled if the button is selected.

2746

Yes.

  • Remove PutCStringTruncatedWidth, use a character limit instead.
  • Handle review.
OmarEmaraDev added inline comments.Jun 17 2021, 9:31 AM
lldb/source/Core/IOHandlerCursesGUI.cpp
2412

What do you think?

  • Always scroll left on removing a character
clayborg requested changes to this revision.Jun 18 2021, 12:45 PM

Lets try the diamond character for the boolean stuff unless anyone has any objections. Maybe handle a few more keys for the boolean field as suggested in the comments. This will be good to go after these changes!

lldb/source/Core/IOHandlerCursesGUI.cpp
2268

It is fine to leave cast to int if this is causing compiler warnings. Many things are integers in the curses API.

2412

I like the diamond one personally. Looks nice and clean

2429

maybe handle '1' to set m_content to true and '0' to set to false? Could also handle 't' for true and 'f' for false?

This revision now requires changes to proceed.Jun 18 2021, 12:45 PM
  • Add form pages.
  • Handle review.
clayborg accepted this revision.Jun 18 2021, 1:33 PM

Looks good!

This revision is now accepted and ready to land.Jun 18 2021, 1:33 PM

@clayborg I tried implementing scrolling mechanisms as suggested. My first trial essentially defined a "visible area" rectangle which gets updated with every time the selection changes, then when it comes to drawing, each field checks if it is completely contained in the visible area and draws itself with an offset that we get from from the visible area origin. This worked, but fields that spans multiple columns can completely disappear leaving mostly no fields on the window, so it worked in most cases, but not all. My second trial was about drawing to an ncurses pad that is large enough to fit all contents, then the window is refreshed from that pad. This used manual ncurses calls because we don't support pads at the moment, so I scratched that for now. I think support for pads would be good for those kind of applications in the future. I plan to work on a proposal that would include support for pads and lightweight subwindows, I will detail that later.

I eventually opted to create "pages" for the forms. Basically, during form construction, if the number of fields is large and can't fit in the window, the user can call NewPage() to create a new page, all fields after that call gets added to the new page. As the user selects fields, the form can scroll to different pages. This is similar to the ncurses form scrolling functionality. Visual indicator for the current pages and the active page is added to the form if there is more than one page. I hope this solution is satisfactory.

@clayborg I tried implementing scrolling mechanisms as suggested. My first trial essentially defined a "visible area" rectangle which gets updated with every time the selection changes, then when it comes to drawing, each field checks if it is completely contained in the visible area and draws itself with an offset that we get from from the visible area origin. This worked, but fields that spans multiple columns can completely disappear leaving mostly no fields on the window, so it worked in most cases, but not all. My second trial was about drawing to an ncurses pad that is large enough to fit all contents, then the window is refreshed from that pad. This used manual ncurses calls because we don't support pads at the moment, so I scratched that for now. I think support for pads would be good for those kind of applications in the future. I plan to work on a proposal that would include support for pads and lightweight subwindows, I will detail that later.

Yeah, that is why I was suggesting that we use the list view for fields as it makes scrolling much easier as I really like your graphical approach that you have, it just means scrolling logic is much more complicated.

I eventually opted to create "pages" for the forms. Basically, during form construction, if the number of fields is large and can't fit in the window, the user can call NewPage() to create a new page, all fields after that call gets added to the new page. As the user selects fields, the form can scroll to different pages. This is similar to the ncurses form scrolling functionality. Visual indicator for the current pages and the active page is added to the form if there is more than one page. I hope this solution is satisfactory.

That will work as long as you are wanting to implement all of the required features. I personally like to keep things simple, but I'm not against seeing a nicer solution.

It it were me I would probably make a ListDelegate that can manage a list of delegates where each field would be a delegate and each field would display on one or more line kind of like I detailed in ASCII art. This would make scrolling really easy to implement in an abtract ListDelegate kind of like how we did for the TreeDelegate. For example the "Path" for the executable would take up one line in the list window part of the form. A "Arguments" field for a Process launch dialog could take up one or more lines depending on how many entries the user typed. Same things for the environment variables field we will need. So think about all of the fields types we will need and think about how easy they all will be to implement as we add new things. The fields I can think of include:

  • file path (for either a file or directory where constructor of the field specifies which one and also constructor would say if the path needs to exist (which would be true for an executable for a target, but not for a path substitution path)
    • target executable as a file that must exist
    • current working directory as a directory that must exist
    • script path to a file that must exist
  • list field (for things like arguments or environment variables where the user specifies one or more items. Each item in the list could use a FieldDelegate so the list can be a list of anything)
    • arguments
    • environment variables
  • string field (with optional validation on the string when it loses focus (This could be used to implement the "file path" above where the user types something and then hits tab to select another field, but we don't let the focus change because the path isn't valid or it is expecting a file and a path to a directory is in the field at the moement)
  • pulldown to select something. If this was done on a single line, then we can pop up a menu from the menu bar to allow selection
  • boolean value
  • integer
    • for pid selection for attach dialog where we need to validate the process ID that the user enters is valid
    • for <port> value for a setting that uses sockets

So just think about all the ways we will want to use each of these items and make sure you are willing to support all that is needed to make these happen. If you prefer the more GUI like approach, it will be good for you to understand how much work or support this will involve as new things are added.

  • Remove Field type and use FieldDelegate directly

Just a few questions on how buttons should be handled since we are making new sweeping changes! See inlined comments and let me know what you think

lldb/source/Core/IOHandlerCursesGUI.cpp
2704–2713

Should we put a cancel button and "Submit" button in the window's bottom line to save space? Any errors could be popped up with a modal dialog when the user tries to submit. The help for a form could specify that "Esc" will cancel the dialog and we wouldn't have to write it into the bottom of the window.

2746

As mentioned above, it might be cleaner to put the "Cancel" and "Submit" into the bottom line of the dialog box to save space?

2758

Maybe we want to add a ButtonDelegate class and each window can add a list of button delegates. Then the window draw code would know to draw them?

Since we are still working on this diff. I will add the other form functionality I have been working on here as well if you don't mind.

lldb/source/Core/IOHandlerCursesGUI.cpp
2704–2713

Moving the buttons to the border seems like a good idea to me, so I will do that.

However, I think we should generally avoid double modals, as it is frowned upon in the UX world. So showing errors in a modal is probably not a good idea.

I am currently reworking some aspects of forms to be more dynamic. The new forms supports full vertical scrolling, field validation, per field errors, and list of fields. So errors can be incorporated nicely in the new design.

2758

I guess it depends if we really need this flexibility. I can't think of a form that would need (or should have) more than confirmation and cancellation. Can you give an example where a different configuration would be needed?

Since we are still working on this diff. I will add the other form functionality I have been working on here as well if you don't mind.

Sure thing!

lldb/source/Core/IOHandlerCursesGUI.cpp
2713

Sounds good! Just want to make sure we do the right thing. I look forward to seeing what you come up with

2758

Just thinking of any window that requires buttons. Forms will have "Cancel" and "OK" or "Submit". Maybe the "Submit" should be a constructor argument so that the target create form can say "Create Target" instead of "Submit"?

We might end up doing modal dialog windows for some settings or validations of fields that pop up a OK/Cancel dialog, or Abort/Retry/Ingore, or a progress dialog that might have a Cancel button. Each of these is a window. Just trying to think ahead of what we will need.

A file picker window would be pretty neat as well, so that when a user selects the "Path" field, maybe a window pops up and allows selecting the file using a nice dialog box, and that maybe have buttons too.

  • Add Surface type.
  • Add Pad and SubPad types.
  • Implement dynamic scrolling.
  • Implement per-field validation and error messages.
  • Implement File field.
  • Implement Directory field.
  • Implement List field.
  • Refactor field drawing.
OmarEmaraDev planned changes to this revision.Jun 24 2021, 2:47 PM

Not available in this patch yet:

  • Global error messages.
  • Contextual scrolling.
  • Action bar.
  • Auto completion.
  • Rewrite internal field navigation.
  • Rewrite form window drawing. Form delegate no longer have drawing routines.
  • Add global error messages.
  • Add action bar. Form delegate can now define as many arbitrary actions as needed.
  • Make action button scrollable.
  • Add support for composite fields.
  • Add backward tab navigation.
This revision is now accepted and ready to land.Tue, Jun 29, 2:12 PM
OmarEmaraDev planned changes to this revision.Tue, Jun 29, 2:18 PM
  • Scrolling was temporarily removed from the patch. It was causing issues with fields that change in size. I will reimplement it as contextual scrolling directly.
  • Action buttons weren't moved to the window border as discussed. The window border is already highlighted when the form is active, which makes highlighting and navigating fields not user friendly. Action buttons are now scrollable though, which solves the issue of space.
  • Add contextual scrolling support.
This revision is now accepted and ready to land.Wed, Jun 30, 1:07 PM

Full example:

Example API definition for the above example:

class TestFormDelegate : public FormDelegate {
public:
  TestFormDelegate() {
    m_text_field = AddTextField("Text", "The big brown fox.");
    m_file_field = AddFileField("File", "/tmp/a");
    m_directory_field = AddDirectoryField("Directory", "/tmp/");
    m_pid_field = AddIntegerField("Number", 5);
    std::vector<std::string> choices;
    choices.push_back(std::string("Choice 1"));
    choices.push_back(std::string("Choice 2"));
    choices.push_back(std::string("Choice 3"));
    choices.push_back(std::string("Choice 4"));
    choices.push_back(std::string("Choice 5"));
    m_choices_field = AddChoicesField("Choices", 3, choices);
    m_bool_field = AddBooleanField("Boolean", true);
    TextFieldDelegate default_field =
        TextFieldDelegate("Text", "The big brown fox.");
    m_text_list_field = AddListField("Text List", default_field);

    AddAction("Submit", [this](Window &window) { Submit(window); });
  }

  void Submit(Window &window) { SetError("An example error."); }

protected:
  TextFieldDelegate *m_text_field;
  FileFieldDelegate *m_file_field;
  DirectoryFieldDelegate *m_directory_field;
  IntegerFieldDelegate *m_pid_field;
  BooleanFieldDelegate *m_bool_field;
  ChoicesFieldDelegate *m_choices_field;
  ListFieldDelegate<TextFieldDelegate> *m_text_list_field;
};
OmarEmaraDev requested review of this revision.Wed, Jun 30, 1:13 PM
clayborg accepted this revision.Thu, Jul 1, 10:04 PM

This looks good. Lets get this in so we can start using it.

This revision is now accepted and ready to land.Thu, Jul 1, 10:04 PM
OmarEmaraDev edited the summary of this revision. (Show Details)Fri, Jul 2, 7:32 AM
This revision was landed with ongoing or failed builds.Wed, Jul 7, 8:18 AM
This revision was automatically updated to reflect the committed changes.