[Lldb-commits] [PATCH] D108545: [LLDB][GUI] Add initial searcher support

[Greg Clayton via Phabricator via lldb-commits]

clayborg added a comment.

In D108545#2962017 <https://reviews.llvm.org/D108545#2962017>, @OmarEmaraDev wrote:

> @clayborg I was planning on getting to field completion later as part of a global "context window" feature. There are reasons why I need this as a full separate window for now. Imminently, I am creating an operator that changes the file in the sources window so that breakpoints can be inserted manually. So once the user press Ctrl+F, a search window will appear where the user can choose a source file to navigate to.

Pressing CTRL+F can and should bring up a dialog window right? The user will search for something, and finalize with some action. So this seems like perfect use for dialog windows and the fields and forms you have already created.

> This will require a custom searcher, and not the common searcher implemented above, which I will add in a following patch. What do you think?

See comments above, but I do think this is a perfect usage for dialog windows:

- user presses CTRL+F and dialog comes up
  - user selects a file and presses OPT+ENTER to finalize the action and source view updates
  - user presses escape and dismisses the dialog



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3592
+        m_selected_match(0), m_first_visible_match(0) {
+    ;
+  }
----------------
OmarEmaraDev wrote:
> OmarEmaraDev wrote:
> > clayborg wrote:
> > > Should we be doing just like any other dialog that we have created and be constructing these items on the fly?
> > > 
> > > ```
> > > m_text_field = AddTextField("Search", "", true);
> > > ```
> > > It seems the matches could use a modified ChoicesField for the matches? Are you currently drawing the choices (matches) manually?
> > `AddTextField` is part of form delegates, I don't think implementing this as a form is a good idea as they are functionally distinct.
> > 
> > I am drawing choices manually because the duplicated code is not really a lot and I like to be able to control the style of the drawing. But I guess we can reimplemented that using a choices field.
> One thing to note is that text representation of matches are lazily computed in this window but not in the choices field, which gives an advantage. We can probably add support for lazy computation in the choice field, but it may not be worth it at the moment.
But this does seem like the perfect use for a dialog window: user presses CTRL+F, dialog shows up and allows user to select something, and then press OPT+ENTER to update the source view. Is there a reason you think this should be a window and not a dialog?

If we re-use the ChoicesField, you can just update the choices with a delegate any time the choices should change and repaint the windows right?


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3731
+  SearcherDelegateSP m_delegate_sp;
+  TextFieldDelegate m_text_field;
+  // The index of the currently selected match.
----------------
OmarEmaraDev wrote:
> clayborg wrote:
> > Is this a new text field itself, or designed to be a reference to an existing text field? 
> A new instance of a text field.
Yeah, the main questions is if this should be a FormDelegate or not. See other comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108545