[Lldb-commits] [PATCH] D108944: [LLDB][GUI] Add source file searcher

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@clayborg Did you take a look at this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108944

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108944: [LLDB][GUI] Add source file searcher

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

An example typical workflow for navigating to a file and adding a breakpoint.

F18748669: 20210830-221617.mp4 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108944

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108944: [LLDB][GUI] Add source file searcher

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: lldb-commits, sstefan1.
Herald added a project: LLDB.

This patch adds a new searcher to find source files and a new shortcut
to the sources window (F) that changes the displayed file to the one
selected in the searcher dialog. The added searcher displays the
smallest unique path that identifies the file for a more compact and
friendly representation.

This patch is not sufficient for the workflow outlined, because the
sources window was not made for that purpose and would need refactoring
to support arbitrary files that are independent of a symbol context. An
attempt at refactoring proved a bit involved, so it will be done later
in a separate patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108944

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -3654,7 +3654,7 @@
   virtual int GetNumberOfMatches() = 0;
 
   // Get the string that will be displayed for the match at the input index.
-  virtual const std::string &GetMatchTextAtIndex(int index) = 0;
+  virtual std::string GetMatchTextAtIndex(int index) = 0;
 
   // Update the matches of the search. This is executed every time the text
   // field handles an event.
@@ -3836,7 +3836,7 @@
 
   int GetNumberOfMatches() override { return m_matches.GetSize(); }
 
-  const std::string &GetMatchTextAtIndex(int index) override {
+  std::string GetMatchTextAtIndex(int index) override {
 return m_matches[index];
   }
 
@@ -3863,6 +3863,116 @@
   StringList m_matches;
 };
 
+class SourceFileSearcherDelegate : public SearcherDelegate, public Searcher {
+public:
+  typedef std::function CallbackType;
+
+  SourceFileSearcherDelegate(Debugger &debugger, CallbackType callback)
+  : m_debugger(debugger), m_callback(callback) {}
+
+  int GetNumberOfMatches() override { return m_matches.GetSize(); }
+
+  bool IsFileNameUnique(const FileSpec &file) {
+auto is_file_name_equal = [&file](const FileSpec &f) {
+  return (&f != &file) && file.FileEquals(f);
+};
+
+FileSpecList::const_iterator begin = m_matches.begin();
+FileSpecList::const_iterator end = m_matches.end();
+return find_if(begin, end, is_file_name_equal) == end;
+  }
+
+  // Find the smallest unique path assuming the file name is not unique.
+  // We start from the file name and progressively prepend the last remaining
+  // path component until we find a path that is unique across matches, that is,
+  // none of the matched files' paths ends with it.
+  std::string FindSmallestUniquePath(const FileSpec &file) {
+FileSpec unique_spec(file.GetFilename().GetStringRef());
+FileSpec remaining = file.CopyByRemovingLastPathComponent();
+
+while (true) {
+  ConstString last_componenet = remaining.GetLastPathComponent();
+  unique_spec.PrependPathComponent(last_componenet.GetStringRef());
+  std::string unique_path = unique_spec.GetPath();
+
+  auto ends_with_path = [&file, &unique_path](const FileSpec &f) {
+std::string path = f.GetPath();
+return (&f != &file) && StringRef(path).endswith(unique_path);
+  };
+
+  FileSpecList::const_iterator begin = m_matches.begin();
+  FileSpecList::const_iterator end = m_matches.end();
+  if (find_if(begin, end, ends_with_path) == end)
+return unique_path;
+
+  if (!remaining.RemoveLastPathComponent()) {
+// We will return before we reach this point, but break in any case.
+break;
+  }
+}
+
+return unique_spec.GetPath();
+  }
+
+  std::string GetMatchTextAtIndex(int index) override {
+const FileSpec &file = m_matches.GetFileSpecAtIndex(index);
+
+// If no other matched file have the same name, then only return the name.
+if (IsFileNameUnique(file))
+  return file.GetFilename().GetStringRef().str();
+
+// If another matched file have the same name, then return the smallest
+// unique path of the file across matches.
+return FindSmallestUniquePath(file);
+  }
+
+  void UpdateMatches(const std::string &text) override {
+m_matches.Clear();
+m_file_name_partial = text;
+if (text.empty())
+  return;
+TargetSP target = m_debugger.GetSelectedTarget();
+SearchFilterForUnconstrainedSearches filter(target);
+filter.Search(*this);
+  }
+
+  void ExecuteCallback(int match_index) override {
+m_callback(m_matches.GetFileSpecAtIndex(match_index));
+  }
+
+  // Searcher implementation.
+
+  lldb::SearchDepth GetDepth() override { return lldb::eSearchDepthCompUnit; }
+
+  Searcher::CallbackReturn SearchCallback(SearchFilter &filter,
+   

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@clayborg I see a number of issues with using forms directly:

- The user will have to bother with form navigation. If we have a text field 
and a choice field, the user will have to switch between them using Tab and 
Shift+Tab in order to explore, rectify, and select a match. This is 
significantly more work then just typing without worry, selecting using arrow 
keys, and executing using enter.
- Not all users will use the OPT+Enter keys, it is likely that once the user 
sees a button that says "Select", the user will try to navigate to that button 
ignoring the tip in the windows border. Which is more work than just pressing 
enter.
- Updating the choices will require that we materialize all choices into text, 
this can work for the common completion searcher, but other searchers will 
require actual processing that might slow down the update process. The searcher 
window only materialize the match that it draws, so it can draw things much 
faster. We could update the choices field to accept a callback to compute its 
contents, but this will complicate the code, so it is worth considering if this 
is really needed.
- Practically all search dialogs work in a manner similar to the searcher 
window, like browser search bar, editor search dialogs, and the like. I don't 
recall seeing a search dialog that is implemented as a form.
- We want the ability to do custom styling. One thing I was considering is 
coloring the matched substring a different color, if we use a choice field, 
then we give up on custom styling.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108545

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3592
+m_selected_match(0), m_first_visible_match(0) {
+;
+  }

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108545

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@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. 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?




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3592
+m_selected_match(0), m_first_visible_match(0) {
+;
+  }

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.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3731
+  SearcherDelegateSP m_delegate_sp;
+  TextFieldDelegate m_text_field;
+  // The index of the currently selected match.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108545

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

An example searcher window for source files:

F18633042: 20210823-141344.mp4 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108545

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new type of reusable UI components. Searcher Windows
contain a text field to enter a search keyword and a list of scrollable
matches are presented. The target match can be selected and executed
which invokes a user callback to do something with the match.

This patch also adds one searcher delegate, which wraps the common
command completion searchers for simple use cases.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108545

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -3558,6 +3558,232 @@
   EnvironmentVariableListFieldDelegate *m_inherited_environment_field;
 };
 
+
+// Searchers
+
+
+class SearcherDelegate {
+public:
+  SearcherDelegate() {}
+
+  virtual ~SearcherDelegate() = default;
+
+  virtual int GetNumberOfMatches() = 0;
+
+  // Get the string that will be displayed for the match at the input index.
+  virtual const std::string &GetMatchTextAtIndex(int index) = 0;
+
+  // Update the matches of the search. This is executed every time the text
+  // field handles an event.
+  virtual void UpdateMatches(const std::string &text) = 0;
+
+  // Execute the user callback given the index of some match. This is executed
+  // once the user selects a match.
+  virtual void ExecuteCallback(int match_index) = 0;
+};
+
+typedef std::shared_ptr SearcherDelegateSP;
+
+class SearcherWindowDelegate : public WindowDelegate {
+public:
+  SearcherWindowDelegate(SearcherDelegateSP &delegate_sp)
+  : m_delegate_sp(delegate_sp), m_text_field("Search", "", false),
+m_selected_match(0), m_first_visible_match(0) {
+;
+  }
+
+  // A completion window is padded by one character from all sides. A text field
+  // is first drawn for inputting the searcher request, then a list of matches
+  // are displayed in a scrollable list.
+  //
+  // ___
+  // |   |
+  // | __[Search]___ |
+  // | |   | |
+  // | |___| |
+  // | - Match 1.|
+  // | - Match 2.|
+  // | - ... |
+  // |   |
+  // |[Press Esc to Cancel]__|
+  //
+
+  // Get the index of the last visible match. Assuming at least one match
+  // exists.
+  int GetLastVisibleMatch(int height) {
+int index = m_first_visible_match + height;
+return std::min(index, m_delegate_sp->GetNumberOfMatches()) - 1;
+  }
+
+  int GetNumberOfVisibleMatches(int height) {
+return GetLastVisibleMatch(height) - m_first_visible_match + 1;
+  }
+
+  void UpdateScrolling(Surface &surface) {
+if (m_selected_match < m_first_visible_match) {
+  m_first_visible_match = m_selected_match;
+  return;
+}
+
+int height = surface.GetHeight();
+int last_visible_match = GetLastVisibleMatch(height);
+if (m_selected_match > last_visible_match) {
+  m_first_visible_match = m_selected_match - height + 1;
+}
+  }
+
+  void DrawMatches(Surface &surface) {
+if (m_delegate_sp->GetNumberOfMatches() == 0)
+  return;
+
+UpdateScrolling(surface);
+
+int count = GetNumberOfVisibleMatches(surface.GetHeight());
+for (int i = 0; i < count; i++) {
+  surface.MoveCursor(1, i);
+  int current_match = m_first_visible_match + i;
+  if (current_match == m_selected_match)
+surface.AttributeOn(A_REVERSE);
+  surface.PutCString(
+  m_delegate_sp->GetMatchTextAtIndex(current_match).c_str());
+  if (current_match == m_selected_match)
+surface.AttributeOff(A_REVERSE);
+}
+  }
+
+  void DrawContent(Surface &surface) {
+Rect content_bounds = surface.GetFrame();
+Rect text_field_bounds, matchs_bounds;
+content_bounds.HorizontalSplit(m_text_field.FieldDelegateGetHeight(),
+   text_field_bounds, matchs_bounds);
+Surface text_field_surface = surface.SubSurface(text_field_bounds);
+Surface matches_surface = surface.SubSurface(matchs_bounds);
+
+m_text_field.FieldDelegateDraw(text_field_surface, true);
+DrawMatches(matches_surface);
+  }
+
+  bool WindowDelegateDraw(Window &window, bool force) override {
+window.Erase();
+
+window.DrawTitleBox(window.GetName(), "Press Esc to Cancel");
+
+Rect content_bounds = window.GetFrame();
+content_bounds.Inset(2, 

[Lldb-commits] [PATCH] D108414: [LLDB][GUI] Handle extra navigation keys in forms

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch handles the up and down keys if they weren't handled by the
selected field. Moreover, it makes sure the form always absorb the key
to take full control until the form is canceled or submitted.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108414

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -2739,6 +2739,8 @@
 }
   }
 
+  // Always return eKeyHandled to absorb all events since forms are always
+  // added as pop-ups that should take full control until canceled or 
submitted.
   HandleCharResult WindowDelegateHandleChar(Window &window, int key) override {
 switch (key) {
 case '\r':
@@ -2750,9 +2752,11 @@
   }
   break;
 case '\t':
-  return SelectNext(key);
+  SelectNext(key);
+  return eKeyHandled;
 case KEY_SHIFT_TAB:
-  return SelectPrevious(key);
+  SelectPrevious(key);
+  return eKeyHandled;
 case KEY_ESCAPE:
   window.GetParent()->RemoveSubWindow(&window);
   return eKeyHandled;
@@ -2764,10 +2768,24 @@
 // to that field.
 if (m_selection_type == SelectionType::Field) {
   FieldDelegate *field = m_delegate_sp->GetField(m_selection_index);
-  return field->FieldDelegateHandleChar(key);
+  if (field->FieldDelegateHandleChar(key) == eKeyHandled)
+return eKeyHandled;
 }
 
-return eKeyNotHandled;
+// If the key wasn't handled by the possibly selected field, handle some
+// extra keys for navigation.
+switch (key) {
+case KEY_DOWN:
+  SelectNext(key);
+  return eKeyHandled;
+case KEY_UP:
+  SelectPrevious(key);
+  return eKeyHandled;
+default:
+  break;
+}
+
+return eKeyHandled;
   }
 
 protected:


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -2739,6 +2739,8 @@
 }
   }
 
+  // Always return eKeyHandled to absorb all events since forms are always
+  // added as pop-ups that should take full control until canceled or submitted.
   HandleCharResult WindowDelegateHandleChar(Window &window, int key) override {
 switch (key) {
 case '\r':
@@ -2750,9 +2752,11 @@
   }
   break;
 case '\t':
-  return SelectNext(key);
+  SelectNext(key);
+  return eKeyHandled;
 case KEY_SHIFT_TAB:
-  return SelectPrevious(key);
+  SelectPrevious(key);
+  return eKeyHandled;
 case KEY_ESCAPE:
   window.GetParent()->RemoveSubWindow(&window);
   return eKeyHandled;
@@ -2764,10 +2768,24 @@
 // to that field.
 if (m_selection_type == SelectionType::Field) {
   FieldDelegate *field = m_delegate_sp->GetField(m_selection_index);
-  return field->FieldDelegateHandleChar(key);
+  if (field->FieldDelegateHandleChar(key) == eKeyHandled)
+return eKeyHandled;
 }
 
-return eKeyNotHandled;
+// If the key wasn't handled by the possibly selected field, handle some
+// extra keys for navigation.
+switch (key) {
+case KEY_DOWN:
+  SelectNext(key);
+  return eKeyHandled;
+case KEY_UP:
+  SelectPrevious(key);
+  return eKeyHandled;
+default:
+  break;
+}
+
+return eKeyHandled;
   }
 
 protected:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D108410: [LLDB][GUI] Add submit form key combination

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new key ALt+Enter key combination to form windows.
Once invoked, the first action is executed without having to navigate to
its button.

Field exit callbacks are now also invoked on validation to support this
aforementioned key combination.

One concern for this key combination is its potential use by the window
manager of the host. I am not sure if this will be a problem, but it is
worth putting in consideration.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108410

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -89,6 +89,7 @@
 #define KEY_ESCAPE 27
 
 #define KEY_SHIFT_TAB (KEY_MAX + 1)
+#define KEY_ALT_ENTER (KEY_MAX + 2)
 
 namespace curses {
 class Menu;
@@ -2137,6 +2138,7 @@
   // action that requires valid fields.
   bool CheckFieldsValidity() {
 for (int i = 0; i < GetNumberOfFields(); i++) {
+  GetField(i)->FieldDelegateExitCallback();
   if (GetField(i)->FieldDelegateHasError()) {
 SetError("Some fields are invalid!");
 return false;
@@ -2456,13 +2458,24 @@
   Size(width, copy_height));
   }
 
+  void DrawSubmitHint(Surface &surface, bool is_active) {
+surface.MoveCursor(2, surface.GetHeight() - 1);
+if (is_active)
+  surface.AttributeOn(A_BOLD | COLOR_PAIR(BlackOnWhite));
+surface.Printf("[Press Alt+Enter to %s]",
+   m_delegate_sp->GetAction(0).GetLabel().c_str());
+if (is_active)
+  surface.AttributeOff(A_BOLD | COLOR_PAIR(BlackOnWhite));
+  }
+
   bool WindowDelegateDraw(Window &window, bool force) override {
 m_delegate_sp->UpdateFieldsVisibility();
 
 window.Erase();
 
 window.DrawTitleBox(m_delegate_sp->GetName().c_str(),
-"Press Esc to cancel");
+"Press Esc to Cancel");
+DrawSubmitHint(window, window.IsActive());
 
 Rect content_bounds = window.GetFrame();
 content_bounds.Inset(2, 2);
@@ -2585,8 +2598,8 @@
 return eKeyHandled;
   }
 
-  void ExecuteAction(Window &window) {
-FormAction &action = m_delegate_sp->GetAction(m_selection_index);
+  void ExecuteAction(Window &window, int index) {
+FormAction &action = m_delegate_sp->GetAction(index);
 action.Execute(window);
 if (m_delegate_sp->HasError()) {
   m_first_visible_line = 0;
@@ -2601,10 +2614,13 @@
 case '\n':
 case KEY_ENTER:
   if (m_selection_type == SelectionType::Action) {
-ExecuteAction(window);
+ExecuteAction(window, m_selection_index);
 return eKeyHandled;
   }
   break;
+case KEY_ALT_ENTER:
+  ExecuteAction(window, 0);
+  return eKeyHandled;
 case '\t':
   return SelectNext(key);
 case KEY_SHIFT_TAB:
@@ -6863,6 +6879,7 @@
 static_assert(LastColorPairIndex == 18, "Color indexes do not match.");
 
 define_key("\033[Z", KEY_SHIFT_TAB);
+define_key("\033\015", KEY_ALT_ENTER);
   }
 }
 


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -89,6 +89,7 @@
 #define KEY_ESCAPE 27
 
 #define KEY_SHIFT_TAB (KEY_MAX + 1)
+#define KEY_ALT_ENTER (KEY_MAX + 2)
 
 namespace curses {
 class Menu;
@@ -2137,6 +2138,7 @@
   // action that requires valid fields.
   bool CheckFieldsValidity() {
 for (int i = 0; i < GetNumberOfFields(); i++) {
+  GetField(i)->FieldDelegateExitCallback();
   if (GetField(i)->FieldDelegateHasError()) {
 SetError("Some fields are invalid!");
 return false;
@@ -2456,13 +2458,24 @@
   Size(width, copy_height));
   }
 
+  void DrawSubmitHint(Surface &surface, bool is_active) {
+surface.MoveCursor(2, surface.GetHeight() - 1);
+if (is_active)
+  surface.AttributeOn(A_BOLD | COLOR_PAIR(BlackOnWhite));
+surface.Printf("[Press Alt+Enter to %s]",
+   m_delegate_sp->GetAction(0).GetLabel().c_str());
+if (is_active)
+  surface.AttributeOff(A_BOLD | COLOR_PAIR(BlackOnWhite));
+  }
+
   bool WindowDelegateDraw(Window &window, bool force) override {
 m_delegate_sp->UpdateFieldsVisibility();
 
 window.Erase();
 
 window.DrawTitleBox(m_delegate_sp->GetName().c_str(),
-"Press Esc to cancel");
+"Press Esc to Cancel");
+DrawSubmitHint(window, window.IsActive());
 
 Rect content_bounds = window.GetFrame();
 content_bounds.Inset(2, 2);
@@ -2585,8 +2598,8 @@
 return eKeyHandled;
   }
 
-  void E

[Lldb-commits] [PATCH] D108385: [LLDB][GUI] Add extra keys to text field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds many new keys to the text field and implements new
behaviors as follows:

case KEY_HOME:
case KEY_CTRL_A:

  MoveCursorToStart();

case KEY_END:
case KEY_CTRL_E:

  MoveCursorToEnd();

case KEY_RIGHT:
case KEY_SF:

  MoveCursorRight();

case KEY_LEFT:
case KEY_SR:

  MoveCursorLeft();

case KEY_BACKSPACE:
case KEY_DELETE:

  RemovePreviousChar();

case KEY_DC:

  RemoveNextChar();

case KEY_EOL:
case KEY_CTRL_K:

  ClearToEnd();

case KEY_DL:
case KEY_CLEAR:

  Clear();

This patch also refactors scrolling to be dynamic at draw time for
easier handing.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108385

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -85,8 +85,12 @@
 // we may want curses to be disabled for some builds for instance, windows
 #if LLDB_ENABLE_CURSES
 
+#define KEY_CTRL_A 1
+#define KEY_CTRL_E 5
+#define KEY_CTRL_K 11
 #define KEY_RETURN 10
 #define KEY_ESCAPE 27
+#define KEY_DELETE 127
 
 #define KEY_SHIFT_TAB (KEY_MAX + 1)
 
@@ -1106,10 +1110,11 @@
   int GetContentLength() { return m_content.length(); }
 
   void DrawContent(Surface &surface, bool is_selected) {
+UpdateScrolling(surface.GetWidth());
+
 surface.MoveCursor(0, 0);
 const char *text = m_content.c_str() + m_first_visibile_char;
 surface.PutCString(text, surface.GetWidth());
-m_last_drawn_content_width = surface.GetWidth();
 
 // Highlight the cursor.
 surface.MoveCursor(GetCursorXPosition(), 0);
@@ -1156,6 +1161,22 @@
 DrawError(error_surface);
   }
 
+  // Get the position of the last visible character.
+  int GetLastVisibleCharPosition(int width) {
+int position = m_first_visibile_char + width - 1;
+return std::min(position, GetContentLength());
+  }
+
+  void UpdateScrolling(int width) {
+if (m_cursor_position < m_first_visibile_char) {
+  m_first_visibile_char = m_cursor_position;
+  return;
+}
+
+if (m_cursor_position > GetLastVisibleCharPosition(width))
+  m_first_visibile_char = m_cursor_position - (width - 1);
+  }
+
   // The cursor is allowed to move one character past the string.
   // m_cursor_position is in range [0, GetContentLength()].
   void MoveCursorRight() {
@@ -1168,42 +1189,54 @@
   m_cursor_position--;
   }
 
-  // If the cursor moved past the last visible character, scroll right by one
-  // character.
-  void ScrollRightIfNeeded() {
-if (m_cursor_position - m_first_visibile_char == m_last_drawn_content_width)
-  m_first_visibile_char++;
-  }
+  void MoveCursorToStart() { m_cursor_position = 0; }
+
+  void MoveCursorToEnd() { m_cursor_position = GetContentLength(); }
 
   void ScrollLeft() {
 if (m_first_visibile_char > 0)
   m_first_visibile_char--;
   }
 
-  // If the cursor moved past the first visible character, scroll left by one
-  // character.
-  void ScrollLeftIfNeeded() {
-if (m_cursor_position < m_first_visibile_char)
-  m_first_visibile_char--;
-  }
-
-  // Insert a character at the current cursor position, advance the cursor
-  // position, and make sure to scroll right if needed.
+  // Insert a character at the current cursor position and advance the cursor
+  // position.
   void InsertChar(char character) {
 m_content.insert(m_cursor_position, 1, character);
 m_cursor_position++;
-ScrollRightIfNeeded();
+ClearError();
   }
 
   // Remove the character before the cursor position, retreat the cursor
-  // position, and make sure to scroll left if needed.
-  void RemoveChar() {
+  // position, and scroll left.
+  void RemovePreviousChar() {
 if (m_cursor_position == 0)
   return;
 
 m_content.erase(m_cursor_position - 1, 1);
 m_cursor_position--;
 ScrollLeft();
+ClearError();
+  }
+
+  // Remove the character after the cursor position.
+  void RemoveNextChar() {
+if (m_cursor_position == GetContentLength())
+  return;
+
+m_content.erase(m_cursor_position, 1);
+ClearError();
+  }
+
+  // Clear characters from the current cursor position to the end.
+  void ClearToEnd() {
+m_content.erase(m_cursor_position);
+ClearError();
+  }
+
+  void Clear() {
+m_content.clear();
+m_cursor_position = 0;
+ClearError();
   }
 
   // True if the key represents a char that can be inserted in the field
@@ -1218,17 +1251,36 @@
 }
 
 switch (key) {
+case KEY_HOME:
+case KEY_CTRL_A:
+  MoveCursorToStart();
+  return eKeyHandled;
+case KEY_END:
+case KEY_CTRL_E:
+  MoveCursorToEnd();
+  return eKeyHandled;
 case KEY_RIGHT:
+case KEY

[Lldb-commits] [PATCH] D108331: [LLDB][GUI] Handle return key for compound fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch handles the return key for compound fields like lists and
mapping fields. The return key, if not handled by the field will select
the next primary element, skipping secondary elements like remove
buttons and the like.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108331

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1816,6 +1816,31 @@
 return eKeyHandled;
   }
 
+  // If the last element of the field is selected and it didn't handle the key.
+  // Select the next field or new button if the selected field is the last one.
+  HandleCharResult SelectNextInList(int key) {
+assert(m_selection_type == SelectionType::Field);
+
+FieldDelegate &field = m_fields[m_selection_index];
+if (field.FieldDelegateHandleChar(key) == eKeyHandled)
+  return eKeyHandled;
+
+if (!field.FieldDelegateOnLastOrOnlyElement())
+  return eKeyNotHandled;
+
+field.FieldDelegateExitCallback();
+
+if (m_selection_index == GetNumberOfFields() - 1) {
+  m_selection_type = SelectionType::NewButton;
+  return eKeyHandled;
+}
+
+m_selection_index++;
+FieldDelegate &next_field = m_fields[m_selection_index];
+next_field.FieldDelegateSelectFirstElement();
+return eKeyHandled;
+  }
+
   HandleCharResult FieldDelegateHandleChar(int key) override {
 switch (key) {
 case '\r':
@@ -1828,16 +1853,14 @@
   case SelectionType::RemoveButton:
 RemoveField();
 return eKeyHandled;
-  default:
-break;
+  case SelectionType::Field:
+return SelectNextInList(key);
   }
   break;
 case '\t':
-  SelectNext(key);
-  return eKeyHandled;
+  return SelectNext(key);
 case KEY_SHIFT_TAB:
-  SelectPrevious(key);
-  return eKeyHandled;
+  return SelectPrevious(key);
 default:
   break;
 }
@@ -1984,14 +2007,34 @@
 return eKeyHandled;
   }
 
+  // If the value field is selected, pass the key to it. If the key field is
+  // selected, its last element is selected, and it didn't handle the key, then
+  // select its corresponding value field.
+  HandleCharResult SelectNextField(int key) {
+if (m_selection_type == SelectionType::Value) {
+  return m_value_field.FieldDelegateHandleChar(key);
+}
+
+if (m_key_field.FieldDelegateHandleChar(key) == eKeyHandled)
+  return eKeyHandled;
+
+if (!m_key_field.FieldDelegateOnLastOrOnlyElement())
+  return eKeyNotHandled;
+
+m_key_field.FieldDelegateExitCallback();
+m_selection_type = SelectionType::Value;
+m_value_field.FieldDelegateSelectFirstElement();
+return eKeyHandled;
+  }
+
   HandleCharResult FieldDelegateHandleChar(int key) override {
 switch (key) {
+case KEY_RETURN:
+  return SelectNextField(key);
 case '\t':
-  SelectNext(key);
-  return eKeyHandled;
+  return SelectNext(key);
 case KEY_SHIFT_TAB:
-  SelectPrevious(key);
-  return eKeyHandled;
+  return SelectPrevious(key);
 default:
   break;
 }


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1816,6 +1816,31 @@
 return eKeyHandled;
   }
 
+  // If the last element of the field is selected and it didn't handle the key.
+  // Select the next field or new button if the selected field is the last one.
+  HandleCharResult SelectNextInList(int key) {
+assert(m_selection_type == SelectionType::Field);
+
+FieldDelegate &field = m_fields[m_selection_index];
+if (field.FieldDelegateHandleChar(key) == eKeyHandled)
+  return eKeyHandled;
+
+if (!field.FieldDelegateOnLastOrOnlyElement())
+  return eKeyNotHandled;
+
+field.FieldDelegateExitCallback();
+
+if (m_selection_index == GetNumberOfFields() - 1) {
+  m_selection_type = SelectionType::NewButton;
+  return eKeyHandled;
+}
+
+m_selection_index++;
+FieldDelegate &next_field = m_fields[m_selection_index];
+next_field.FieldDelegateSelectFirstElement();
+return eKeyHandled;
+  }
+
   HandleCharResult FieldDelegateHandleChar(int key) override {
 switch (key) {
 case '\r':
@@ -1828,16 +1853,14 @@
   case SelectionType::RemoveButton:
 RemoveField();
 return eKeyHandled;
-  default:
-break;
+  case SelectionType::Field:
+return SelectNextInList(key);
   }
   break;
 case '\t':
-  SelectNext(key);
-  return eKeyHandle

[Lldb-commits] [PATCH] D108327: [LLDB][GUI] Fix text field incorrect key handling

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

The isprint libc function was used to determine if the key code
represents a printable character. The problem is that the specification
leaves the behavior undefined if the key is not representable as an
unsigned char, which is the case for many ncurses keys. This patch adds
and explicit check for this undefined behavior and make it consistent.

The llvm::isPrint function didn't work correctly for some reason, most
likely because it takes a char instead of an int, which I guess makes it
unsuitable for checking ncurses key codes.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D108327

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1208,7 +1208,13 @@
 
   // True if the key represents a char that can be inserted in the field
   // content, false otherwise.
-  virtual bool IsAcceptableChar(int key) { return isprint(key); }
+  virtual bool IsAcceptableChar(int key) {
+// The behavior of isprint is undefined when the value is not representable
+// as an unsigned char. So explicitly check for non-ascii key codes.
+if (key > 127)
+  return false;
+return isprint(key);
+  }
 
   HandleCharResult FieldDelegateHandleChar(int key) override {
 if (IsAcceptableChar(key)) {


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1208,7 +1208,13 @@
 
   // True if the key represents a char that can be inserted in the field
   // content, false otherwise.
-  virtual bool IsAcceptableChar(int key) { return isprint(key); }
+  virtual bool IsAcceptableChar(int key) {
+// The behavior of isprint is undefined when the value is not representable
+// as an unsigned char. So explicitly check for non-ascii key codes.
+if (key > 127)
+  return false;
+return isprint(key);
+  }
 
   HandleCharResult FieldDelegateHandleChar(int key) override {
 if (IsAcceptableChar(key)) {
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 367171.
OmarEmaraDev added a comment.

- Address review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

Files:
  lldb/include/lldb/Target/Target.h
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -3985,6 +3985,45 @@
   return ComputeEnvironment();
 }
 
+Environment TargetProperties::GetInheritedEnvironment() const {
+  Environment environment;
+
+  if (m_target == nullptr)
+return environment;
+
+  if (!m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, ePropertyInheritEnv,
+  g_target_properties[ePropertyInheritEnv].default_uint_value != 0))
+return environment;
+
+  PlatformSP platform_sp = m_target->GetPlatform();
+  if (platform_sp == nullptr)
+return environment;
+
+  Environment platform_environment = platform_sp->GetEnvironment();
+  for (const auto &KV : platform_environment)
+environment[KV.first()] = KV.second;
+
+  Args property_unset_environment;
+  m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyUnsetEnvVars,
+property_unset_environment);
+  for (const auto &var : property_unset_environment)
+environment.erase(var.ref());
+
+  return environment;
+}
+
+Environment TargetProperties::GetTargetEnvironment() const {
+  Args property_environment;
+  m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyEnvVars,
+property_environment);
+  Environment environment;
+  for (const auto &KV : Environment(property_environment))
+environment[KV.first()] = KV.second;
+
+  return environment;
+}
+
 void TargetProperties::SetEnvironment(Environment env) {
   // TODO: Get rid of the Args intermediate step
   const uint32_t idx = ePropertyEnvVars;
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1260,6 +1260,14 @@
 
   const std::string &GetText() { return m_content; }
 
+  void SetText(const char *text) {
+if (text == nullptr) {
+  m_content.clear();
+  return;
+}
+m_content = text;
+  }
+
 protected:
   std::string m_label;
   bool m_required;
@@ -1619,6 +1627,33 @@
   }
 };
 
+class LazyBooleanFieldDelegate : public ChoicesFieldDelegate {
+public:
+  LazyBooleanFieldDelegate(const char *label, const char *calculate_label)
+  : ChoicesFieldDelegate(label, 3, GetPossibleOptions(calculate_label)) {}
+
+  static constexpr const char *kNo = "No";
+  static constexpr const char *kYes = "Yes";
+
+  std::vector GetPossibleOptions(const char *calculate_label) {
+std::vector options;
+options.push_back(calculate_label);
+options.push_back(kYes);
+options.push_back(kNo);
+return options;
+  }
+
+  LazyBool GetLazyBoolean() {
+std::string choice = GetChoiceContent();
+if (choice == kNo)
+  return eLazyBoolNo;
+else if (choice == kYes)
+  return eLazyBoolYes;
+else
+  return eLazyBoolCalculate;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1909,6 +1944,29 @@
   SelectionType m_selection_type;
 };
 
+class ArgumentsFieldDelegate : public ListFieldDelegate {
+public:
+  ArgumentsFieldDelegate()
+  : ListFieldDelegate("Arguments",
+  TextFieldDelegate("Argument", "", false)) {}
+
+  Args GetArguments() {
+Args arguments;
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  arguments.AppendArgument(GetField(i).GetText());
+}
+return arguments;
+  }
+
+  void AddArguments(const Args &arguments) {
+for (size_t i = 0; i < arguments.GetArgumentCount(); i++) {
+  AddNewField();
+  TextFieldDelegate &field = GetField(GetNumberOfFields() - 1);
+  field.SetText(arguments.GetArgumentAtIndex(i));
+}
+  }
+};
+
 template 
 class MappingFieldDelegate : public FieldDelegate {
 public:
@@ -2069,14 +2127,36 @@
   const std::string &GetName() { return GetKeyField().GetName(); }
 
   const std::string &GetValue() { return GetValueField().GetText(); }
+
+  void SetName(const char *name) { return GetKeyField().SetText(name); }
+
+  void SetValue(const char *value) { return GetValueField().SetText(value); }
 };
 
 class EnvironmentVariableListFieldDelegate
 : public ListFieldDelegate {
 public:
-  EnvironmentVariableListFieldDelegate()
-  : ListFieldDelegate("Environment Variables",
-  EnvironmentVariableFieldDelegate()) {}
+  EnvironmentVariableListFieldDelegate(const char *label)
+  : ListFieldDelegate(label, EnvironmentVariableFieldDelegat

[Lldb-commits] [PATCH] D107761: [LLDB][GUI] Refactor form drawing using subsurfaces

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@clayborg  Perhaps you missed this, it is essential the same as D107182 
 but without the unsupported function. It 
would be good to have this committed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107761

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 365958.
OmarEmaraDev added a comment.

- Separate environment field into two fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

Files:
  lldb/include/lldb/Target/Target.h
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/source/Target/Target.cpp

Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -3985,6 +3985,45 @@
   return ComputeEnvironment();
 }
 
+Environment TargetProperties::GetInheritedEnvironment() const {
+  Environment environment;
+
+  if (m_target == nullptr)
+return environment;
+
+  if (!m_collection_sp->GetPropertyAtIndexAsBoolean(
+  nullptr, ePropertyInheritEnv,
+  g_target_properties[ePropertyInheritEnv].default_uint_value != 0))
+return environment;
+
+  PlatformSP platform_sp = m_target->GetPlatform();
+  if (platform_sp == nullptr)
+return environment;
+
+  Environment platform_environment = platform_sp->GetEnvironment();
+  for (const auto &KV : platform_environment)
+environment[KV.first()] = KV.second;
+
+  Args property_unset_environment;
+  m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyUnsetEnvVars,
+property_unset_environment);
+  for (const auto &var : property_unset_environment)
+environment.erase(var.ref());
+
+  return environment;
+}
+
+Environment TargetProperties::GetTargetEnvironment() const {
+  Args property_environment;
+  m_collection_sp->GetPropertyAtIndexAsArgs(nullptr, ePropertyEnvVars,
+property_environment);
+  Environment environment;
+  for (const auto &KV : Environment(property_environment))
+environment[KV.first()] = KV.second;
+
+  return environment;
+}
+
 void TargetProperties::SetEnvironment(Environment env) {
   // TODO: Get rid of the Args intermediate step
   const uint32_t idx = ePropertyEnvVars;
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -28,6 +28,7 @@
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/ValueObjectUpdater.h"
 #include "lldb/Host/File.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/Predicate.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
@@ -1260,6 +1261,14 @@
 
   const std::string &GetText() { return m_content; }
 
+  void SetText(const char *text) {
+if (text == nullptr) {
+  m_content.clear();
+  return;
+}
+m_content = text;
+  }
+
 protected:
   std::string m_label;
   bool m_required;
@@ -1619,6 +1628,33 @@
   }
 };
 
+class LazyBooleanFieldDelegate : public ChoicesFieldDelegate {
+public:
+  LazyBooleanFieldDelegate(const char *label, const char *calculate_label)
+  : ChoicesFieldDelegate(label, 3, GetPossibleOptions(calculate_label)) {}
+
+  static constexpr const char *kNo = "No";
+  static constexpr const char *kYes = "Yes";
+
+  std::vector GetPossibleOptions(const char *calculate_label) {
+std::vector options;
+options.push_back(calculate_label);
+options.push_back(kYes);
+options.push_back(kNo);
+return options;
+  }
+
+  LazyBool GetLazyBoolean() {
+std::string choice = GetChoiceContent();
+if (choice == kNo)
+  return eLazyBoolNo;
+else if (choice == kYes)
+  return eLazyBoolYes;
+else
+  return eLazyBoolCalculate;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1909,6 +1945,29 @@
   SelectionType m_selection_type;
 };
 
+class ArgumentsFieldDelegate : public ListFieldDelegate {
+public:
+  ArgumentsFieldDelegate()
+  : ListFieldDelegate("Arguments",
+  TextFieldDelegate("Argument", "", false)) {}
+
+  Args GetArguments() {
+Args arguments;
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  arguments.AppendArgument(GetField(i).GetText());
+}
+return arguments;
+  }
+
+  void AddArguments(const Args &arguments) {
+for (size_t i = 0; i < arguments.GetArgumentCount(); i++) {
+  AddNewField();
+  TextFieldDelegate &field = GetField(GetNumberOfFields() - 1);
+  field.SetText(arguments.GetArgumentAtIndex(i));
+}
+  }
+};
+
 template 
 class MappingFieldDelegate : public FieldDelegate {
 public:
@@ -2069,14 +2128,36 @@
   const std::string &GetName() { return GetKeyField().GetName(); }
 
   const std::string &GetValue() { return GetValueField().GetText(); }
+
+  void SetName(const char *name) { return GetKeyField().SetText(name); }
+
+  void SetValue(const char *value) { return GetValueField().SetText(value); }
 };
 
 class EnvironmentVariableListFieldDelegate
 : public ListFieldDelega

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Adding a boolean for inheriting the environment may not be necessary, we can 
just add two environment fields, one for inherited and one for user specified. 
The inherited one will be put in advanced settings with another boolean that 
show or hide the field. Both will be filled with the appropriate default 
values. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3223
+
+const Environment &target_environment = target->GetEnvironment();
+m_enviroment_field->AddEnvironmentVariables(target_environment);

clayborg wrote:
> Does this currently get all target env vars including the inherited ones?
Yes. It seems the logic in `TargetProperties::ComputeEnvironment` adds the 
inherited environment, erase the unset environment, and finally add the target 
environment. If we want the target environment only, we can add another method 
to `TargetProperties` to get only those, or we could  erase the platform 
environment elements from the computed environment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 365825.
OmarEmaraDev added a comment.

- Address review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -28,6 +28,7 @@
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/ValueObjectUpdater.h"
 #include "lldb/Host/File.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/Predicate.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
@@ -1260,6 +1261,14 @@
 
   const std::string &GetText() { return m_content; }
 
+  void SetText(const char *text) {
+if (text == nullptr) {
+  m_content.clear();
+  return;
+}
+m_content = text;
+  }
+
 protected:
   std::string m_label;
   bool m_required;
@@ -1619,6 +1628,33 @@
   }
 };
 
+class LazyBooleanFieldDelegate : public ChoicesFieldDelegate {
+public:
+  LazyBooleanFieldDelegate(const char *label, const char *calculate_label)
+  : ChoicesFieldDelegate(label, 3, GetPossibleOptions(calculate_label)) {}
+
+  static constexpr const char *kNo = "No";
+  static constexpr const char *kYes = "Yes";
+
+  std::vector GetPossibleOptions(const char *calculate_label) {
+std::vector options;
+options.push_back(calculate_label);
+options.push_back(kYes);
+options.push_back(kNo);
+return options;
+  }
+
+  LazyBool GetLazyBoolean() {
+std::string choice = GetChoiceContent();
+if (choice == kNo)
+  return eLazyBoolNo;
+else if (choice == kYes)
+  return eLazyBoolYes;
+else
+  return eLazyBoolCalculate;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1909,6 +1945,29 @@
   SelectionType m_selection_type;
 };
 
+class ArgumentsFieldDelegate : public ListFieldDelegate {
+public:
+  ArgumentsFieldDelegate()
+  : ListFieldDelegate("Arguments",
+  TextFieldDelegate("Argument", "", false)) {}
+
+  Args GetArguments() {
+Args arguments;
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  arguments.AppendArgument(GetField(i).GetText());
+}
+return arguments;
+  }
+
+  void AddArguments(const Args &arguments) {
+for (size_t i = 0; i < arguments.GetArgumentCount(); i++) {
+  AddNewField();
+  TextFieldDelegate &field = GetField(GetNumberOfFields() - 1);
+  field.SetText(arguments.GetArgumentAtIndex(i));
+}
+  }
+};
+
 template 
 class MappingFieldDelegate : public FieldDelegate {
 public:
@@ -2069,6 +2128,10 @@
   const std::string &GetName() { return GetKeyField().GetName(); }
 
   const std::string &GetValue() { return GetValueField().GetText(); }
+
+  void SetName(const char *name) { return GetKeyField().SetText(name); }
+
+  void SetValue(const char *value) { return GetValueField().SetText(value); }
 };
 
 class EnvironmentVariableListFieldDelegate
@@ -2077,6 +2140,25 @@
   EnvironmentVariableListFieldDelegate()
   : ListFieldDelegate("Environment Variables",
   EnvironmentVariableFieldDelegate()) {}
+
+  Environment GetEnvironment() {
+Environment environment;
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  environment.insert(
+  std::make_pair(GetField(i).GetName(), GetField(i).GetValue()));
+}
+return environment;
+  }
+
+  void AddEnvironmentVariables(const Environment &environment) {
+for (auto &variable : environment) {
+  AddNewField();
+  EnvironmentVariableFieldDelegate &field =
+  GetField(GetNumberOfFields() - 1);
+  field.SetName(variable.getKey().str().c_str());
+  field.SetValue(variable.getValue().c_str());
+}
+  }
 };
 
 class FormAction {
@@ -2201,6 +2283,14 @@
 return delegate;
   }
 
+  LazyBooleanFieldDelegate *AddLazyBooleanField(const char *label,
+const char *calculate_label) {
+LazyBooleanFieldDelegate *delegate =
+new LazyBooleanFieldDelegate(label, calculate_label);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   ChoicesFieldDelegate *AddChoicesField(const char *label, int height,
 std::vector choices) {
 ChoicesFieldDelegate *delegate =
@@ -2230,6 +2320,12 @@
 return delegate;
   }
 
+  ArgumentsFieldDelegate *AddArgumentsField() {
+ArgumentsFieldDelegate *delegate = new ArgumentsFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   MappingFieldDelegate *AddMappingField(K key_field, V value_field) {
 MappingFieldDelegate *delegate =
@@ -3031,6 +3127,359 @@
   ChoicesFieldDelegate *m_load_dependent_files_field;
 }

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@jingham I wasn't arguing that we should remove those environment variables, on 
the contrary. Greg was suggesting that we populate the environment field with 
the target environment instead of implicitly adding them to the environment of 
the launch info. The problem with that is that there is a large number of 
environments that gets added (15 in my case) as shown in the following 
screenshot. What I was saying in my comment above is that I don't think we 
should show those to the user, we should transparently add them instead and let 
the user add additional variables as needed. We can then look into adding other 
settings to exclude them if necessary.

F18486746: 20210811-210121.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

One thing to note as well is that the target environment seem to include many 
environment variables like PATH and HOME, I don't think those should be 
included.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:3237-3240
+} else {
+  action.Open(STDIN_FILENO, dev_null, true, false);
+  launch_info.AppendFileAction(action);
+}

clayborg wrote:
> We don't need to do anything if this isn't specified as by default the input 
> will be hooked up by the debugging to something valid. If the users wants to 
> redirect to /dev/null, they can just specify "/dev/null" (or the right path 
> for this on their system.
But since we are in GUI mode, what will the standard files be hooked into? It 
seems the only two options is to either redirect to a file or to /dev/null, 
hence the condition I have. Is this not the case?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

This is not fully working yet, and I am still debugging it. But I thought I 
would push it for early feedback regardless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107869

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107869: [LLDB][GUI] Add Process Launch form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a process launch form. Additionally, a LazyBoolean field
was implemented and numerous utility methods were added to various
fields to get the launch form working.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107869

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -28,6 +28,7 @@
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Core/ValueObjectUpdater.h"
 #include "lldb/Host/File.h"
+#include "lldb/Host/HostInfo.h"
 #include "lldb/Utility/Predicate.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
@@ -1619,6 +1620,33 @@
   }
 };
 
+class LazyBooleanFieldDelegate : public ChoicesFieldDelegate {
+public:
+  LazyBooleanFieldDelegate(const char *label, const char *calculate_label)
+  : ChoicesFieldDelegate(label, 3, GetPossibleOptions(calculate_label)) {}
+
+  static constexpr const char *kNo = "No";
+  static constexpr const char *kYes = "Yes";
+
+  std::vector GetPossibleOptions(const char *calculate_label) {
+std::vector options;
+options.push_back(calculate_label);
+options.push_back(kYes);
+options.push_back(kNo);
+return options;
+  }
+
+  LazyBool GetLazyBoolean() {
+std::string choice = GetChoiceContent();
+if (choice == kNo)
+  return eLazyBoolNo;
+else if (choice == kYes)
+  return eLazyBoolYes;
+else
+  return eLazyBoolCalculate;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1909,6 +1937,21 @@
   SelectionType m_selection_type;
 };
 
+class ArgumentsFieldDelegate : public ListFieldDelegate {
+public:
+  ArgumentsFieldDelegate()
+  : ListFieldDelegate("Arguments",
+  TextFieldDelegate("Argument", "", false)) {}
+
+  Args GetArguments() {
+Args arguments;
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  arguments.AppendArgument(GetField(i).GetText());
+}
+return arguments;
+  }
+};
+
 template 
 class MappingFieldDelegate : public FieldDelegate {
 public:
@@ -2077,6 +2120,15 @@
   EnvironmentVariableListFieldDelegate()
   : ListFieldDelegate("Environment Variables",
   EnvironmentVariableFieldDelegate()) {}
+
+  Environment GetEnvironment() {
+Environment environment;
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  environment.insert(
+  std::make_pair(GetField(i).GetName(), GetField(i).GetValue()));
+}
+return environment;
+  }
 };
 
 class FormAction {
@@ -2201,6 +2253,14 @@
 return delegate;
   }
 
+  LazyBooleanFieldDelegate *AddLazyBooleanField(const char *label,
+const char *calculate_label) {
+LazyBooleanFieldDelegate *delegate =
+new LazyBooleanFieldDelegate(label, calculate_label);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   ChoicesFieldDelegate *AddChoicesField(const char *label, int height,
 std::vector choices) {
 ChoicesFieldDelegate *delegate =
@@ -2230,6 +2290,12 @@
 return delegate;
   }
 
+  ArgumentsFieldDelegate *AddArgumentsField() {
+ArgumentsFieldDelegate *delegate = new ArgumentsFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   MappingFieldDelegate *AddMappingField(K key_field, V value_field) {
 MappingFieldDelegate *delegate =
@@ -3031,6 +3097,309 @@
   ChoicesFieldDelegate *m_load_dependent_files_field;
 };
 
+class ProcessLaunchFormDelegate : public FormDelegate {
+public:
+  ProcessLaunchFormDelegate(Debugger &debugger, WindowSP main_window_sp)
+  : m_debugger(debugger), m_main_window_sp(main_window_sp) {
+
+m_arguments_field = AddArgumentsField();
+m_enviroment_field = AddEnvironmentVariableListField();
+
+m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+
+m_stop_at_entry_field = AddBooleanField("Stop at entry point.", false);
+m_working_directory_field =
+AddDirectoryField("Working Directory", "", /*need_to_exist=*/true,
+  /*required=*/false);
+m_disable_aslr_field =
+AddLazyBooleanField("Disable ASLR", "Use target setting");
+m_plugin_field = AddProcessPluginField();
+m_arch_field = AddArchField("Architecture", "", /*required=*/false);
+m_shell_field = AddFileField("Shell", "", /*need_to_exist=*/true,
+ /*required=*/false);
+m_expand_shell_arguments_field =
+   

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@clayborg This is unrelated to D107761 , so 
it should be safe to commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 365216.
OmarEmaraDev added a comment.

- Fix possible crash in SetText


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -45,6 +45,7 @@
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
+#include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/VariableList.h"
@@ -3772,9 +3773,14 @@
TreeItem *&selected_item) {
 return;
   }
-  virtual bool TreeDelegateItemSelected(
-  TreeItem &item) = 0; // Return true if we need to update views
+  // This is invoked when a tree item is selected. If true is returned, the
+  // views are updated.
+  virtual bool TreeDelegateItemSelected(TreeItem &item) = 0;
   virtual bool TreeDelegateExpandRootByDefault() { return false; }
+  // This is mostly useful for root tree delegates. If false is returned,
+  // drawing will be skipped completely. This is needed, for instance, in
+  // skipping drawing of the threads tree if there is no running process.
+  virtual bool TreeDelegateShouldDraw() { return true; }
 };
 
 typedef std::shared_ptr TreeDelegateSP;
@@ -3964,6 +3970,16 @@
 
   void SetIdentifier(uint64_t identifier) { m_identifier = identifier; }
 
+  const std::string &GetText() const { return m_text; }
+
+  void SetText(const char *text) {
+if (text == nullptr) {
+  m_text.clear();
+  return;
+}
+m_text = text;
+  }
+
   void SetMightHaveChildren(bool b) { m_might_have_children = b; }
 
 protected:
@@ -3971,6 +3987,7 @@
   TreeDelegate &m_delegate;
   void *m_user_data;
   uint64_t m_identifier;
+  std::string m_text;
   int m_row_idx; // Zero based visible row index, -1 if not visible or for the
  // root item
   std::vector m_children;
@@ -3989,21 +4006,6 @@
   int NumVisibleRows() const { return m_max_y - m_min_y; }
 
   bool WindowDelegateDraw(Window &window, bool force) override {
-ExecutionContext exe_ctx(
-m_debugger.GetCommandInterpreter().GetExecutionContext());
-Process *process = exe_ctx.GetProcessPtr();
-
-bool display_content = false;
-if (process) {
-  StateType state = process->GetState();
-  if (StateIsStoppedState(state, true)) {
-// We are stopped, so it is ok to
-display_content = true;
-  } else if (StateIsRunningState(state)) {
-return true; // Don't do any updating when we are running
-  }
-}
-
 m_min_x = 2;
 m_min_y = 1;
 m_max_x = window.GetWidth() - 1;
@@ -4012,35 +4014,36 @@
 window.Erase();
 window.DrawTitleBox(window.GetName());
 
-if (display_content) {
-  const int num_visible_rows = NumVisibleRows();
-  m_num_rows = 0;
-  m_root.CalculateRowIndexes(m_num_rows);
-  m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
- m_selected_item);
-
-  // If we unexpanded while having something selected our total number of
-  // rows is less than the num visible rows, then make sure we show all the
-  // rows by setting the first visible row accordingly.
-  if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
-m_first_visible_row = 0;
-
-  // Make sure the selected row is always visible
-  if (m_selected_row_idx < m_first_visible_row)
-m_first_visible_row = m_selected_row_idx;
-  else if (m_first_visible_row + num_visible_rows <= m_selected_row_idx)
-m_first_visible_row = m_selected_row_idx - num_visible_rows + 1;
-
-  int row_idx = 0;
-  int num_rows_left = num_visible_rows;
-  m_root.Draw(window, m_first_visible_row, m_selected_row_idx, row_idx,
-  num_rows_left);
-  // Get the selected row
-  m_selected_item = m_root.GetItemForRowIndex(m_selected_row_idx);
-} else {
+if (!m_delegate_sp->TreeDelegateShouldDraw()) {
   m_selected_item = nullptr;
+  return true;
 }
 
+const int num_visible_rows = NumVisibleRows();
+m_num_rows = 0;
+m_root.CalculateRowIndexes(m_num_rows);
+m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
+   m_selected_item);
+
+// If we unexpanded while having something selected our total number of
+// rows is less than the num visible rows, then make sure we show all the
+// rows by setting the first visible row accordingly.
+if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
+  m_first_visible_row = 0;
+
+// Make sure the selected row i

[Lldb-commits] [PATCH] D107761: [LLDB][GUI] Refactor form drawing using subsurfaces

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

This is a reimplementation of D107182 , which 
was committed and then reverted because the `is_pad` function is not 
universally supported. The updated patch adds an explicit type member to 
identify the backing object of the surface.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107761

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107761: [LLDB][GUI] Refactor form drawing using subsurfaces

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new method SubSurface to the Surface class. The method
returns another surface that is a subset of this surface. This is
important to further abstract away drawing from the ncurses objects. For
instance, fields could previously be drawn on subpads only but can now
be drawn on any surface. This is needed to create the file search
dialogs and similar functionalities.

There is an opportunity to refactor window drawing in general using
surfaces, but we shall consider this separately later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107761

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -343,15 +343,30 @@
 // A surface is an abstraction for something than can be drawn on. The surface
 // have a width, a height, a cursor position, and a multitude of drawing
 // operations. This type should be sub-classed to get an actually useful ncurses
-// object, such as a Window, SubWindow, Pad, or a SubPad.
+// object, such as a Window or a Pad.
 class Surface {
 public:
-  Surface() : m_window(nullptr) {}
+  enum class Type { Window, Pad };
+
+  Surface(Surface::Type type) : m_type(type), m_window(nullptr) {}
 
   WINDOW *get() { return m_window; }
 
   operator WINDOW *() { return m_window; }
 
+  Surface SubSurface(Rect bounds) {
+Surface subSurface(m_type);
+if (m_type == Type::Pad)
+  subSurface.m_window =
+  ::subpad(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+else
+  subSurface.m_window =
+  ::derwin(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+return subSurface;
+  }
+
   // Copy a region of the surface to another surface.
   void CopyToSurface(Surface &target, Point source_origin, Point target_origin,
  Size size) {
@@ -535,41 +550,32 @@
   }
 
 protected:
+  Type m_type;
   WINDOW *m_window;
 };
 
 class Pad : public Surface {
 public:
-  Pad(Size size) { m_window = ::newpad(size.height, size.width); }
-
-  ~Pad() { ::delwin(m_window); }
-};
-
-class SubPad : public Surface {
-public:
-  SubPad(Pad &pad, Rect bounds) {
-m_window = ::subpad(pad.get(), bounds.size.height, bounds.size.width,
-bounds.origin.y, bounds.origin.x);
-  }
-  SubPad(SubPad &subpad, Rect bounds) {
-m_window = ::subpad(subpad.get(), bounds.size.height, bounds.size.width,
-bounds.origin.y, bounds.origin.x);
+  Pad(Size size) : Surface(Surface::Type::Pad) {
+m_window = ::newpad(size.height, size.width);
   }
 
-  ~SubPad() { ::delwin(m_window); }
+  ~Pad() { ::delwin(m_window); }
 };
 
 class Window : public Surface {
 public:
   Window(const char *name)
-  : m_name(name), m_panel(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_panel(nullptr),
+m_parent(nullptr), m_subwindows(), m_delegate_sp(),
+m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(false),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {}
 
   Window(const char *name, WINDOW *w, bool del = true)
-  : m_name(name), m_panel(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_panel(nullptr),
+m_parent(nullptr), m_subwindows(), m_delegate_sp(),
+m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(del),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
 if (w)
@@ -577,8 +583,8 @@
   }
 
   Window(const char *name, const Rect &bounds)
-  : m_name(name), m_parent(nullptr), m_subwindows(), m_delegate_sp(),
-m_curr_active_window_idx(UINT32_MAX),
+  : Surface(Surface::Type::Window), m_name(name), m_parent(nullptr),
+m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
 m_prev_active_window_idx(UINT32_MAX), m_delete(true),
 m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
 Reset(::newwin(bounds.size.height, bounds.size.width, bounds.origin.y,
@@ -970,20 +976,6 @@
   const Window &operator=(const Window &) = delete;
 };
 
-class DerivedWindow : public Surface {
-public:
-  DerivedWindow(Window &window, Rect bounds) {
-m_window = ::derwin(window.get(), bounds.size.height, bounds.size.width,
-   

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@jingham I see. Thanks!

Here is how it looks now:

F18450643: 20210809-130123.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 365145.
OmarEmaraDev added a comment.
Herald added a reviewer: jdoerfert.
Herald added a subscriber: sstefan1.

- Address review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -45,6 +45,7 @@
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
+#include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/Function.h"
 #include "lldb/Symbol/Symbol.h"
 #include "lldb/Symbol/VariableList.h"
@@ -3772,9 +3773,14 @@
TreeItem *&selected_item) {
 return;
   }
-  virtual bool TreeDelegateItemSelected(
-  TreeItem &item) = 0; // Return true if we need to update views
+  // This is invoked when a tree item is selected. If true is returned, the
+  // views are updated.
+  virtual bool TreeDelegateItemSelected(TreeItem &item) = 0;
   virtual bool TreeDelegateExpandRootByDefault() { return false; }
+  // This is mostly useful for root tree delegates. If false is returned,
+  // drawing will be skipped completely. This is needed, for instance, in
+  // skipping drawing of the threads tree if there is no running process.
+  virtual bool TreeDelegateShouldDraw() { return true; }
 };
 
 typedef std::shared_ptr TreeDelegateSP;
@@ -3964,6 +3970,10 @@
 
   void SetIdentifier(uint64_t identifier) { m_identifier = identifier; }
 
+  const std::string &GetText() const { return m_text; }
+
+  void SetText(const char *text) { m_text = text; }
+
   void SetMightHaveChildren(bool b) { m_might_have_children = b; }
 
 protected:
@@ -3971,6 +3981,7 @@
   TreeDelegate &m_delegate;
   void *m_user_data;
   uint64_t m_identifier;
+  std::string m_text;
   int m_row_idx; // Zero based visible row index, -1 if not visible or for the
  // root item
   std::vector m_children;
@@ -3989,21 +4000,6 @@
   int NumVisibleRows() const { return m_max_y - m_min_y; }
 
   bool WindowDelegateDraw(Window &window, bool force) override {
-ExecutionContext exe_ctx(
-m_debugger.GetCommandInterpreter().GetExecutionContext());
-Process *process = exe_ctx.GetProcessPtr();
-
-bool display_content = false;
-if (process) {
-  StateType state = process->GetState();
-  if (StateIsStoppedState(state, true)) {
-// We are stopped, so it is ok to
-display_content = true;
-  } else if (StateIsRunningState(state)) {
-return true; // Don't do any updating when we are running
-  }
-}
-
 m_min_x = 2;
 m_min_y = 1;
 m_max_x = window.GetWidth() - 1;
@@ -4012,35 +4008,36 @@
 window.Erase();
 window.DrawTitleBox(window.GetName());
 
-if (display_content) {
-  const int num_visible_rows = NumVisibleRows();
-  m_num_rows = 0;
-  m_root.CalculateRowIndexes(m_num_rows);
-  m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
- m_selected_item);
-
-  // If we unexpanded while having something selected our total number of
-  // rows is less than the num visible rows, then make sure we show all the
-  // rows by setting the first visible row accordingly.
-  if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
-m_first_visible_row = 0;
-
-  // Make sure the selected row is always visible
-  if (m_selected_row_idx < m_first_visible_row)
-m_first_visible_row = m_selected_row_idx;
-  else if (m_first_visible_row + num_visible_rows <= m_selected_row_idx)
-m_first_visible_row = m_selected_row_idx - num_visible_rows + 1;
-
-  int row_idx = 0;
-  int num_rows_left = num_visible_rows;
-  m_root.Draw(window, m_first_visible_row, m_selected_row_idx, row_idx,
-  num_rows_left);
-  // Get the selected row
-  m_selected_item = m_root.GetItemForRowIndex(m_selected_row_idx);
-} else {
+if (!m_delegate_sp->TreeDelegateShouldDraw()) {
   m_selected_item = nullptr;
+  return true;
 }
 
+const int num_visible_rows = NumVisibleRows();
+m_num_rows = 0;
+m_root.CalculateRowIndexes(m_num_rows);
+m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
+   m_selected_item);
+
+// If we unexpanded while having something selected our total number of
+// rows is less than the num visible rows, then make sure we show all the
+// rows by setting the first visible row accordingly.
+if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
+  m_first_visible_row = 0;
+
+// Make sure the selected row is always visible
+i

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@clayborg With you suggestions it currently looks like this. The concern I have 
is that lines are now 36 characters longer, and some important information are 
now truncated. The breakpoint and breakpoint locations IDs seems to be just the 
index of the objects in the tree, are they not? In this case, they seem 
redundant to me. Similarly, the address takes a lot of space and is included in 
the next level of details, so it might not be as important to include in this 
level either.

F18404076: 20210806-225259.png 

I am having some difficulties adding the last level of details. My initial 
approach was to add and compute a StringList containing all the details to the 
BreakpointLocationTreeDelegate. Then I added a  StringTreeDelegate that have 
the string from the string list in its user data which  it then simply draws. 
The problem, I realized, is that the BreakpointLocationTreeDelegate is shared 
between all Breakpoint Location items, so the list is overwritten by different 
elements. What I did was instance multiple BreakpointLocationTreeDelegate for 
each item, but that tuned more involved that I thought, so I am not sure what 
the best approach would be now. What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107182: [LLDB][GUI] Refactor form drawing using subsurfaces

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@jasonmolenda Okay, I will just reimplement the patch without `is_pad` somehow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107182

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107182: [LLDB][GUI] Refactor form drawing using subsurfaces

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

`is_pad` was added in 2009 in ncurses 5.7 - patch 20090906. So it is 
sufficiently old to be used, is it not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107182

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107182: [LLDB][GUI] Refactor form drawing using subsurfaces

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@jasonmolenda `is_pad` is defined by ncurses. According to the man page,  it is 
an extension that is `not supported on Version 7, BSD or System V 
implementations.`, but that shouldn't be a problem as far as I know. Can you 
maybe check the ncurses header and see if it is there?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107182

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@clayborg What do you think? Should we add another tree level with more details 
or do something else?

By the way, it would be nice to have D107182  
commited because it is needed by the search form.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

I faced a bit of difficulty in this patch, so it is not exactly complete. One 
issue is that breakpoint descriptions can be very long, I tried to implemented 
multiline items to workaround this, but that didn't work out and was a waste of 
time. For now, I just tried to keep descriptions as small as possible. One 
thing I want to try now is to simply add another tree level and add all 
necessary information there, similar to verbose breakpoint description.
This is also missing the breakpoint operators, like enabling and disabling 
breakpoints. Since the window doesn't delegate key handing to the items, this 
will have to be implemented first.

F18327498: 20210803-28.png 

F18327497: 20210803-211059.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107386

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D107386: [LLDB][GUI] Add Breakpoints window

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a breakpoints window that lists all breakpoints and
breakpoints locations. The window is implemented as a tree, where the
first level is the breakpoints and the second level is breakpoints
locations.

The tree delegate was hardcoded to only draw when there is a process,
which is not necessary for breakpoints, so the relevant logic was
abstracted in the TreeDelegateShouldDraw method.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107386

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -3772,9 +3772,14 @@
TreeItem *&selected_item) {
 return;
   }
-  virtual bool TreeDelegateItemSelected(
-  TreeItem &item) = 0; // Return true if we need to update views
+  // This is invoked when a tree item is selected. If true is returned, the
+  // views are updated.
+  virtual bool TreeDelegateItemSelected(TreeItem &item) = 0;
   virtual bool TreeDelegateExpandRootByDefault() { return false; }
+  // This is mostly useful for root tree delegates. If false is returned,
+  // drawing will be skipped completely. This is needed, for instance, in
+  // skipping drawing of the threads tree if there is no running process.
+  virtual bool TreeDelegateShouldDraw() { return true; }
 };
 
 typedef std::shared_ptr TreeDelegateSP;
@@ -3989,21 +3994,6 @@
   int NumVisibleRows() const { return m_max_y - m_min_y; }
 
   bool WindowDelegateDraw(Window &window, bool force) override {
-ExecutionContext exe_ctx(
-m_debugger.GetCommandInterpreter().GetExecutionContext());
-Process *process = exe_ctx.GetProcessPtr();
-
-bool display_content = false;
-if (process) {
-  StateType state = process->GetState();
-  if (StateIsStoppedState(state, true)) {
-// We are stopped, so it is ok to
-display_content = true;
-  } else if (StateIsRunningState(state)) {
-return true; // Don't do any updating when we are running
-  }
-}
-
 m_min_x = 2;
 m_min_y = 1;
 m_max_x = window.GetWidth() - 1;
@@ -4012,35 +4002,36 @@
 window.Erase();
 window.DrawTitleBox(window.GetName());
 
-if (display_content) {
-  const int num_visible_rows = NumVisibleRows();
-  m_num_rows = 0;
-  m_root.CalculateRowIndexes(m_num_rows);
-  m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
- m_selected_item);
-
-  // If we unexpanded while having something selected our total number of
-  // rows is less than the num visible rows, then make sure we show all the
-  // rows by setting the first visible row accordingly.
-  if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
-m_first_visible_row = 0;
-
-  // Make sure the selected row is always visible
-  if (m_selected_row_idx < m_first_visible_row)
-m_first_visible_row = m_selected_row_idx;
-  else if (m_first_visible_row + num_visible_rows <= m_selected_row_idx)
-m_first_visible_row = m_selected_row_idx - num_visible_rows + 1;
-
-  int row_idx = 0;
-  int num_rows_left = num_visible_rows;
-  m_root.Draw(window, m_first_visible_row, m_selected_row_idx, row_idx,
-  num_rows_left);
-  // Get the selected row
-  m_selected_item = m_root.GetItemForRowIndex(m_selected_row_idx);
-} else {
+if (!m_delegate_sp->TreeDelegateShouldDraw()) {
   m_selected_item = nullptr;
+  return true;
 }
 
+const int num_visible_rows = NumVisibleRows();
+m_num_rows = 0;
+m_root.CalculateRowIndexes(m_num_rows);
+m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
+   m_selected_item);
+
+// If we unexpanded while having something selected our total number of
+// rows is less than the num visible rows, then make sure we show all the
+// rows by setting the first visible row accordingly.
+if (m_first_visible_row > 0 && m_num_rows < num_visible_rows)
+  m_first_visible_row = 0;
+
+// Make sure the selected row is always visible
+if (m_selected_row_idx < m_first_visible_row)
+  m_first_visible_row = m_selected_row_idx;
+else if (m_first_visible_row + num_visible_rows <= m_selected_row_idx)
+  m_first_visible_row = m_selected_row_idx - num_visible_rows + 1;
+
+int row_idx = 0;
+int num_rows_left = num_visible_rows;
+m_root.Draw(window, m_first_visible_row, m_selected_row_idx, row_idx,
+num_rows_left);
+// Get the sel

[Lldb-commits] [PATCH] D107182: [LLDB][GUI] Refactor form drawing using subsurfaces

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new method SubSurface to the Surface class. The method
returns another surface that is a subset of this surface. This is
important to further abstract away drawing from the ncurses objects. For
instance, fields could previously be drawn on subpads only but can now
be drawn on any surface. This is needed to create the file search
dialogs and similar functionalities.

There is an opportunity to refactor window drawing in general using
surfaces, but we shall consider this separately later.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D107182

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -352,6 +352,19 @@
 
   operator WINDOW *() { return m_window; }
 
+  Surface SubSurface(Rect bounds) {
+Surface subSurface;
+if (is_pad(m_window))
+  subSurface.m_window =
+  ::subpad(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+else
+  subSurface.m_window =
+  ::derwin(m_window, bounds.size.height, bounds.size.width,
+   bounds.origin.y, bounds.origin.x);
+return subSurface;
+  }
+
   // Copy a region of the surface to another surface.
   void CopyToSurface(Surface &target, Point source_origin, Point target_origin,
  Size size) {
@@ -1025,7 +1038,7 @@
   // Draw the field in the given subpad surface. The surface have a height that
   // is equal to the height returned by FieldDelegateGetHeight(). If the field
   // is selected in the form window, then is_selected will be true.
-  virtual void FieldDelegateDraw(SubPad &surface, bool is_selected) = 0;
+  virtual void FieldDelegateDraw(Surface &surface, bool is_selected) = 0;
 
   // Handle the key that wasn't handled by the form window or a container field.
   virtual HandleCharResult FieldDelegateHandleChar(int key) {
@@ -1112,7 +1125,7 @@
 
   int GetContentLength() { return m_content.length(); }
 
-  void DrawContent(SubPad &surface, bool is_selected) {
+  void DrawContent(Surface &surface, bool is_selected) {
 surface.MoveCursor(0, 0);
 const char *text = m_content.c_str() + m_first_visibile_char;
 surface.PutCString(text, surface.GetWidth());
@@ -1131,17 +1144,17 @@
   surface.AttributeOff(A_REVERSE);
   }
 
-  void DrawField(SubPad &surface, bool is_selected) {
+  void DrawField(Surface &surface, bool is_selected) {
 surface.TitledBox(m_label.c_str());
 
 Rect content_bounds = surface.GetFrame();
 content_bounds.Inset(1, 1);
-SubPad content_surface = SubPad(surface, content_bounds);
+Surface content_surface = surface.SubSurface(content_bounds);
 
 DrawContent(content_surface, is_selected);
   }
 
-  void DrawError(SubPad &surface) {
+  void DrawError(Surface &surface) {
 if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
@@ -1152,12 +1165,12 @@
 surface.AttributeOff(COLOR_PAIR(RedOnBlack));
   }
 
-  void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+  void FieldDelegateDraw(Surface &surface, bool is_selected) override {
 Rect frame = surface.GetFrame();
 Rect field_bounds, error_bounds;
 frame.HorizontalSplit(GetFieldHeight(), field_bounds, error_bounds);
-SubPad field_surface = SubPad(surface, field_bounds);
-SubPad error_surface = SubPad(surface, error_bounds);
+Surface field_surface = surface.SubSurface(field_bounds);
+Surface error_surface = surface.SubSurface(error_bounds);
 
 DrawField(field_surface, is_selected);
 DrawError(error_surface);
@@ -1406,7 +1419,7 @@
   // Boolean fields are have a single line.
   int FieldDelegateGetHeight() override { return 1; }
 
-  void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+  void FieldDelegateDraw(Surface &surface, bool is_selected) override {
 surface.MoveCursor(0, 0);
 surface.PutChar('[');
 if (is_selected)
@@ -1486,7 +1499,7 @@
 return std::min(index, GetNumberOfChoices()) - 1;
   }
 
-  void DrawContent(SubPad &surface, bool is_selected) {
+  void DrawContent(Surface &surface, bool is_selected) {
 int choices_to_draw = GetLastVisibleChoice() - m_first_visibile_choice + 1;
 for (int i = 0; i < choices_to_draw; i++) {
   surface.MoveCursor(0, i);
@@ -1502,14 +1515,14 @@
 }
   }
 
-  void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+  void FieldDelegateDraw(Surface &surface, bool is_selected) override {
 UpdateScrolling();
 
 surface.TitledBox(m_label.c_str());
 
 Rect content_bounds = surface.GetFrame();
 

[Lldb-commits] [PATCH] D106999: [LLDB][GUI] Add Environment Variable Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev marked an inline comment as done.
OmarEmaraDev added a comment.

I generalized the implementation as suggested and added a ready to use list 
version of the field.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106999

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106999: [LLDB][GUI] Add Environment Variable Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 362755.
OmarEmaraDev added a comment.

- Generalize the implementation to any Mapping field


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106999

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1908,6 +1908,176 @@
   SelectionType m_selection_type;
 };
 
+template 
+class MappingFieldDelegate : public FieldDelegate {
+public:
+  MappingFieldDelegate(KeyFieldDelegateType key_field,
+   ValueFieldDelegateType value_field)
+  : m_key_field(key_field), m_value_field(value_field),
+m_selection_type(SelectionType::Key) {}
+
+  // Signify which element is selected. The key field or its value field.
+  enum class SelectionType { Key, Value };
+
+  // A mapping field is drawn as two text fields with a right arrow in between.
+  // The first field stores the key of the mapping and the second stores the
+  // value if the mapping.
+  //
+  // __[Key]_   __[Value]___
+  // |  | > |  |
+  // |__|   |__|
+  // - Error message if it exists.
+
+  // The mapping field has a height that is equal to the maximum height between
+  // the key and value fields.
+  int FieldDelegateGetHeight() override {
+return std::max(m_key_field.FieldDelegateGetHeight(),
+m_value_field.FieldDelegateGetHeight());
+  }
+
+  void DrawArrow(SubPad &surface) {
+surface.MoveCursor(0, 1);
+surface.PutChar(ACS_RARROW);
+  }
+
+  void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+Rect bounds = surface.GetFrame();
+Rect key_field_bounds, arrow_and_value_field_bounds;
+bounds.VerticalSplit(bounds.size.width / 2, key_field_bounds,
+ arrow_and_value_field_bounds);
+Rect arrow_bounds, value_field_bounds;
+arrow_and_value_field_bounds.VerticalSplit(1, arrow_bounds,
+   value_field_bounds);
+
+SubPad key_field_surface = SubPad(surface, key_field_bounds);
+SubPad arrow_surface = SubPad(surface, arrow_bounds);
+SubPad value_field_surface = SubPad(surface, value_field_bounds);
+
+bool key_is_selected =
+m_selection_type == SelectionType::Key && is_selected;
+m_key_field.FieldDelegateDraw(key_field_surface, key_is_selected);
+DrawArrow(arrow_surface);
+bool value_is_selected =
+m_selection_type == SelectionType::Value && is_selected;
+m_value_field.FieldDelegateDraw(value_field_surface, value_is_selected);
+  }
+
+  HandleCharResult SelectNext(int key) {
+if (FieldDelegateOnLastOrOnlyElement())
+  return eKeyNotHandled;
+
+if (!m_key_field.FieldDelegateOnLastOrOnlyElement()) {
+  return m_key_field.FieldDelegateHandleChar(key);
+}
+
+m_key_field.FieldDelegateExitCallback();
+m_selection_type = SelectionType::Value;
+m_value_field.FieldDelegateSelectFirstElement();
+return eKeyHandled;
+  }
+
+  HandleCharResult SelectPrevious(int key) {
+if (FieldDelegateOnFirstOrOnlyElement())
+  return eKeyNotHandled;
+
+if (!m_value_field.FieldDelegateOnFirstOrOnlyElement()) {
+  return m_value_field.FieldDelegateHandleChar(key);
+}
+
+m_value_field.FieldDelegateExitCallback();
+m_selection_type = SelectionType::Key;
+m_key_field.FieldDelegateSelectLastElement();
+return eKeyHandled;
+  }
+
+  HandleCharResult FieldDelegateHandleChar(int key) override {
+switch (key) {
+case '\t':
+  SelectNext(key);
+  return eKeyHandled;
+case KEY_SHIFT_TAB:
+  SelectPrevious(key);
+  return eKeyHandled;
+default:
+  break;
+}
+
+// If the key wasn't handled, pass the key to the selected field.
+if (m_selection_type == SelectionType::Key)
+  return m_key_field.FieldDelegateHandleChar(key);
+else
+  return m_value_field.FieldDelegateHandleChar(key);
+
+return eKeyNotHandled;
+  }
+
+  bool FieldDelegateOnFirstOrOnlyElement() override {
+return m_selection_type == SelectionType::Key;
+  }
+
+  bool FieldDelegateOnLastOrOnlyElement() override {
+return m_selection_type == SelectionType::Value;
+  }
+
+  void FieldDelegateSelectFirstElement() override {
+m_selection_type = SelectionType::Key;
+  }
+
+  void FieldDelegateSelectLastElement() override {
+m_selection_type = SelectionType::Value;
+  }
+
+  bool FieldDelegateHasError() override {
+return m_key_field.FieldDelegateHasError() ||
+   m_value_field.FieldDelegateHasError();
+  }
+
+  KeyFieldDelegateType &GetKeyField() { return m_key_field; }
+
+  ValueFieldDelegateType &GetValueField() { return m_value_field; }
+
+protected:
+  KeyFieldDelegateTyp

[Lldb-commits] [PATCH] D106999: [LLDB][GUI] Add Environment Variable Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

Example list of environment fields:

F18194751: 20210728-221522.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106999

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106999: [LLDB][GUI] Add Environment Variable Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds an environment variable field. This is usually used as
the basic type of a List field. This is needed to create the process
launch form.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106999

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1908,6 +1908,144 @@
   SelectionType m_selection_type;
 };
 
+class EnvironmentVariableNameFieldDelegate : public TextFieldDelegate {
+public:
+  EnvironmentVariableNameFieldDelegate(const char *label, const char *content)
+  : TextFieldDelegate(label, content, true) {}
+
+  // Environment variable names can't contain an equal sign.
+  bool IsAcceptableChar(int key) override {
+return TextFieldDelegate::IsAcceptableChar(key) && key != '=';
+  }
+
+  const std::string &GetName() { return m_content; }
+};
+
+class EnvironmentVariableFieldDelegate : public FieldDelegate {
+public:
+  EnvironmentVariableFieldDelegate()
+  : m_name_field(EnvironmentVariableNameFieldDelegate("Name", "")),
+m_value_field(TextFieldDelegate("Value", "", /*required=*/false)),
+m_selection_type(SelectionType::Name) {}
+
+  // Signify which element is selected. The variable name field or its value
+  // field.
+  enum class SelectionType { Name, Value };
+
+  // An environment variable field is drawn as two text fields with a right
+  // arrow in between. The first text field stores the name of the variable and
+  // the second stores the value if the variable.
+  //
+  // __[Name]   __[Value]___
+  // |  | > |  |
+  // |__|   |__|
+  // - Error message if it exists.
+
+  // The environment variable field has a height that is equal to the maximum
+  // height between the name and value fields.
+  int FieldDelegateGetHeight() override {
+return std::max(m_name_field.FieldDelegateGetHeight(),
+m_value_field.FieldDelegateGetHeight());
+  }
+
+  void DrawArrow(SubPad &surface) {
+surface.MoveCursor(0, 1);
+surface.PutChar(ACS_RARROW);
+  }
+
+  void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+Rect bounds = surface.GetFrame();
+Rect name_field_bounds, arrow_and_value_field_bounds;
+bounds.VerticalSplit(bounds.size.width / 2, name_field_bounds,
+ arrow_and_value_field_bounds);
+Rect arrow_bounds, value_field_bounds;
+arrow_and_value_field_bounds.VerticalSplit(1, arrow_bounds,
+   value_field_bounds);
+
+SubPad name_field_surface = SubPad(surface, name_field_bounds);
+SubPad arrow_surface = SubPad(surface, arrow_bounds);
+SubPad value_field_surface = SubPad(surface, value_field_bounds);
+
+bool name_is_selected =
+m_selection_type == SelectionType::Name && is_selected;
+m_name_field.FieldDelegateDraw(name_field_surface, name_is_selected);
+DrawArrow(arrow_surface);
+bool value_is_selected =
+m_selection_type == SelectionType::Value && is_selected;
+m_value_field.FieldDelegateDraw(value_field_surface, value_is_selected);
+  }
+
+  HandleCharResult SelectNext(int key) {
+if (FieldDelegateOnLastOrOnlyElement())
+  return eKeyNotHandled;
+
+m_selection_type = SelectionType::Value;
+m_name_field.FieldDelegateExitCallback();
+return eKeyHandled;
+  }
+
+  HandleCharResult SelectPrevious(int key) {
+if (FieldDelegateOnFirstOrOnlyElement())
+  return eKeyNotHandled;
+
+m_selection_type = SelectionType::Name;
+m_value_field.FieldDelegateExitCallback();
+return eKeyHandled;
+  }
+
+  HandleCharResult FieldDelegateHandleChar(int key) override {
+switch (key) {
+case '\t':
+  SelectNext(key);
+  return eKeyHandled;
+case KEY_SHIFT_TAB:
+  SelectPrevious(key);
+  return eKeyHandled;
+default:
+  break;
+}
+
+// If the key wasn't handled, pass the key to the selected field.
+if (m_selection_type == SelectionType::Name)
+  return m_name_field.FieldDelegateHandleChar(key);
+else
+  return m_value_field.FieldDelegateHandleChar(key);
+
+return eKeyNotHandled;
+  }
+
+  bool FieldDelegateOnFirstOrOnlyElement() override {
+return m_selection_type == SelectionType::Name;
+  }
+
+  bool FieldDelegateOnLastOrOnlyElement() override {
+return m_selection_type == SelectionType::Value;
+  }
+
+  void FieldDelegateSelectFirstElement() override {
+m_selection_type = SelectionType::Name;
+  }
+
+  void FieldDelegateSelectLastElement() override {
+m_selec

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 362450.
OmarEmaraDev added a comment.

- Address review comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -36,6 +36,7 @@
 
 #include "lldb/Interpreter/CommandCompletions.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionGroupPlatform.h"
 
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
@@ -2651,6 +2652,185 @@
   ProcessPluginFieldDelegate *m_plugin_field;
 };
 
+class TargetCreateFormDelegate : public FormDelegate {
+public:
+  TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+m_executable_field = AddFileField("Executable", "", /*need_to_exist=*/true,
+  /*required=*/true);
+m_core_file_field = AddFileField("Core File", "", /*need_to_exist=*/true,
+ /*required=*/false);
+m_symbol_file_field = AddFileField(
+"Symbol File", "", /*need_to_exist=*/true, /*required=*/false);
+m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+m_remote_file_field = AddFileField(
+"Remote File", "", /*need_to_exist=*/false, /*required=*/false);
+m_arch_field = AddArchField("Architecture", "", /*required=*/false);
+m_platform_field = AddPlatformPluginField(debugger);
+m_load_dependent_files_field =
+AddChoicesField("Load Dependents", 3, GetLoadDependentFilesChoices());
+
+AddAction("Create", [this](Window &window) { CreateTarget(window); });
+  }
+
+  std::string GetName() override { return "Create Target"; }
+
+  void UpdateFieldsVisibility() override {
+if (m_show_advanced_field->GetBoolean()) {
+  m_remote_file_field->FieldDelegateShow();
+  m_arch_field->FieldDelegateShow();
+  m_platform_field->FieldDelegateShow();
+  m_load_dependent_files_field->FieldDelegateShow();
+} else {
+  m_remote_file_field->FieldDelegateHide();
+  m_arch_field->FieldDelegateHide();
+  m_platform_field->FieldDelegateHide();
+  m_load_dependent_files_field->FieldDelegateHide();
+}
+  }
+
+  static constexpr const char *kLoadDependentFilesNo = "No";
+  static constexpr const char *kLoadDependentFilesYes = "Yes";
+  static constexpr const char *kLoadDependentFilesExecOnly = "Executable only";
+
+  std::vector GetLoadDependentFilesChoices() {
+std::vector load_depentents_options;
+load_depentents_options.push_back(kLoadDependentFilesExecOnly);
+load_depentents_options.push_back(kLoadDependentFilesYes);
+load_depentents_options.push_back(kLoadDependentFilesNo);
+return load_depentents_options;
+  }
+
+  LoadDependentFiles GetLoadDependentFiles() {
+std::string choice = m_load_dependent_files_field->GetChoiceContent();
+if (choice == kLoadDependentFilesNo)
+  return eLoadDependentsNo;
+if (choice == kLoadDependentFilesYes)
+  return eLoadDependentsYes;
+return eLoadDependentsDefault;
+  }
+
+  OptionGroupPlatform GetPlatformOptions() {
+OptionGroupPlatform platform_options(false);
+platform_options.SetPlatformName(m_platform_field->GetPluginName().c_str());
+return platform_options;
+  }
+
+  TargetSP GetTarget() {
+OptionGroupPlatform platform_options = GetPlatformOptions();
+TargetSP target_sp;
+Status status = m_debugger.GetTargetList().CreateTarget(
+m_debugger, m_executable_field->GetPath(),
+m_arch_field->GetArchString(), GetLoadDependentFiles(),
+&platform_options, target_sp);
+
+if (status.Fail()) {
+  SetError(status.AsCString());
+  return nullptr;
+}
+
+m_debugger.GetTargetList().SetSelectedTarget(target_sp);
+
+return target_sp;
+  }
+
+  void SetSymbolFile(TargetSP target_sp) {
+if (!m_symbol_file_field->IsSpecified())
+  return;
+
+ModuleSP module_sp(target_sp->GetExecutableModule());
+if (!module_sp)
+  return;
+
+module_sp->SetSymbolFileFileSpec(
+m_symbol_file_field->GetResolvedFileSpec());
+  }
+
+  void SetCoreFile(TargetSP target_sp) {
+if (!m_core_file_field->IsSpecified())
+  return;
+
+FileSpec core_file_spec = m_core_file_field->GetResolvedFileSpec();
+
+FileSpec core_file_directory_spec;
+core_file_directory_spec.GetDirectory() = core_file_spec.GetDirectory();
+target_sp->AppendExecutableSearchPaths(core_file_directory_spec);
+
+ProcessSP process_sp(target_sp->CreateProcess(
+m_debugger.GetListener(), llvm::StringRef(), &core_file_spec, false));
+
+if (!process_sp) {
+  SetError("Unable to find process plug-in for core file!");
+  return;
+}
+
+Status status =

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

I still can't get remote debugging to work unfortunately, or maybe I don't 
understand it really. The way I understand it is as follows:

- If the remote file is specified, then that means we are creating a target for 
remote debugging.
- If the remote file doesn't exist, we upload the executable to the location 
specified by the remote file.
- If the remote file exists but the executable doesn't exist, then the remote 
file is downloaded to the local file. (Why? And does that mean the executable 
needn't exist if the remote file is specified? And also, does that mean the 
executable isn't a required field?)
- Everything happens using the platform of the created target, which is 
specified in the platform field. (When/Where does the connection with the 
server get established? `platform connect` connects the currently selected 
platform, but the target have its own platform which may not necessary be the 
currently selected one.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 362189.
OmarEmaraDev added a comment.

- Rebase on main.
- Add basic remote debugging support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -36,6 +36,7 @@
 
 #include "lldb/Interpreter/CommandCompletions.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionGroupPlatform.h"
 
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
@@ -2651,6 +2652,195 @@
   ProcessPluginFieldDelegate *m_plugin_field;
 };
 
+class TargetCreateFormDelegate : public FormDelegate {
+public:
+  TargetCreateFormDelegate(Debugger &debugger) : m_debugger(debugger) {
+m_executable_field = AddFileField("Executable", "", true, true);
+m_core_file_field = AddFileField("Core File", "", true, false);
+m_symbol_file_field = AddFileField("Symbol File", "", true, false);
+m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+m_remote_file_field = AddFileField("Remote File", "", false, false);
+m_arch_field = AddArchField("Architecture", "", false);
+m_platform_field = AddPlatformPluginField(debugger);
+m_load_dependent_files_field =
+AddChoicesField("Load Dependents", 3, GetLoadDependentFilesChoices());
+
+AddAction("Create", [this](Window &window) { CreateTarget(window); });
+  }
+
+  std::string GetName() override { return "Create Target"; }
+
+  void UpdateFieldsVisibility() override {
+if (m_show_advanced_field->GetBoolean()) {
+  m_remote_file_field->FieldDelegateShow();
+  m_arch_field->FieldDelegateShow();
+  m_platform_field->FieldDelegateShow();
+  m_load_dependent_files_field->FieldDelegateShow();
+} else {
+  m_remote_file_field->FieldDelegateHide();
+  m_arch_field->FieldDelegateHide();
+  m_platform_field->FieldDelegateHide();
+  m_load_dependent_files_field->FieldDelegateHide();
+}
+  }
+
+  std::vector GetLoadDependentFilesChoices() {
+std::vector load_depentents_options;
+load_depentents_options.push_back(
+std::string("Only if the \"Executable\" is an executable file."));
+load_depentents_options.push_back(std::string("Yes."));
+load_depentents_options.push_back(std::string("No."));
+return load_depentents_options;
+  }
+
+  LoadDependentFiles GetLoadDependentFiles() {
+std::string choice = m_load_dependent_files_field->GetChoiceContent();
+if (choice == "No.")
+  return eLoadDependentsNo;
+if (choice == "Yes.")
+  return eLoadDependentsYes;
+return eLoadDependentsDefault;
+  }
+
+  OptionGroupPlatform GetPlatformOptions() {
+OptionGroupPlatform platform_options(false);
+platform_options.SetPlatformName(m_platform_field->GetPluginName().c_str());
+return platform_options;
+  }
+
+  TargetSP GetTarget() {
+OptionGroupPlatform platform_options = GetPlatformOptions();
+TargetSP target_sp;
+Status status = m_debugger.GetTargetList().CreateTarget(
+m_debugger, m_executable_field->GetPath(),
+m_arch_field->GetArchString(), GetLoadDependentFiles(),
+&platform_options, target_sp);
+
+if (status.Fail()) {
+  SetError(status.AsCString());
+  return nullptr;
+}
+
+m_debugger.GetTargetList().SetSelectedTarget(target_sp);
+
+return target_sp;
+  }
+
+  void SetSymbolFile(TargetSP target_sp) {
+if (!m_symbol_file_field->IsSpecified())
+  return;
+
+ModuleSP module_sp(target_sp->GetExecutableModule());
+if (!module_sp)
+  return;
+
+module_sp->SetSymbolFileFileSpec(
+m_symbol_file_field->GetResolvedFileSpec());
+  }
+
+  void SetCoreFile(TargetSP target_sp) {
+if (!m_core_file_field->IsSpecified())
+  return;
+
+FileSpec core_file_spec = m_core_file_field->GetResolvedFileSpec();
+
+FileSpec core_file_directory_spec;
+core_file_directory_spec.GetDirectory() = core_file_spec.GetDirectory();
+target_sp->AppendExecutableSearchPaths(core_file_directory_spec);
+
+ProcessSP process_sp(target_sp->CreateProcess(
+m_debugger.GetListener(), llvm::StringRef(), &core_file_spec, false));
+
+if (!process_sp) {
+  SetError("Unable to find process plug-in for core file!");
+  return;
+}
+
+Status status = process_sp->LoadCore();
+if (status.Fail()) {
+  SetError("Can't find plug-in for core file!");
+  return;
+}
+  }
+
+  void SetRemoteFile(TargetSP target_sp) {
+if (!m_remote_file_field->IsSpecified())
+  return;
+
+PlatformSP platform_sp = target_sp->GetPlatform();
+if (!platform_sp) {
+  SetError("No platform found for target!");
+  return;
+}
+
+Fil

[Lldb-commits] [PATCH] D106553: [LLDB][GUI] Resolve paths in file/directory fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@clayborg Did you check D106564 ? This is the 
last patch needed to do the rebase.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106553

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106553: [LLDB][GUI] Resolve paths in file/directory fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1308
 }
 if (FileSystem::Instance().IsDirectory(file)) {
   SetError("Not a file!");

clayborg wrote:
> This is checking for a directory, not a file
Not sure what you mean. It is setting an error if the file was in fact a 
directory,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106553

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106564: [LLDB][GUI] Add Arch Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds an Arch field that inputs and validates an arch spec.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106564

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1345,6 +1345,25 @@
   bool m_need_to_exist;
 };
 
+class ArchFieldDelegate : public TextFieldDelegate {
+public:
+  ArchFieldDelegate(const char *label, const char *content, bool required)
+  : TextFieldDelegate(label, content, required) {}
+
+  void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
+if (!GetArchSpec().IsValid())
+  SetError("Not a valid arch!");
+  }
+
+  const std::string &GetArchString() { return m_content; }
+
+  ArchSpec GetArchSpec() { return ArchSpec(GetArchString()); }
+};
+
 class BooleanFieldDelegate : public FieldDelegate {
 public:
   BooleanFieldDelegate(const char *label, bool content)
@@ -1919,6 +1938,14 @@
 return delegate;
   }
 
+  ArchFieldDelegate *AddArchField(const char *label, const char *content,
+  bool required) {
+ArchFieldDelegate *delegate =
+new ArchFieldDelegate(label, content, required);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   IntegerFieldDelegate *AddIntegerField(const char *label, int content,
 bool required) {
 IntegerFieldDelegate *delegate =


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1345,6 +1345,25 @@
   bool m_need_to_exist;
 };
 
+class ArchFieldDelegate : public TextFieldDelegate {
+public:
+  ArchFieldDelegate(const char *label, const char *content, bool required)
+  : TextFieldDelegate(label, content, required) {}
+
+  void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
+if (!GetArchSpec().IsValid())
+  SetError("Not a valid arch!");
+  }
+
+  const std::string &GetArchString() { return m_content; }
+
+  ArchSpec GetArchSpec() { return ArchSpec(GetArchString()); }
+};
+
 class BooleanFieldDelegate : public FieldDelegate {
 public:
   BooleanFieldDelegate(const char *label, bool content)
@@ -1919,6 +1938,14 @@
 return delegate;
   }
 
+  ArchFieldDelegate *AddArchField(const char *label, const char *content,
+  bool required) {
+ArchFieldDelegate *delegate =
+new ArchFieldDelegate(label, content, required);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   IntegerFieldDelegate *AddIntegerField(const char *label, int content,
 bool required) {
 IntegerFieldDelegate *delegate =
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106553: [LLDB][GUI] Resolve paths in file/directory fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch resolves the paths in the file/directory fields before
performing checks. Those checks are applied on the file system if
m_need_to_exist is true, so remote files can set this to false to avoid
performing host-side file system checks. Additionally, methods to get
a resolved and a direct file specs were added to be used by client code.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106553

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1297,8 +1297,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
   return;
 }
@@ -1308,7 +1311,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:
@@ -1327,8 +1340,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
   return;
 }
@@ -1338,7 +1354,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1297,8 +1297,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
   return;
 }
@@ -1308,7 +1311,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:
@@ -1327,8 +1340,11 @@
 if (!IsSpecified())
   return;
 
-FileSpec file(GetPath());
-if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
+if (!m_need_to_exist)
+  return;
+
+FileSpec file = GetResolvedFileSpec();
+if (!FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
   return;
 }
@@ -1338,7 +1354,17 @@
 }
   }
 
-  // Returns the path of the file.
+  FileSpec GetFileSpec() {
+FileSpec file_spec(GetPath());
+return file_spec;
+  }
+
+  FileSpec GetResolvedFileSpec() {
+FileSpec file_spec(GetPath());
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
   const std::string &GetPath() { return m_content; }
 
 protected:
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106459: [LLDB][GUI] Check fields validity in actions

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 360778.
OmarEmaraDev added a comment.

- Merge main and fix conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106459

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1098,7 +1101,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1138,7 +1141,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1239,6 +1242,8 @@
 return eKeyNotHandled;
   }
 
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
+
   void FieldDelegateExitCallback() override {
 if (!IsSpecified() && m_required)
   SetError("This field is required!");
@@ -1246,8 +1251,6 @@
 
   bool IsSpecified() { return !m_content.empty(); }
 
-  bool HasError() { return !m_error.empty(); }
-
   void ClearError() { m_error.clear(); }
 
   const std::string &GetError() { return m_error; }
@@ -1892,6 +1895,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of 
an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldDelegate *AddTextField(const char *label, const char *content,
@@ -2500,6 +2516,10 @@
   void Attach(Window &window) {
 ClearError();
 
+bool all_fields_are_valid = CheckFieldsValidity();
+if (!all_fields_are_valid)
+  return;
+
 bool process_is_running = StopRunningProcess();
 if (process_is_running)
   return;


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1098,7 +1101,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1138,7 +1141,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1239,6 +1242,8 @@
 return eKeyNotHandled;
   }
 
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
+
   void FieldDelegateExitCallback() override {
 if (!IsSpecified() && m_required)
   SetError("This field is required!");
@@ -1246,8 +1251,6 @@
 
   bool IsSpecified() { return !m_content.empty(); }
 
-  bool HasError() { return !m_error.empty(); }
-
   void ClearError() { m_error.clear(); }
 
   const std::string &GetError() { return m_error; }
@@ -1892,6 +1895,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldD

[Lldb-commits] [PATCH] D106483: [LLDB][GUI] Add Platform Plugin Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 360767.
OmarEmaraDev added a comment.

- Rebase on main and fix conflicts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106483

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1454,6 +1454,8 @@
   }
 
   void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+UpdateScrolling();
+
 surface.TitledBox(m_label.c_str());
 
 Rect content_bounds = surface.GetFrame();
@@ -1473,29 +1475,23 @@
   m_choice++;
   }
 
-  // If the cursor moved past the first visible choice, scroll up by one
-  // choice.
-  void ScrollUpIfNeeded() {
-if (m_choice < m_first_visibile_choice)
-  m_first_visibile_choice--;
-  }
+  void UpdateScrolling() {
+if (m_choice > GetLastVisibleChoice()) {
+  m_first_visibile_choice = m_choice - (m_number_of_visible_choices - 1);
+  return;
+}
 
-  // If the cursor moved past the last visible choice, scroll down by one
-  // choice.
-  void ScrollDownIfNeeded() {
-if (m_choice > GetLastVisibleChoice())
-  m_first_visibile_choice++;
+if (m_choice < m_first_visibile_choice)
+  m_first_visibile_choice = m_choice;
   }
 
   HandleCharResult FieldDelegateHandleChar(int key) override {
 switch (key) {
 case KEY_UP:
   SelectPrevious();
-  ScrollUpIfNeeded();
   return eKeyHandled;
 case KEY_DOWN:
   SelectNext();
-  ScrollDownIfNeeded();
   return eKeyHandled;
 default:
   break;
@@ -1509,6 +1505,15 @@
   // Returns the index of the choice.
   int GetChoice() { return m_choice; }
 
+  void SetChoice(const std::string &choice) {
+for (int i = 0; i < GetNumberOfChoices(); i++) {
+  if (choice == m_choices[i]) {
+m_choice = i;
+return;
+  }
+}
+  }
+
 protected:
   std::string m_label;
   int m_number_of_visible_choices;
@@ -1519,6 +1524,29 @@
   int m_first_visibile_choice;
 };
 
+class PlatformPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  PlatformPluginFieldDelegate(Debugger &debugger)
+  : ChoicesFieldDelegate("Platform Plugin", 3, GetPossiblePluginNames()) {
+PlatformSP platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
+if (platform_sp)
+  SetChoice(platform_sp->GetName().AsCString());
+  }
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+size_t i = 0;
+while (auto name = PluginManager::GetPlatformPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+return plugin_name;
+  }
+};
+
 class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
 public:
   ProcessPluginFieldDelegate()
@@ -1941,6 +1969,13 @@
 return delegate;
   }
 
+  PlatformPluginFieldDelegate *AddPlatformPluginField(Debugger &debugger) {
+PlatformPluginFieldDelegate *delegate =
+new PlatformPluginFieldDelegate(debugger);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   ProcessPluginFieldDelegate *AddProcessPluginField() {
 ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
 m_fields.push_back(FieldDelegateUP(delegate));
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106458: [LLDB][GUI] Add required property to text fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 360575.
OmarEmaraDev added a comment.

- Remove default arguments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106458

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1075,8 +1075,9 @@
 
 class TextFieldDelegate : public FieldDelegate {
 public:
-  TextFieldDelegate(const char *label, const char *content)
-  : m_label(label), m_cursor_position(0), m_first_visibile_char(0) {
+  TextFieldDelegate(const char *label, const char *content, bool required)
+  : m_label(label), m_required(required), m_cursor_position(0),
+m_first_visibile_char(0) {
 if (content)
   m_content = content;
   }
@@ -1238,6 +1239,13 @@
 return eKeyNotHandled;
   }
 
+  void FieldDelegateExitCallback() override {
+if (!IsSpecified() && m_required)
+  SetError("This field is required!");
+  }
+
+  bool IsSpecified() { return !m_content.empty(); }
+
   bool HasError() { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
@@ -1250,6 +1258,7 @@
 
 protected:
   std::string m_label;
+  bool m_required;
   // The position of the top left corner character of the border.
   std::string m_content;
   // The cursor position in the content string itself. Can be in the range
@@ -1266,8 +1275,8 @@
 
 class IntegerFieldDelegate : public TextFieldDelegate {
 public:
-  IntegerFieldDelegate(const char *label, int content)
-  : TextFieldDelegate(label, std::to_string(content).c_str()) {}
+  IntegerFieldDelegate(const char *label, int content, bool required)
+  : TextFieldDelegate(label, std::to_string(content).c_str(), required) {}
 
   // Only accept digits.
   bool IsAcceptableChar(int key) override { return isdigit(key); }
@@ -1278,13 +1287,16 @@
 
 class FileFieldDelegate : public TextFieldDelegate {
 public:
-  FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+  FileFieldDelegate(const char *label, const char *content, bool need_to_exist,
+bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
@@ -1306,12 +1318,15 @@
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist, bool required)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
@@ -1856,31 +1871,35 @@
 
   // Factory methods to create and add fields of specific types.
 
-  TextFieldDelegate *AddTextField(const char *label, const char *content) {
-TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
+  TextFieldDelegate *AddTextField(const char *label, const char *content,
+  bool required) {
+TextFieldDelegate *delegate =
+new TextFieldDelegate(label, content, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist, bool required) {
 FileFieldDelegate *delegate =
-new FileFieldDelegate(label, content, need_to_exist);
+new FileFieldDelegate(label, content, need_to_exist, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   DirectoryFieldDelegate *AddDirectoryField(const char *label,
 const char *content,
-bool need_to_exist = true) {
+bool need_to_exist, bool re

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

> Are you going to land the other smaller diffs first and then update this one 
> after hey have landed?

Yes. There is much dependency between patches, so the smaller ones will have to 
land first then we will rebase this one on main. Also, notice that the smaller 
patches might conflict with each other, so I will have to rebase some of them 
once other land.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2573
+m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
+m_arch_field = AddArchField("Architecture", "");
+std::vector load_depentents_options;

clayborg wrote:
> we should add a "Platform" TextField that is populated with all of the 
> platform names. The default value should be the currently selected platform. 
> To get a list of all platform plug-in names you can do this like you did for 
> the process plug-ins:
> 
> ```
>   static const char *GetPlatformPluginNameAtIndex(uint32_t idx);
> ```
> 
> To get the current selected platform:
> ```
> ConstString default_platform_name;
> lldb::PlatformSP platform_sp = debugger.GetPlatformList()GetSelectedPlatform()
> if (platform_sp)
>   default_platform_name = platform_sp->GetName();
> ```
> This will help with remote targets. The main reason we need this is many 
> linux or android binaries have very minimal target triples encoded into the 
> ELF files, so we often need to specify "remote-android" or "remote-linux" 
> when creating targets. The user can of course set the "Architecture" with a 
> fully specified triple, but they can also avoid setting the arch and just 
> specify the platform.
> 
The required field is implemented in D106483.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106483: [LLDB][GUI] Add Platform Plugin Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new Platform Plugin Field. It is a choices field that
lists all the available platform plugins and can retrieve the name of the
selected plugin. The default selected plugin is the currently selected
one. This patch also allows for arbitrary scrolling to make scrolling
easier when setting choices.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106483

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1439,6 +1439,8 @@
   }
 
   void FieldDelegateDraw(SubPad &surface, bool is_selected) override {
+UpdateScrolling();
+
 surface.TitledBox(m_label.c_str());
 
 Rect content_bounds = surface.GetFrame();
@@ -1458,29 +1460,23 @@
   m_choice++;
   }
 
-  // If the cursor moved past the first visible choice, scroll up by one
-  // choice.
-  void ScrollUpIfNeeded() {
-if (m_choice < m_first_visibile_choice)
-  m_first_visibile_choice--;
-  }
+  void UpdateScrolling() {
+if (m_choice > GetLastVisibleChoice()) {
+  m_first_visibile_choice = m_choice - (m_number_of_visible_choices - 1);
+  return;
+}
 
-  // If the cursor moved past the last visible choice, scroll down by one
-  // choice.
-  void ScrollDownIfNeeded() {
-if (m_choice > GetLastVisibleChoice())
-  m_first_visibile_choice++;
+if (m_choice < m_first_visibile_choice)
+  m_first_visibile_choice = m_choice;
   }
 
   HandleCharResult FieldDelegateHandleChar(int key) override {
 switch (key) {
 case KEY_UP:
   SelectPrevious();
-  ScrollUpIfNeeded();
   return eKeyHandled;
 case KEY_DOWN:
   SelectNext();
-  ScrollDownIfNeeded();
   return eKeyHandled;
 default:
   break;
@@ -1494,6 +1490,15 @@
   // Returns the index of the choice.
   int GetChoice() { return m_choice; }
 
+  void SetChoice(const std::string &choice) {
+for (int i = 0; i < GetNumberOfChoices(); i++) {
+  if (choice == m_choices[i]) {
+m_choice = i;
+return;
+  }
+}
+  }
+
 protected:
   std::string m_label;
   int m_number_of_visible_choices;
@@ -1504,6 +1509,29 @@
   int m_first_visibile_choice;
 };
 
+class PlatformPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  PlatformPluginFieldDelegate(Debugger &debugger)
+  : ChoicesFieldDelegate("Platform Plugin", 3, GetPossiblePluginNames()) {
+PlatformSP platform_sp = debugger.GetPlatformList().GetSelectedPlatform();
+if (platform_sp)
+  SetChoice(platform_sp->GetName().AsCString());
+  }
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+size_t i = 0;
+while (auto name = PluginManager::GetPlatformPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1927,13 @@
 return delegate;
   }
 
+  PlatformPluginFieldDelegate *AddPlatformPluginField(Debugger &debugger) {
+PlatformPluginFieldDelegate *delegate =
+new PlatformPluginFieldDelegate(debugger);
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106467: [LLDB][GUI] Add Process Plugin Field

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a new Process Plugin Field. It is a choices field that
lists all the available process plugins and can retrieve the name of the
selected plugin or an empty string if the default is selected.

The Attach form now uses that field instead of manually creating a
choices field.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106467

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1504,6 +1504,29 @@
   int m_first_visibile_choice;
 };
 
+class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  ProcessPluginFieldDelegate()
+  : ChoicesFieldDelegate("Process Plugin", 3, GetPossiblePluginNames()) {}
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+names.push_back("");
+
+size_t i = 0;
+while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+if (plugin_name == "")
+  return "";
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1922,12 @@
 return delegate;
   }
 
+  ProcessPluginFieldDelegate *AddProcessPluginField() {
+ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
@@ -2347,8 +2376,7 @@
 m_include_existing_field =
 AddBooleanField("Include existing processes.", false);
 m_show_advanced_field = AddBooleanField("Show advanced settings.", false);
-m_plugin_field =
-AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_plugin_field = AddProcessPluginField();
 
 AddAction("Attach", [this](Window &window) { Attach(window); });
   }
@@ -2390,16 +2418,6 @@
 return module_sp->GetFileSpec().GetFilename().AsCString();
   }
 
-  std::vector GetPossiblePluginNames() {
-std::vector names;
-names.push_back("");
-
-size_t i = 0;
-while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
-  names.push_back(name);
-return names;
-  }
-
   bool StopRunningProcess() {
 ExecutionContext exe_ctx =
 m_debugger.GetCommandInterpreter().GetExecutionContext();
@@ -2455,8 +2473,7 @@
 } else {
   attach_info.SetProcessID(m_pid_field->GetInteger());
 }
-if (m_plugin_field->GetChoiceContent() != "")
-  attach_info.SetProcessPluginName(m_plugin_field->GetChoiceContent());
+attach_info.SetProcessPluginName(m_plugin_field->GetPluginName());
 
 return attach_info;
   }
@@ -2504,7 +2521,7 @@
   BooleanFieldDelegate *m_wait_for_field;
   BooleanFieldDelegate *m_include_existing_field;
   BooleanFieldDelegate *m_show_advanced_field;
-  ChoicesFieldDelegate *m_plugin_field;
+  ProcessPluginFieldDelegate *m_plugin_field;
 };
 
 class MenuDelegate {


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1504,6 +1504,29 @@
   int m_first_visibile_choice;
 };
 
+class ProcessPluginFieldDelegate : public ChoicesFieldDelegate {
+public:
+  ProcessPluginFieldDelegate()
+  : ChoicesFieldDelegate("Process Plugin", 3, GetPossiblePluginNames()) {}
+
+  std::vector GetPossiblePluginNames() {
+std::vector names;
+names.push_back("");
+
+size_t i = 0;
+while (auto name = PluginManager::GetProcessPluginNameAtIndex(i++))
+  names.push_back(name);
+return names;
+  }
+
+  std::string GetPluginName() {
+std::string plugin_name = GetChoiceContent();
+if (plugin_name == "")
+  return "";
+return plugin_name;
+  }
+};
+
 template  class ListFieldDelegate : public FieldDelegate {
 public:
   ListFieldDelegate(const char *label, T default_field)
@@ -1899,6 +1922,12 @@
 return delegate;
   }
 
+  ProcessPluginFieldDelegate *AddProcessPluginField() {
+ProcessPluginFieldDelegate *delegate = new ProcessPluginFieldDelegate();
+m_fields.push_back(FieldDelegateUP(delegate));
+return delegate;
+  }
+
   template 
   ListFieldDelegate *AddListField(const char *label, T default_field) {
 ListFieldDelegate *delegate =
@@ -2347,8 +2376,7 @@
 m_include_existing_field =
 AddBooleanField("Include existing proc

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Thanks @DavidSpickett! I will look into this and let you know how it goes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

I am currently breaking this patch into smaller independent viable patches as 
suggested.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1309
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;

clayborg wrote:
> It should be a property of the FileFieldDelegate if the file is for the 
> current system. We might be specifying a file on a remote system, so it 
> doesn't always have to exist, nor should it be resolved on the current 
> system. When creating a target we would always want to specify this file 
> should exist on the current system. 
> 
> 
I was trying to add properties that indicates if the file is local or not and 
if it should be resolved or not. But they all seems identical to the 
need_to_exist property. If m_need_to_exist is false, then the file will not be 
resolved and will not be checked with the host file system (Which is what we 
want to remote files). If it is true, then it will be resolved and checked with 
the host file system (Which is what we want for local files). So adding more 
properties might be redundant? What do you think?



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1315
   bool m_need_to_exist;
+  bool m_required;
 };

clayborg wrote:
> Should this to into the FieldDelegate so we don't need to put it into both 
> FileFieldDelegate and DirectoryFieldDelegate? Then the default 
> FieldDelegate::FieldDelegateExitCallback() function could be:
> 
> ```
> virtual void FieldDelegate::FieldDelegateExitCallback() {
>   if (!m_required)
> return;
>   // Check value with another virtual function?
>   FieldDelegateValueIsValid();
> }
> ```
> This seems like many fields would want to require being set and we don't want 
> to re-invent this for each kind of field.
The fields that can have a required property is the TextField and its 
derivatives (File, Directory, and Number), so we can can add this logic there. 
This is implemented in D106458.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2609
+
+if (!FileSystem::Instance().Exists(file_spec)) {
+  SetError("File doesn't exist!");

clayborg wrote:
> Won't this be taken care of already by the File field? We should construct 
> the FileDelegate so that:
> - the file must exist
> - the file gets resolved on the local system
> - the file field is required
> This way, the only way for the user to get to the "Create" button if is if 
> they already were able to set a valid and required by the FileFieldDelegate 
> class.
D106459 implements the necessary bits to allow for this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106459: [LLDB][GUI] Check fields validity in actions

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a virtual method HasError to fields, it can be
overridden by fields that have errors. Additionally, a form method
CheckFieldsValidity was added to be called by actions that expects all
the field to be valid.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106459

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1097,7 +1100,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1137,7 +1140,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1238,7 +1241,7 @@
 return eKeyNotHandled;
   }
 
-  bool HasError() { return !m_error.empty(); }
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
 
@@ -1854,6 +1857,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of 
an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
@@ -2464,6 +2480,10 @@
   void Attach(Window &window) {
 ClearError();
 
+bool all_fields_are_valid = CheckFieldsValidity();
+if (!all_fields_are_valid)
+  return;
+
 bool process_is_running = StopRunningProcess();
 if (process_is_running)
   return;


Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1061,6 +1061,9 @@
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
 
+  // Returns true if the field has an error, false otherwise.
+  virtual bool FieldDelegateHasError() { return false; }
+
   bool FieldDelegateIsVisible() { return m_is_visible; }
 
   void FieldDelegateHide() { m_is_visible = false; }
@@ -1097,7 +1100,7 @@
   // field and an optional line for an error if it exists.
   int FieldDelegateGetHeight() override {
 int height = GetFieldHeight();
-if (HasError())
+if (FieldDelegateHasError())
   height++;
 return height;
   }
@@ -1137,7 +1140,7 @@
   }
 
   void DrawError(SubPad &surface) {
-if (!HasError())
+if (!FieldDelegateHasError())
   return;
 surface.MoveCursor(0, 0);
 surface.AttributeOn(COLOR_PAIR(RedOnBlack));
@@ -1238,7 +1241,7 @@
 return eKeyNotHandled;
   }
 
-  bool HasError() { return !m_error.empty(); }
+  bool FieldDelegateHasError() override { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
 
@@ -1854,6 +1857,19 @@
 
   void SetError(const char *error) { m_error = error; }
 
+  // If all fields are valid, true is returned. Otherwise, an error message is
+  // set and false is returned. This method is usually called at the start of an
+  // action that requires valid fields.
+  bool CheckFieldsValidity() {
+for (int i = 0; i < GetNumberOfFields(); i++) {
+  if (GetField(i)->FieldDelegateHasError()) {
+SetError("Some fields are invalid!");
+return false;
+  }
+}
+return true;
+  }
+
   // Factory methods to create and add fields of specific types.
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
@@ -2464,6 +2480,10 @@
   void Attach(Window &window) {
 ClearError();
 
+bool all_fields_are_valid = CheckFieldsValidity();
+if (!all_fields_are_valid)
+  return;
+

[Lldb-commits] [PATCH] D106458: [LLDB][GUI] Add required property to text fields

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a required property to text fields and their
derivatives. Additionally, the Process Name and PID fields in the attach
form were marked as required.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106458

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1075,8 +1075,10 @@
 
 class TextFieldDelegate : public FieldDelegate {
 public:
-  TextFieldDelegate(const char *label, const char *content)
-  : m_label(label), m_cursor_position(0), m_first_visibile_char(0) {
+  TextFieldDelegate(const char *label, const char *content,
+bool required = false)
+  : m_label(label), m_required(required), m_cursor_position(0),
+m_first_visibile_char(0) {
 if (content)
   m_content = content;
   }
@@ -1238,6 +1240,13 @@
 return eKeyNotHandled;
   }
 
+  void FieldDelegateExitCallback() override {
+if (!IsSpecified() && m_required)
+  SetError("This field is required!");
+  }
+
+  bool IsSpecified() { return !m_content.empty(); }
+
   bool HasError() { return !m_error.empty(); }
 
   void ClearError() { m_error.clear(); }
@@ -1250,6 +1259,7 @@
 
 protected:
   std::string m_label;
+  bool m_required;
   // The position of the top left corner character of the border.
   std::string m_content;
   // The cursor position in the content string itself. Can be in the range
@@ -1266,8 +1276,8 @@
 
 class IntegerFieldDelegate : public TextFieldDelegate {
 public:
-  IntegerFieldDelegate(const char *label, int content)
-  : TextFieldDelegate(label, std::to_string(content).c_str()) {}
+  IntegerFieldDelegate(const char *label, int content, bool required = false)
+  : TextFieldDelegate(label, std::to_string(content).c_str(), required) {}
 
   // Only accept digits.
   bool IsAcceptableChar(int key) override { return isdigit(key); }
@@ -1279,12 +1289,15 @@
 class FileFieldDelegate : public TextFieldDelegate {
 public:
   FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+bool need_to_exist = true, bool required = false)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
@@ -1306,12 +1319,15 @@
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist = true, bool required = false)
+  : TextFieldDelegate(label, content, required),
+m_need_to_exist(need_to_exist) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
+TextFieldDelegate::FieldDelegateExitCallback();
+if (!IsSpecified())
+  return;
+
 FileSpec file(GetPath());
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
@@ -1856,31 +1872,37 @@
 
   // Factory methods to create and add fields of specific types.
 
-  TextFieldDelegate *AddTextField(const char *label, const char *content) {
-TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
+  TextFieldDelegate *AddTextField(const char *label, const char *content,
+  bool required = false) {
+TextFieldDelegate *delegate =
+new TextFieldDelegate(label, content, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist = true,
+  bool required = false) {
 FileFieldDelegate *delegate =
-new FileFieldDelegate(label, content, need_to_exist);
+new FileFieldDelegate(label, content, need_to_exist, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

This doesn't support remote files yet, I am still having trouble testing those. 
Also, there is also an unrelated clang-format change, not sure if I should 
revert it or keep it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106192

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D106192: [LLDB][GUI] Add Create Target form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a Create Target form for the LLDB GUI. Additionally, an
Arch Field was introduced to input an arch and the file and directory
fields now have a required property.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D106192

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -36,6 +36,7 @@
 
 #include "lldb/Interpreter/CommandCompletions.h"
 #include "lldb/Interpreter/CommandInterpreter.h"
+#include "lldb/Interpreter/OptionGroupPlatform.h"
 
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
@@ -56,6 +57,7 @@
 #include "lldb/Utility/State.h"
 #endif
 
+#include "llvm/ADT/ScopeExit.h"
 #include "llvm/ADT/StringRef.h"
 
 #ifdef _WIN32
@@ -1279,13 +1281,15 @@
 class FileFieldDelegate : public TextFieldDelegate {
 public:
   FileFieldDelegate(const char *label, const char *content,
-bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+bool need_to_exist = true, bool required = true)
+  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist),
+m_required(required) {}
 
-  // Set appropriate error messages if the file doesn't exists or is, in fact, a
-  // directory.
   void FieldDelegateExitCallback() override {
-FileSpec file(GetPath());
+if (m_content.empty() && !m_required)
+  return;
+
+FileSpec file = GetFileSpec();
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("File doesn't exist!");
   return;
@@ -1296,23 +1300,33 @@
 }
   }
 
-  // Returns the path of the file.
   const std::string &GetPath() { return m_content; }
 
+  bool IsSpecified() { return !m_content.empty(); }
+
+  FileSpec GetFileSpec() {
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
 protected:
   bool m_need_to_exist;
+  bool m_required;
 };
 
 class DirectoryFieldDelegate : public TextFieldDelegate {
 public:
   DirectoryFieldDelegate(const char *label, const char *content,
- bool need_to_exist = true)
-  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist) {}
+ bool need_to_exist = true, bool required = true)
+  : TextFieldDelegate(label, content), m_need_to_exist(need_to_exist),
+m_required(required) {}
 
-  // Set appropriate error messages if the directory doesn't exists or is, in
-  // fact, a file.
   void FieldDelegateExitCallback() override {
-FileSpec file(GetPath());
+if (m_content.empty() && !m_required)
+  return;
+
+FileSpec file = GetFileSpec();
 if (m_need_to_exist && !FileSystem::Instance().Exists(file)) {
   SetError("Directory doesn't exist!");
   return;
@@ -1323,11 +1337,43 @@
 }
   }
 
-  // Returns the path of the file.
   const std::string &GetPath() { return m_content; }
 
+  bool IsSpecified() { return !m_content.empty(); }
+
+  FileSpec GetFileSpec() {
+FileSpec file_spec(m_content);
+FileSystem::Instance().Resolve(file_spec);
+return file_spec;
+  }
+
 protected:
   bool m_need_to_exist;
+  bool m_required;
+};
+
+class ArchFieldDelegate : public TextFieldDelegate {
+public:
+  ArchFieldDelegate(const char *label, const char *content,
+bool required = false)
+  : TextFieldDelegate(label, content), m_required(required) {}
+
+  void FieldDelegateExitCallback() override {
+if (m_content.empty() && !m_required)
+  return;
+
+if (GetArchSpec().IsValid())
+  return;
+
+SetError("Not a valid arch!");
+  }
+
+  const std::string &GetArchString() { return m_content; }
+
+  ArchSpec GetArchSpec() { return ArchSpec(GetArchString()); }
+
+protected:
+  bool m_required;
 };
 
 class BooleanFieldDelegate : public FieldDelegate {
@@ -1863,18 +1909,28 @@
   }
 
   FileFieldDelegate *AddFileField(const char *label, const char *content,
-  bool need_to_exist = true) {
+  bool need_to_exist = true,
+  bool required = true) {
 FileFieldDelegate *delegate =
-new FileFieldDelegate(label, content, need_to_exist);
+new FileFieldDelegate(label, content, need_to_exist, required);
 m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   DirectoryFieldDelegate *AddDirectoryField(const char *label,
 const char *content,
-bool need_

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2425
+return true;
+  }
+

clayborg wrote:
> Should be easy to verify if it is needed or not. If it is, we will need to 
> add it back.
Looks like it is actually needed. I added it it back.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 358766.
OmarEmaraDev marked an inline comment as done.
OmarEmaraDev added a comment.

- Manually continue if needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -40,6 +40,7 @@
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
@@ -608,9 +609,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,9 +1061,18 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
-typedef std::shared_ptr FieldDelegateSP;
+typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
@@ -1227,7 +1247,6 @@
 
   void SetError(const char *error) { m_error = error; }
 
-  // Returns the text content of the field.
   const std::string &GetText() { return m_content; }
 
 protected:
@@ -1634,7 +1653,7 @@
   m_selection_type = SelectionType::NewButton;
   }
 
-  HandleCharResult SelecteNext(int key) {
+  HandleCharResult SelectNext(int key) {
 if (m_selection_type == SelectionType::NewButton)
   return eKeyNotHandled;
 
@@ -1707,7 +1726,7 @@
   }
   break;
 case '\t':
-  SelecteNext(key);
+  SelectNext(key);
   return eKeyHandled;
 case KEY_SHIFT_TAB:
   SelectPrevious(key);
@@ -1812,7 +1831,15 @@
 
   virtual ~FormDelegate() = default;
 
-  FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
+  FieldDelegate *GetField(uint32_t field_index) {
+if (field_index < m_fields.size())
+  return m_fields[field_index].get();
+return nullptr;
+  }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
 
@@ -1832,8 +1859,7 @@
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
 TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1841,8 +1867,7 @@
   bool need_to_exist = true) {
 FileFieldDelegate *delegate =
 new FileFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1851,22 +1876,19 @@
 bool need_to_exist = true) {
 DirectoryFieldDelegate *delegate =
 new DirectoryFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
 IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   BooleanFieldDelegate *AddBooleanField(const char *label, bool content) {
 BooleanFieldDelegate *delegate = new BooleanFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1874,8 +1896,7 @@
 std::vector c

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Two actions with an option:

F17916215: 20210713-214929.png 




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1838
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 

clayborg wrote:
> You don't want to return a unique_ptr here, if you do something like:
> ```
> FieldDelegateUP field_up = form->GetField(1);
> ```
> You have now given away the one reference to this field to the local variable 
> and it will destruct the item when the local goes out of scope.
> 
> Easy solution: just return a pointer, not a unique_ptr. The FormDelegate owns 
> all of its fields, so it is ok to hand out a pointer as others should 
> temporarily use the FieldDelegate. Also we should bounds check "field_index".
I see, I though returning it as a reference wouldn't case such issues.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2298
+  DetachOrKillProcessFormDelegate(Process *process) : m_process(process) {
+if (process->GetShouldDetach()) {
+  SetError("There is a running process, detach from it?");

clayborg wrote:
> When we have interaction, we should allow the user to select kill or detach. 
> The GetShouldDetach is only for when the user doesn't interact. Can we use a 
> ChoicesField for this with the two options? We can even go a step further and 
> add three options: "Detach", "Detach (stopped)", and "Kill". This is easy 
> since lldb_private::Process has this option on detach:
> 
> ```
>   Status Process::Detach(bool keep_stopped);
> ```
> 
> When you detach with "keep_stopped" set to true the process will not resume. 
> This allows another debugger to be attached, or someone to run another tool 
> on the process in its suspended state.
> 
I think this might be a good place to add two actions with possible parameters, 
let me know if you think this would be a good idea.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2324
+}
+window.GetParent()->RemoveSubWindow(&window);
+  }

clayborg wrote:
> Does anyone remove the sub window if the user cancels this dialog?
Yes. This is done in `WindowDelegateHandleChar` of the `FormWindowDelegate`.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2342-2343
+AddTextField("Process Name", GetDefaultProcessName().c_str());
+m_plugin_field =
+AddChoicesField("Plugin Name", 3, GetPossiblePluginNames());
+m_continue_field = AddBooleanField("Continue once attached.", false);

clayborg wrote:
> most people when attaching won't ever specify the plug-in names. I wonder if 
> we should include an extra boolean field that enables this? Maybe:
> ```
> m_show_plugin_field = AddBooleanField("Show advanced settings.", false);
> ```
> And then show/hide any settings that aren't commonly used like the "Plugin 
> Name" field?
This what I do for the launch and target creation forms. In this case, I didn't 
think it was worth it because it was just one field. But now that it is a 
choice field that takes a bit more space, maybe it would be a good idea to do 
this here as well.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2418
+form_window_sp->SetDelegate(window_delegate_sp);
+
+return true;

clayborg wrote:
> Is the form for the kill/detach done by the time it reaches this code? If so, 
> what if the user cancels the Kill/Detach dialog? Can we return false here if 
> the user cancels?
No, the return boolean here essentially denotes that a form for 
killing/detaching was spawned and that the attach form shouldn't continue 
executing. Once the user executes an action or cancels, the user will end up in 
the same attach from, where the user can execute attach again where it will 
succeed without the kill/detach dialog.

I can't think of a way to make the form block execution of another form until 
it is done without resorting to complicated trickery.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 358399.
OmarEmaraDev marked 10 inline comments as done.
OmarEmaraDev added a comment.

- Return raw pointers instead of unique ones.
- Add Show Advance Settings option.
- Allow detaching and killing at the same time.
- Allow detaching while keeping process stopped.
- Handle review comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -40,6 +40,7 @@
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
@@ -608,9 +609,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,9 +1061,18 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
-typedef std::shared_ptr FieldDelegateSP;
+typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
@@ -1227,7 +1247,6 @@
 
   void SetError(const char *error) { m_error = error; }
 
-  // Returns the text content of the field.
   const std::string &GetText() { return m_content; }
 
 protected:
@@ -1634,7 +1653,7 @@
   m_selection_type = SelectionType::NewButton;
   }
 
-  HandleCharResult SelecteNext(int key) {
+  HandleCharResult SelectNext(int key) {
 if (m_selection_type == SelectionType::NewButton)
   return eKeyNotHandled;
 
@@ -1707,7 +1726,7 @@
   }
   break;
 case '\t':
-  SelecteNext(key);
+  SelectNext(key);
   return eKeyHandled;
 case KEY_SHIFT_TAB:
   SelectPrevious(key);
@@ -1812,7 +1831,15 @@
 
   virtual ~FormDelegate() = default;
 
-  FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
+  FieldDelegate *GetField(uint32_t field_index) {
+if (field_index < m_fields.size())
+  return m_fields[field_index].get();
+return nullptr;
+  }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
 
@@ -1832,8 +1859,7 @@
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
 TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1841,8 +1867,7 @@
   bool need_to_exist = true) {
 FileFieldDelegate *delegate =
 new FileFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1851,22 +1876,19 @@
 bool need_to_exist = true) {
 DirectoryFieldDelegate *delegate =
 new DirectoryFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
 IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   BooleanFieldDelegate *AddBooleanField(const char *label, bool content) {
 BooleanFieldDelegate *delegate = new BooleanFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_b

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Updated UI:

F17912164: 20210713-170719.png 

Kill/Detach form.

F17912163: 20210713-170734.png 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 358268.
OmarEmaraDev added a comment.

- Add kill/detach form.
- Fix forms with no fields.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -40,6 +40,7 @@
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
@@ -608,9 +609,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,9 +1061,18 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
-typedef std::shared_ptr FieldDelegateSP;
+typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
@@ -1227,7 +1247,6 @@
 
   void SetError(const char *error) { m_error = error; }
 
-  // Returns the text content of the field.
   const std::string &GetText() { return m_content; }
 
 protected:
@@ -1634,7 +1653,7 @@
   m_selection_type = SelectionType::NewButton;
   }
 
-  HandleCharResult SelecteNext(int key) {
+  HandleCharResult SelectNext(int key) {
 if (m_selection_type == SelectionType::NewButton)
   return eKeyNotHandled;
 
@@ -1707,7 +1726,7 @@
   }
   break;
 case '\t':
-  SelecteNext(key);
+  SelectNext(key);
   return eKeyHandled;
 case KEY_SHIFT_TAB:
   SelectPrevious(key);
@@ -1812,7 +1831,11 @@
 
   virtual ~FormDelegate() = default;
 
-  FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
 
@@ -1832,8 +1855,7 @@
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
 TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1841,8 +1863,7 @@
   bool need_to_exist = true) {
 FileFieldDelegate *delegate =
 new FileFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1851,22 +1872,19 @@
 bool need_to_exist = true) {
 DirectoryFieldDelegate *delegate =
 new DirectoryFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
 IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   BooleanFieldDelegate *AddBooleanField(const char *label, bool content) {
 BooleanFieldDelegate *delegate = new BooleanFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1874,8 +1892,7 @@
 std::vector choices) {
 ChoicesFieldDelegate *delegate =
 new ChoicesFieldDelegate(label, height, choices);

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev planned changes to this revision.
OmarEmaraDev added a comment.

Currently working on the detach/kill form.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2346
+  return;
+
+if (process->GetShouldDetach()) {

clayborg wrote:
> We might want to pop up a dialog here asking the user if they want to kill or 
> detach since we have a GUI. In the "process attach" command we confirm with 
> the user if the terminal is interactive. 
> A few ways we can do this:
> - popup a form asking the user to kill/detach/cancel when the user hits 
> submit on this form
> - pop up the dialog before even showing the this form and asking the user to 
> kill or detach, then show this form after that has been handled
> - not allow the Process->Attach to happen by disabling the menu item when a 
> process is running.
> 
> 
I think the first option is the best, because it delays this decision at the 
very end giving the user as much time as possible to cancel attaching without 
altering the debugger state.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2420-2421
+
+if (attach_info.GetContinueOnceAttached())
+  process_sp->Resume();
+

clayborg wrote:
> You shouldn't need to do this since you already called 
> ProcessAttachInfo::SetContinueOnceAttached(true) on the attach_info. LLDB 
> should take care of this for you already I think.
In the command code, there is this snippet of code at the end, so I though it 
might be necessary:

```lang=cpp
// This supports the use-case scenario of immediately continuing the
// process once attached.
if (m_options.attach_info.GetContinueOnceAttached())
  m_interpreter.HandleCommand("process continue", eLazyBoolNo, result);
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 358228.
OmarEmaraDev marked 10 inline comments as done.
OmarEmaraDev added a comment.

- Use unique pointers for field delegates.
- Fix typos in Select methods.
- Set the process name to the main executable name by default.
- Turn plugin name into a choice field.
- Update debugger target if a new one was created.
- Remove superfluous continue operation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -40,6 +40,7 @@
 #if LLDB_ENABLE_CURSES
 #include "lldb/Breakpoint/BreakpointLocation.h"
 #include "lldb/Core/Module.h"
+#include "lldb/Core/PluginManager.h"
 #include "lldb/Core/ValueObject.h"
 #include "lldb/Core/ValueObjectRegister.h"
 #include "lldb/Symbol/Block.h"
@@ -608,9 +609,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,9 +1061,18 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
-typedef std::shared_ptr FieldDelegateSP;
+typedef std::unique_ptr FieldDelegateUP;
 
 class TextFieldDelegate : public FieldDelegate {
 public:
@@ -1227,7 +1247,6 @@
 
   void SetError(const char *error) { m_error = error; }
 
-  // Returns the text content of the field.
   const std::string &GetText() { return m_content; }
 
 protected:
@@ -1634,7 +1653,7 @@
   m_selection_type = SelectionType::NewButton;
   }
 
-  HandleCharResult SelecteNext(int key) {
+  HandleCharResult SelectNext(int key) {
 if (m_selection_type == SelectionType::NewButton)
   return eKeyNotHandled;
 
@@ -1707,7 +1726,7 @@
   }
   break;
 case '\t':
-  SelecteNext(key);
+  SelectNext(key);
   return eKeyHandled;
 case KEY_SHIFT_TAB:
   SelectPrevious(key);
@@ -1812,7 +1831,11 @@
 
   virtual ~FormDelegate() = default;
 
-  FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
+  FieldDelegateUP &GetField(int field_index) { return m_fields[field_index]; }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
 
@@ -1832,8 +1855,7 @@
 
   TextFieldDelegate *AddTextField(const char *label, const char *content) {
 TextFieldDelegate *delegate = new TextFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1841,8 +1863,7 @@
   bool need_to_exist = true) {
 FileFieldDelegate *delegate =
 new FileFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
@@ -1851,22 +1872,19 @@
 bool need_to_exist = true) {
 DirectoryFieldDelegate *delegate =
 new DirectoryFieldDelegate(label, content, need_to_exist);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   IntegerFieldDelegate *AddIntegerField(const char *label, int content) {
 IntegerFieldDelegate *delegate = new IntegerFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp);
+m_fields.push_back(FieldDelegateUP(delegate));
 return delegate;
   }
 
   BooleanFieldDelegate *AddBooleanField(const char *label, bool content) {
 BooleanFieldDelegate *delegate = new BooleanFieldDelegate(label, content);
-FieldDelegateSP delegate_sp = FieldDelegateSP(delegate);
-m_fields.push_back(delegate_sp)

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

I initially created two forms for attach by name and attach by PID, because the 
options were divided between them. Today I tried to reimplement that such that 
it is a single form with a choices field at the top that determines if it is by 
name or PID. The fields are then shown or hidden based on that enum. 
Additionally, other fields can control the visibility of other fields, for 
instance, the "Include existing processes" field. I think it works fine and the 
UX looks good. Here is an example, what do you think?

F17834972: 20210708-223744.mp4 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105655

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D105655: [LLDB][GUI] Add Process Attach form

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds a form window to attach a process, either by PID or by
name. This patch also adds support for dynamic field visibility such
that the form delegate can hide or show certain fields based on some
conditions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D105655

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -608,9 +608,19 @@
   m_delete = del;
 }
   }
-  //
+
   // Get the rectangle in our parent window
   Rect GetBounds() const { return Rect(GetParentOrigin(), GetSize()); }
+
+  Rect GetCenteredRect(int width, int height) {
+Size size = GetSize();
+width = std::min(size.width, width);
+height = std::min(size.height, height);
+int x = (size.width - width) / 2;
+int y = (size.height - height) / 2;
+return Rect(Point(x, y), Size(width, height));
+  }
+
   int GetChar() { return ::wgetch(m_window); }
   Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
   int GetParentX() const { return getparx(m_window); }
@@ -1050,6 +1060,15 @@
 
   // Select the last element in the field if multiple elements exists.
   virtual void FieldDelegateSelectLastElement() { return; }
+
+  bool FieldDelegateIsVisible() { return m_is_visible; }
+
+  void FieldDelegateHide() { m_is_visible = false; }
+
+  void FieldDelegateShow() { m_is_visible = true; }
+
+protected:
+  bool m_is_visible = true;
 };
 
 typedef std::shared_ptr FieldDelegateSP;
@@ -1812,6 +1831,10 @@
 
   virtual ~FormDelegate() = default;
 
+  virtual std::string GetName() = 0;
+
+  virtual void UpdateFieldsVisibility() { return; }
+
   FieldDelegateSP &GetField(int field_index) { return m_fields[field_index]; }
 
   FormAction &GetAction(int action_index) { return m_actions[action_index]; }
@@ -1948,6 +1971,8 @@
 int height = 0;
 height += GetErrorHeight();
 for (int i = 0; i < m_delegate_sp->GetNumberOfFields(); i++) {
+  if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+continue;
   height += m_delegate_sp->GetField(i)->FieldDelegateGetHeight();
 }
 height += GetActionsHeight();
@@ -1963,6 +1988,8 @@
 
 int offset = GetErrorHeight();
 for (int i = 0; i < m_selection_index; i++) {
+  if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+continue;
   offset += m_delegate_sp->GetField(i)->FieldDelegateGetHeight();
 }
 context.Offset(offset);
@@ -2018,6 +2045,8 @@
 int width = surface.GetWidth();
 bool a_field_is_selected = m_selection_type == SelectionType::Field;
 for (int i = 0; i < m_delegate_sp->GetNumberOfFields(); i++) {
+  if (!m_delegate_sp->GetField(i)->FieldDelegateIsVisible())
+continue;
   bool is_field_selected = a_field_is_selected && m_selection_index == i;
   FieldDelegateSP &field = m_delegate_sp->GetField(i);
   int height = field->FieldDelegateGetHeight();
@@ -2080,10 +2109,12 @@
   }
 
   bool WindowDelegateDraw(Window &window, bool force) override {
+m_delegate_sp->UpdateFieldsVisibility();
 
 window.Erase();
 
-window.DrawTitleBox(window.GetName(), "Press Esc to cancel");
+window.DrawTitleBox(m_delegate_sp->GetName().c_str(),
+"Press Esc to cancel");
 
 Rect content_bounds = window.GetFrame();
 content_bounds.Inset(2, 2);
@@ -2093,6 +2124,21 @@
 return true;
   }
 
+  void SkipNextHiddenFields() {
+while (true) {
+  if (m_delegate_sp->GetField(m_selection_index)->FieldDelegateIsVisible())
+return;
+
+  if (m_selection_index == m_delegate_sp->GetNumberOfFields() - 1) {
+m_selection_type = SelectionType::Action;
+m_selection_index = 0;
+return;
+  }
+
+  m_selection_index++;
+}
+  }
+
   HandleCharResult SelecteNext(int key) {
 if (m_selection_type == SelectionType::Action) {
   if (m_selection_index < m_delegate_sp->GetNumberOfActions() - 1) {
@@ -2102,8 +2148,12 @@
 
   m_selection_index = 0;
   m_selection_type = SelectionType::Field;
-  FieldDelegateSP &next_field = m_delegate_sp->GetField(m_selection_index);
-  next_field->FieldDelegateSelectFirstElement();
+  SkipNextHiddenFields();
+  if (m_selection_type == SelectionType::Field) {
+FieldDelegateSP &next_field =
+m_delegate_sp->GetField(m_selection_index);
+next_field->FieldDelegateSelectFirstElement();
+  }
   return eKeyHandled;
 }
 
@@ -2121,13 +2171,31 @@
 }
 
 m_selection_index++;
+SkipNextHiddenFields();
 
-FieldDelegateSP &next_field = m_delegate_sp-

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

Full example:

F17684763: 20210630-220202.mp4 

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 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 *m_text_list_field;
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 355672.
OmarEmaraDev added a comment.
This revision is now accepted and ready to land.

- Add contextual scrolling support.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -71,6 +71,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 using namespace lldb;
@@ -85,6 +86,8 @@
 #define KEY_RETURN 10
 #define KEY_ESCAPE 27
 
+#define KEY_SHIFT_TAB (KEY_MAX + 1)
+
 namespace curses {
 class Menu;
 class MenuDelegate;
@@ -336,93 +339,52 @@
   int m_first_visible_line;
 };
 
-class Window {
+// A surface is an abstraction for something than can be drawn on. The surface
+// have a width, a height, a cursor position, and a multitude of drawing
+// operations. This type should be sub-classed to get an actually useful ncurses
+// object, such as a Window, SubWindow, Pad, or a SubPad.
+class Surface {
 public:
-  Window(const char *name)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(false),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {}
+  Surface() : m_window(nullptr) {}
 
-  Window(const char *name, WINDOW *w, bool del = true)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(del),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-if (w)
-  Reset(w);
-  }
+  WINDOW *get() { return m_window; }
 
-  Window(const char *name, const Rect &bounds)
-  : m_name(name), m_window(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(true),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-Reset(::newwin(bounds.size.height, bounds.size.width, bounds.origin.y,
-   bounds.origin.y));
-  }
+  operator WINDOW *() { return m_window; }
 
-  virtual ~Window() {
-RemoveSubWindows();
-Reset();
+  // Copy a region of the surface to another surface.
+  void CopyToSurface(Surface &target, Point source_origin, Point target_origin,
+ Size size) {
+::copywin(m_window, target.get(), source_origin.y, source_origin.x,
+  target_origin.y, target_origin.x,
+  target_origin.y + size.height - 1,
+  target_origin.x + size.width - 1, false);
   }
 
-  void Reset(WINDOW *w = nullptr, bool del = true) {
-if (m_window == w)
-  return;
-
-if (m_panel) {
-  ::del_panel(m_panel);
-  m_panel = nullptr;
-}
-if (m_window && m_delete) {
-  ::delwin(m_window);
-  m_window = nullptr;
-  m_delete = false;
-}
-if (w) {
-  m_window = w;
-  m_panel = ::new_panel(m_window);
-  m_delete = del;
-}
-  }
+  int GetCursorX() const { return getcurx(m_window); }
+  int GetCursorY() const { return getcury(m_window); }
+  void MoveCursor(int x, int y) { ::wmove(m_window, y, x); }
 
   void AttributeOn(attr_t attr) { ::wattron(m_window, attr); }
   void AttributeOff(attr_t attr) { ::wattroff(m_window, attr); }
-  void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
-::box(m_window, v_char, h_char);
-  }
-  void Clear() { ::wclear(m_window); }
-  void Erase() { ::werase(m_window); }
-  Rect GetBounds() const {
-return Rect(GetParentOrigin(), GetSize());
-  } // Get the rectangle in our parent window
-  int GetChar() { return ::wgetch(m_window); }
-  int GetCursorX() const { return getcurx(m_window); }
-  int GetCursorY() const { return getcury(m_window); }
-  Rect GetFrame() const {
-return Rect(Point(), GetSize());
-  } // Get our rectangle in our own coordinate system
-  Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
-  Size GetSize() const { return Size(GetWidth(), GetHeight()); }
-  int GetParentX() const { return getparx(m_window); }
-  int GetParentY() const { return getpary(m_window); }
+
   int GetMaxX() const { return getmaxx(m_window); }
   int GetMaxY() const { return getmaxy(m_window); }
   int GetWidth() const { return GetMaxX(); }
   int GetHeight() const { return GetMaxY(); }
-  void MoveCursor(int x, int y) { ::wmove(m_window, y, x); }
-  void MoveWindow(int x, int y) { MoveWindow(Point(x, y)); }
-  void Resize(int w, int h) { ::wresize(m_window, h, w); }
-  void Resize(const Size &size) {
-::wresize(m_window, size.height, size.width);
- 

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev planned changes to this revision.
OmarEmaraDev added a comment.

- 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 355364.
OmarEmaraDev added a comment.
This revision is now accepted and ready to land.

- 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -71,6 +71,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 
 using namespace lldb;
@@ -85,6 +86,8 @@
 #define KEY_RETURN 10
 #define KEY_ESCAPE 27
 
+#define KEY_SHIFT_TAB (KEY_MAX + 1)
+
 namespace curses {
 class Menu;
 class MenuDelegate;
@@ -336,93 +339,52 @@
   int m_first_visible_line;
 };
 
-class Window {
+// A surface is an abstraction for something than can be drawn on. The surface
+// have a width, a height, a cursor position, and a multitude of drawing
+// operations. This type should be sub-classed to get an actually useful ncurses
+// object, such as a Window, SubWindow, Pad, or a SubPad.
+class Surface {
 public:
-  Window(const char *name)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(false),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {}
+  Surface() : m_window(nullptr) {}
 
-  Window(const char *name, WINDOW *w, bool del = true)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(del),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-if (w)
-  Reset(w);
-  }
+  WINDOW *get() { return m_window; }
 
-  Window(const char *name, const Rect &bounds)
-  : m_name(name), m_window(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(true),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-Reset(::newwin(bounds.size.height, bounds.size.width, bounds.origin.y,
-   bounds.origin.y));
-  }
+  operator WINDOW *() { return m_window; }
 
-  virtual ~Window() {
-RemoveSubWindows();
-Reset();
+  // Copy a region of the surface to another surface.
+  void CopyToSurface(Surface &target, Point source_origin, Point target_origin,
+ Size size) {
+::copywin(m_window, target.get(), source_origin.y, source_origin.x,
+  target_origin.y, target_origin.x,
+  target_origin.y + size.height - 1,
+  target_origin.x + size.width - 1, false);
   }
 
-  void Reset(WINDOW *w = nullptr, bool del = true) {
-if (m_window == w)
-  return;
-
-if (m_panel) {
-  ::del_panel(m_panel);
-  m_panel = nullptr;
-}
-if (m_window && m_delete) {
-  ::delwin(m_window);
-  m_window = nullptr;
-  m_delete = false;
-}
-if (w) {
-  m_window = w;
-  m_panel = ::new_panel(m_window);
-  m_delete = del;
-}
-  }
+  int GetCursorX() const { return getcurx(m_window); }
+  int GetCursorY() const { return getcury(m_window); }
+  void MoveCursor(int x, int y) { ::wmove(m_window, y, x); }
 
   void AttributeOn(attr_t attr) { ::wattron(m_window, attr); }
   void AttributeOff(attr_t attr) { ::wattroff(m_window, attr); }
-  void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
-::box(m_window, v_char, h_char);
-  }
-  void Clear() { ::wclear(m_window); }
-  void Erase() { ::werase(m_window); }
-  Rect GetBounds() const {
-return Rect(GetParentOrigin(), GetSize());
-  } // Get the rectangle in our parent window
-  int GetChar() { return ::wgetch(m_window); }
-  int GetCursorX() const { return getcurx(m_window); }
-  int GetCursorY() const { return getcury(m_window); }
-  Rect GetFrame() const {
-return Rect(Point(), GetSize());
-  } // Get our rectangle in our own coordinate system
-  Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
-  Size GetSize() const { return Size(GetWidth(), GetHeight()); }
-  int GetParentX() const { return getparx(m_window); }
-  int GetParentY() const { return getpary(m_window); }
+
   int GetMaxX() const { return getmaxx(m_window); }
   int GetMaxY() const { return getmaxy(m_window); }
   int GetWidth() const { return GetMaxX(); }
   int GetHeight() const {

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev planned changes to this revision.
OmarEmaraDev added a comment.

Not available in this patch yet:

- Global error messages.
- Contextual scrolling.
- Action bar.
- Auto completion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 354364.
OmarEmaraDev added a comment.

- 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -336,93 +336,52 @@
   int m_first_visible_line;
 };
 
-class Window {
+// A surface is an abstraction for something than can be drawn on. The surface
+// have a width, a height, a cursor position, and a multitude of drawing
+// operations. This type should be sub-classed to get an actually useful ncurses
+// object, such as a Window, SubWindow, Pad, or a SubPad.
+class Surface {
 public:
-  Window(const char *name)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(false),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {}
+  Surface() : m_window(nullptr) {}
 
-  Window(const char *name, WINDOW *w, bool del = true)
-  : m_name(name), m_window(nullptr), m_panel(nullptr), m_parent(nullptr),
-m_subwindows(), m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(del),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-if (w)
-  Reset(w);
-  }
+  WINDOW *get() { return m_window; }
 
-  Window(const char *name, const Rect &bounds)
-  : m_name(name), m_window(nullptr), m_parent(nullptr), m_subwindows(),
-m_delegate_sp(), m_curr_active_window_idx(UINT32_MAX),
-m_prev_active_window_idx(UINT32_MAX), m_delete(true),
-m_needs_update(true), m_can_activate(true), m_is_subwin(false) {
-Reset(::newwin(bounds.size.height, bounds.size.width, bounds.origin.y,
-   bounds.origin.y));
-  }
+  operator WINDOW *() { return m_window; }
 
-  virtual ~Window() {
-RemoveSubWindows();
-Reset();
+  // Copy a region of the surface to another surface.
+  void CopyToSurface(Surface &target, Point source_origin, Point target_origin,
+ Size size) {
+::copywin(m_window, target.get(), source_origin.y, source_origin.x,
+  target_origin.y, target_origin.x,
+  target_origin.y + size.height - 1,
+  target_origin.x + size.width - 1, false);
   }
 
-  void Reset(WINDOW *w = nullptr, bool del = true) {
-if (m_window == w)
-  return;
-
-if (m_panel) {
-  ::del_panel(m_panel);
-  m_panel = nullptr;
-}
-if (m_window && m_delete) {
-  ::delwin(m_window);
-  m_window = nullptr;
-  m_delete = false;
-}
-if (w) {
-  m_window = w;
-  m_panel = ::new_panel(m_window);
-  m_delete = del;
-}
-  }
+  int GetCursorX() const { return getcurx(m_window); }
+  int GetCursorY() const { return getcury(m_window); }
+  void MoveCursor(int x, int y) { ::wmove(m_window, y, x); }
 
   void AttributeOn(attr_t attr) { ::wattron(m_window, attr); }
   void AttributeOff(attr_t attr) { ::wattroff(m_window, attr); }
-  void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
-::box(m_window, v_char, h_char);
-  }
-  void Clear() { ::wclear(m_window); }
-  void Erase() { ::werase(m_window); }
-  Rect GetBounds() const {
-return Rect(GetParentOrigin(), GetSize());
-  } // Get the rectangle in our parent window
-  int GetChar() { return ::wgetch(m_window); }
-  int GetCursorX() const { return getcurx(m_window); }
-  int GetCursorY() const { return getcury(m_window); }
-  Rect GetFrame() const {
-return Rect(Point(), GetSize());
-  } // Get our rectangle in our own coordinate system
-  Point GetParentOrigin() const { return Point(GetParentX(), GetParentY()); }
-  Size GetSize() const { return Size(GetWidth(), GetHeight()); }
-  int GetParentX() const { return getparx(m_window); }
-  int GetParentY() const { return getpary(m_window); }
+
   int GetMaxX() const { return getmaxx(m_window); }
   int GetMaxY() const { return getmaxy(m_window); }
   int GetWidth() const { return GetMaxX(); }
   int GetHeight() const { return GetMaxY(); }
-  void MoveCursor(int x, int y) { ::wmove(m_window, y, x); }
-  void MoveWindow(int x, int y) { MoveWindow(Point(x, y)); }
-  void Resize(int w, int h) { ::wresize(m_window, h, w); }
-  void Resize(const Size &size) {
-::wresize(m_window, size.height, size.width);
-  }
-  void PutChar(int ch) { ::waddch(m_window, ch); }
-  void PutCString(const char *s, int len = -1) { ::

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

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.




Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1396-1405
+  // 
+  // | |
+  // | |
+  // | Form elements here. |
+  // | |
+  // |...  |
+  // |-|

clayborg wrote:
> 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.
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.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1450
+// Draw the centered submit button.
+const char *button_text = "[Submit]";
+int x = (window.GetWidth() - sizeof(button_text) - 1) / 2;

clayborg wrote:
> 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?
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 353783.
OmarEmaraDev added a comment.

- Remove Field type and use FieldDelegate directly


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -392,6 +392,12 @@
   void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
 ::box(m_window, v_char, h_char);
   }
+  void VerticalLine(int n, chtype v_char = ACS_VLINE) {
+::wvline(m_window, v_char, n);
+  }
+  void HorizontalLine(int n, chtype h_char = ACS_HLINE) {
+::whline(m_window, h_char, n);
+  }
   void Clear() { ::wclear(m_window); }
   void Erase() { ::werase(m_window); }
   Rect GetBounds() const {
@@ -674,6 +680,36 @@
   AttributeOff(attr);
   }
 
+  void DrawBox(const Rect &bounds, chtype v_char = ACS_VLINE,
+   chtype h_char = ACS_HLINE) {
+MoveCursor(bounds.origin.x, bounds.origin.y);
+VerticalLine(bounds.size.height);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_ULCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1, bounds.origin.y);
+VerticalLine(bounds.size.height);
+PutChar(ACS_URCORNER);
+
+MoveCursor(bounds.origin.x, bounds.origin.y + bounds.size.height - 1);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_LLCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1,
+   bounds.origin.y + bounds.size.height - 1);
+PutChar(ACS_LRCORNER);
+  }
+
+  void DrawTitledBox(const Rect &bounds, const char *title,
+ chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
+DrawBox(bounds, v_char, h_char);
+int title_offset = 2;
+MoveCursor(bounds.origin.x + title_offset, bounds.origin.y);
+PutChar('[');
+PutCString(title, bounds.size.width - title_offset);
+PutChar(']');
+  }
+
   virtual void Draw(bool force) {
 if (m_delegate_sp && m_delegate_sp->WindowDelegateDraw(*this, force))
   return;
@@ -869,6 +905,627 @@
   const Window &operator=(const Window &) = delete;
 };
 
+/
+// Forms
+/
+
+class FieldDelegate {
+public:
+  virtual ~FieldDelegate() = default;
+
+  virtual Rect FieldDelegateGetBounds() = 0;
+
+  virtual void FieldDelegateDraw(Window &window, bool is_active) = 0;
+
+  virtual HandleCharResult FieldDelegateHandleChar(int key) {
+return eKeyNotHandled;
+  }
+
+  void FieldDelegateSetPageIndex(int page_index) { m_page_index = page_index; }
+
+  int FieldDelegateGetPageIndex() { return m_page_index; }
+
+protected:
+  // The index of the page this field belongs to.
+  int m_page_index;
+};
+
+typedef std::shared_ptr FieldDelegateSP;
+
+class TextFieldDelegate : public FieldDelegate {
+public:
+  TextFieldDelegate(const char *label, int width, Point origin,
+const char *content)
+  : m_label(label), m_width(width), m_origin(origin), m_cursor_position(0),
+m_first_visibile_char(0) {
+if (content)
+  m_content = content;
+assert(m_width > 2);
+  }
+
+  // Get the bounding box of the field. The text field has a height of 3, 2
+  // lines for borders and 1 for the content.
+  Rect FieldDelegateGetBounds() override {
+return Rect(m_origin, Size(m_width, 3));
+  }
+
+  // Get the start X position of the content in window space, without the
+  // borders.
+  int GetX() { return m_origin.x + 1; }
+
+  // Get the start Y position of the content in window space, without the
+  // borders.
+  int GetY() { return m_origin.y + 1; }
+
+  // Get the effective width of the field, without the borders.
+  int GetEffectiveWidth() { return m_width - 2; }
+
+  // Get the cursor position in window space.
+  int GetCursorWindowXPosition() {
+return GetX() + m_cursor_position - m_first_visibile_char;
+  }
+
+  int GetContentLength() { return m_content.length(); }
+
+  void FieldDelegateDraw(Window &window, bool is_active) override {
+// Draw label box.
+window.DrawTitledBox(FieldDelegateGetBounds(), m_label.c_str());
+
+// Draw content.
+window.MoveCursor(GetX(), GetY());
+const char *text = m_content.c_str() + m_first_visibile_char;
+window.PutCString(text, GetEffectiveWidth());
+
+// Highlight the cursor.
+window.MoveCursor(GetCursorWindowXPosition(), GetY());
+if (is_active)
+  window.AttributeOn(A_REVERSE);
+if (m_cursor_position == GetContentLength())
+  // Cursor is past the last character. Highlight an empty space.
+  window.PutChar(' ');
+else
+  window.PutChar(m_content[m_cursor_position]);
+if (is_active)
+  window.AttributeOff(A_REVERSE);
+  }
+
+  // The cursor is allowed to move one character past the string.
+  // m_cursor_position is in range [0, GetContentLength()].
+  v

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.

@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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 353087.
OmarEmaraDev added a comment.

- Add form pages.
- Handle review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -392,6 +392,12 @@
   void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
 ::box(m_window, v_char, h_char);
   }
+  void VerticalLine(int n, chtype v_char = ACS_VLINE) {
+::wvline(m_window, v_char, n);
+  }
+  void HorizontalLine(int n, chtype h_char = ACS_HLINE) {
+::whline(m_window, h_char, n);
+  }
   void Clear() { ::wclear(m_window); }
   void Erase() { ::werase(m_window); }
   Rect GetBounds() const {
@@ -674,6 +680,36 @@
   AttributeOff(attr);
   }
 
+  void DrawBox(const Rect &bounds, chtype v_char = ACS_VLINE,
+   chtype h_char = ACS_HLINE) {
+MoveCursor(bounds.origin.x, bounds.origin.y);
+VerticalLine(bounds.size.height);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_ULCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1, bounds.origin.y);
+VerticalLine(bounds.size.height);
+PutChar(ACS_URCORNER);
+
+MoveCursor(bounds.origin.x, bounds.origin.y + bounds.size.height - 1);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_LLCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1,
+   bounds.origin.y + bounds.size.height - 1);
+PutChar(ACS_LRCORNER);
+  }
+
+  void DrawTitledBox(const Rect &bounds, const char *title,
+ chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
+DrawBox(bounds, v_char, h_char);
+int title_offset = 2;
+MoveCursor(bounds.origin.x + title_offset, bounds.origin.y);
+PutChar('[');
+PutCString(title, bounds.size.width - title_offset);
+PutChar(']');
+  }
+
   virtual void Draw(bool force) {
 if (m_delegate_sp && m_delegate_sp->WindowDelegateDraw(*this, force))
   return;
@@ -869,6 +905,636 @@
   const Window &operator=(const Window &) = delete;
 };
 
+/
+// Forms
+/
+
+class FieldDelegate {
+public:
+  virtual ~FieldDelegate() = default;
+
+  virtual Rect FieldDelegateGetBounds() = 0;
+
+  virtual void FieldDelegateDraw(Window &window, bool is_active) = 0;
+
+  virtual HandleCharResult FieldDelegateHandleChar(int key) {
+return eKeyNotHandled;
+  }
+};
+
+typedef std::shared_ptr FieldDelegateSP;
+
+class TextFieldDelegate : public FieldDelegate {
+public:
+  TextFieldDelegate(const char *label, int width, Point origin,
+const char *content)
+  : m_label(label), m_width(width), m_origin(origin), m_cursor_position(0),
+m_first_visibile_char(0) {
+if (content)
+  m_content = content;
+assert(m_width > 2);
+  }
+
+  // Get the bounding box of the field. The text field has a height of 3, 2
+  // lines for borders and 1 for the content.
+  Rect FieldDelegateGetBounds() override {
+return Rect(m_origin, Size(m_width, 3));
+  }
+
+  // Get the start X position of the content in window space, without the
+  // borders.
+  int GetX() { return m_origin.x + 1; }
+
+  // Get the start Y position of the content in window space, without the
+  // borders.
+  int GetY() { return m_origin.y + 1; }
+
+  // Get the effective width of the field, without the borders.
+  int GetEffectiveWidth() { return m_width - 2; }
+
+  // Get the cursor position in window space.
+  int GetCursorWindowXPosition() {
+return GetX() + m_cursor_position - m_first_visibile_char;
+  }
+
+  int GetContentLength() { return m_content.length(); }
+
+  void FieldDelegateDraw(Window &window, bool is_active) override {
+// Draw label box.
+window.DrawTitledBox(FieldDelegateGetBounds(), m_label.c_str());
+
+// Draw content.
+window.MoveCursor(GetX(), GetY());
+const char *text = m_content.c_str() + m_first_visibile_char;
+window.PutCString(text, GetEffectiveWidth());
+
+// Highlight the cursor.
+window.MoveCursor(GetCursorWindowXPosition(), GetY());
+if (is_active)
+  window.AttributeOn(A_REVERSE);
+if (m_cursor_position == GetContentLength())
+  // Cursor is past the last character. Highlight an empty space.
+  window.PutChar(' ');
+else
+  window.PutChar(m_content[m_cursor_position]);
+if (is_active)
+  window.AttributeOff(A_REVERSE);
+  }
+
+  // The cursor is allowed to move one character past the string.
+  // m_cursor_position is in range [0, GetContentLength()].
+  void MoveCursorRight() {
+if (m_cursor_position < GetContentLength())
+  m_cursor_position++;
+  }
+
+  void MoveCursorLeft() {
+if (m_cursor_position > 0)
+  m_cursor_position--;
+  }
+
+  // If the cursor moved past the last visib

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 352778.
OmarEmaraDev added a comment.

- Always scroll left on removing a character


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -392,6 +392,12 @@
   void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
 ::box(m_window, v_char, h_char);
   }
+  void VerticalLine(int n, chtype v_char = ACS_VLINE) {
+::wvline(m_window, v_char, n);
+  }
+  void HorizontalLine(int n, chtype h_char = ACS_HLINE) {
+::whline(m_window, h_char, n);
+  }
   void Clear() { ::wclear(m_window); }
   void Erase() { ::werase(m_window); }
   Rect GetBounds() const {
@@ -674,6 +680,36 @@
   AttributeOff(attr);
   }
 
+  void DrawBox(const Rect &bounds, chtype v_char = ACS_VLINE,
+   chtype h_char = ACS_HLINE) {
+MoveCursor(bounds.origin.x, bounds.origin.y);
+VerticalLine(bounds.size.height);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_ULCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1, bounds.origin.y);
+VerticalLine(bounds.size.height);
+PutChar(ACS_URCORNER);
+
+MoveCursor(bounds.origin.x, bounds.origin.y + bounds.size.height - 1);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_LLCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1,
+   bounds.origin.y + bounds.size.height - 1);
+PutChar(ACS_LRCORNER);
+  }
+
+  void DrawTitledBox(const Rect &bounds, const char *title,
+ chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
+DrawBox(bounds, v_char, h_char);
+int title_offset = 2;
+MoveCursor(bounds.origin.x + title_offset, bounds.origin.y);
+PutChar('[');
+PutCString(title, bounds.size.width - title_offset);
+PutChar(']');
+  }
+
   virtual void Draw(bool force) {
 if (m_delegate_sp && m_delegate_sp->WindowDelegateDraw(*this, force))
   return;
@@ -869,6 +905,550 @@
   const Window &operator=(const Window &) = delete;
 };
 
+/
+// Forms
+/
+
+class FieldDelegate {
+public:
+  virtual ~FieldDelegate() = default;
+
+  virtual Rect FieldDelegateGetBounds() = 0;
+
+  virtual void FieldDelegateDraw(Window &window, bool is_active) = 0;
+
+  virtual HandleCharResult FieldDelegateHandleChar(int key) {
+return eKeyNotHandled;
+  }
+};
+
+typedef std::shared_ptr FieldDelegateSP;
+
+class TextFieldDelegate : public FieldDelegate {
+public:
+  TextFieldDelegate(const char *label, int width, Point origin,
+const char *content)
+  : m_label(label), m_width(width), m_origin(origin), m_cursor_position(0),
+m_first_visibile_char(0) {
+if (content)
+  m_content = content;
+assert(m_width > 2);
+  }
+
+  // Get the bounding box of the field. The text field has a height of 3, 2
+  // lines for borders and 1 for the content.
+  Rect FieldDelegateGetBounds() override {
+return Rect(m_origin, Size(m_width, 3));
+  }
+
+  // Get the start X position of the content in window space, without the
+  // borders.
+  int GetX() { return m_origin.x + 1; }
+
+  // Get the start Y position of the content in window space, without the
+  // borders.
+  int GetY() { return m_origin.y + 1; }
+
+  // Get the effective width of the field, without the borders.
+  int GetEffectiveWidth() { return m_width - 2; }
+
+  // Get the cursor position in window space.
+  int GetCursorWindowXPosition() {
+return GetX() + m_cursor_position - m_first_visibile_char;
+  }
+
+  int GetContentLength() { return (int)m_content.length(); }
+
+  void FieldDelegateDraw(Window &window, bool is_active) override {
+// Draw label box.
+window.DrawTitledBox(FieldDelegateGetBounds(), m_label.c_str());
+
+// Draw content.
+window.MoveCursor(GetX(), GetY());
+const char *text = m_content.c_str() + m_first_visibile_char;
+window.PutCString(text, GetEffectiveWidth());
+
+// Highlight the cursor.
+window.MoveCursor(GetCursorWindowXPosition(), GetY());
+if (is_active)
+  window.AttributeOn(A_REVERSE);
+if (m_cursor_position == GetContentLength())
+  // Cursor is past the last character. Highlight an empty space.
+  window.PutChar(' ');
+else
+  window.PutChar(m_content[m_cursor_position]);
+if (is_active)
+  window.AttributeOff(A_REVERSE);
+  }
+
+  // The cursor is allowed to move one character past the string.
+  // m_cursor_position is in range [0, GetContentLength()].
+  void MoveCursorRight() {
+if (m_cursor_position < GetContentLength())
+  m_cursor_position++;
+  }
+
+  void MoveCursorLeft() {
+if (m_cursor_position > 0)
+  m_cursor_position--;
+  }
+
+  // If the cursor moved past

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1108
+  window.AttributeOn(A_REVERSE);
+window.PutChar(m_content ? 'X' : ' ');
+if (is_active)

clayborg wrote:
> We could use ACS_DIAMOND or ACS_BULLET? Check it out and see how it looks?
What do you think?

{F17450120}

{F17450119}

{F17450118}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 352761.
OmarEmaraDev added a comment.

- Remove PutCStringTruncatedWidth, use a character limit instead.
- Handle review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -392,6 +392,12 @@
   void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
 ::box(m_window, v_char, h_char);
   }
+  void VerticalLine(int n, chtype v_char = ACS_VLINE) {
+::wvline(m_window, v_char, n);
+  }
+  void HorizontalLine(int n, chtype h_char = ACS_HLINE) {
+::whline(m_window, h_char, n);
+  }
   void Clear() { ::wclear(m_window); }
   void Erase() { ::werase(m_window); }
   Rect GetBounds() const {
@@ -674,6 +680,36 @@
   AttributeOff(attr);
   }
 
+  void DrawBox(const Rect &bounds, chtype v_char = ACS_VLINE,
+   chtype h_char = ACS_HLINE) {
+MoveCursor(bounds.origin.x, bounds.origin.y);
+VerticalLine(bounds.size.height);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_ULCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1, bounds.origin.y);
+VerticalLine(bounds.size.height);
+PutChar(ACS_URCORNER);
+
+MoveCursor(bounds.origin.x, bounds.origin.y + bounds.size.height - 1);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_LLCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1,
+   bounds.origin.y + bounds.size.height - 1);
+PutChar(ACS_LRCORNER);
+  }
+
+  void DrawTitledBox(const Rect &bounds, const char *title,
+ chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
+DrawBox(bounds, v_char, h_char);
+int title_offset = 2;
+MoveCursor(bounds.origin.x + title_offset, bounds.origin.y);
+PutChar('[');
+PutCString(title, bounds.size.width - title_offset);
+PutChar(']');
+  }
+
   virtual void Draw(bool force) {
 if (m_delegate_sp && m_delegate_sp->WindowDelegateDraw(*this, force))
   return;
@@ -869,6 +905,545 @@
   const Window &operator=(const Window &) = delete;
 };
 
+/
+// Forms
+/
+
+class FieldDelegate {
+public:
+  virtual ~FieldDelegate() = default;
+
+  virtual Rect FieldDelegateGetBounds() = 0;
+
+  virtual void FieldDelegateDraw(Window &window, bool is_active) = 0;
+
+  virtual HandleCharResult FieldDelegateHandleChar(int key) {
+return eKeyNotHandled;
+  }
+};
+
+typedef std::shared_ptr FieldDelegateSP;
+
+class TextFieldDelegate : public FieldDelegate {
+public:
+  TextFieldDelegate(const char *label, int width, Point origin,
+const char *content)
+  : m_label(label), m_width(width), m_origin(origin), m_cursor_position(0),
+m_first_visibile_char(0) {
+if (content)
+  m_content = content;
+assert(m_width > 2);
+  }
+
+  // Get the bounding box of the field. The text field has a height of 3, 2
+  // lines for borders and 1 for the content.
+  Rect FieldDelegateGetBounds() override {
+return Rect(m_origin, Size(m_width, 3));
+  }
+
+  // Get the start X position of the content in window space, without the
+  // borders.
+  int GetX() { return m_origin.x + 1; }
+
+  // Get the start Y position of the content in window space, without the
+  // borders.
+  int GetY() { return m_origin.y + 1; }
+
+  // Get the effective width of the field, without the borders.
+  int GetEffectiveWidth() { return m_width - 2; }
+
+  // Get the cursor position in window space.
+  int GetCursorWindowXPosition() {
+return GetX() + m_cursor_position - m_first_visibile_char;
+  }
+
+  int GetContentLength() { return (int)m_content.length(); }
+
+  void FieldDelegateDraw(Window &window, bool is_active) override {
+// Draw label box.
+window.DrawTitledBox(FieldDelegateGetBounds(), m_label.c_str());
+
+// Draw content.
+window.MoveCursor(GetX(), GetY());
+const char *text = m_content.c_str() + m_first_visibile_char;
+window.PutCString(text, GetEffectiveWidth());
+
+// Highlight the cursor.
+window.MoveCursor(GetCursorWindowXPosition(), GetY());
+if (is_active)
+  window.AttributeOn(A_REVERSE);
+if (m_cursor_position == GetContentLength())
+  // Cursor is past the last character. Highlight an empty space.
+  window.PutChar(' ');
+else
+  window.PutChar(m_content[m_cursor_position]);
+if (is_active)
+  window.AttributeOff(A_REVERSE);
+  }
+
+  // The cursor is allowed to move one character past the string.
+  // m_cursor_position is in range [0, GetContentLength()].
+  void MoveCursorRight() {
+if (m_cursor_position < GetContentLength())
+  m_cursor_position++;
+  }
+
+  void MoveCursorLeft() {
+if (m_cursor_position > 0)
+  m_cursor_position--;

[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

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added a comment.
Herald added a subscriber: JDevlieghere.

An example form:

F17429353: 20210616-174753.mp4 

  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 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;
  };


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104395

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
Herald added a reviewer: teemperor.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch adds initial support for forms and dialogs for the LLDB GUI.
The current supported form elements are Text, Integer, Boolean, and
Choices.

A new window delegate FormWindowDelegate was added to represent the
dialog window that contains the form. The window takes a FormDelegate as
an input.

FormDelegate is an abstract class than have a default implementation for
the drawing and char handing methods, while it require the
FormDelegateSubmit method to be defined, which contains the main dialog
submitting routine and can set error message as needed. In constructor
of the FormDelegate, the necessary fields are added using the provided
factory methods for fields. The returned field delegate can be used to
retrieve the information that the fields contains.

A field is defined by a FieldDelegate and implements the char handing
and drawing as needed.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D104395

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp

Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -392,6 +392,12 @@
   void Box(chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
 ::box(m_window, v_char, h_char);
   }
+  void VerticalLine(int n, chtype v_char = ACS_VLINE) {
+::wvline(m_window, v_char, n);
+  }
+  void HorizontalLine(int n, chtype h_char = ACS_HLINE) {
+::whline(m_window, h_char, n);
+  }
   void Clear() { ::wclear(m_window); }
   void Erase() { ::werase(m_window); }
   Rect GetBounds() const {
@@ -423,14 +429,19 @@
 ::wbkgd(m_window, COLOR_PAIR(color_pair_idx));
   }
 
-  void PutCStringTruncated(int right_pad, const char *s, int len = -1) {
-int bytes_left = GetWidth() - GetCursorX();
+  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));
 }
   }
 
+  void PutCStringTruncated(int right_pad, const char *s, int len = -1) {
+PutCStringTruncatedWidth(right_pad, s, GetWidth(), len);
+  }
+
   void MoveWindow(const Point &origin) {
 const bool moving_window = origin != GetParentOrigin();
 if (m_is_subwin && moving_window) {
@@ -674,6 +685,35 @@
   AttributeOff(attr);
   }
 
+  void DrawBox(const Rect &bounds, chtype v_char = ACS_VLINE,
+   chtype h_char = ACS_HLINE) {
+MoveCursor(bounds.origin.x, bounds.origin.y);
+VerticalLine(bounds.size.height);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_ULCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1, bounds.origin.y);
+VerticalLine(bounds.size.height);
+PutChar(ACS_URCORNER);
+
+MoveCursor(bounds.origin.x, bounds.origin.y + bounds.size.height - 1);
+HorizontalLine(bounds.size.width);
+PutChar(ACS_LLCORNER);
+
+MoveCursor(bounds.origin.x + bounds.size.width - 1,
+   bounds.origin.y + bounds.size.height - 1);
+PutChar(ACS_LRCORNER);
+  }
+
+  void DrawTitledBox(const Rect &bounds, const char *title,
+ chtype v_char = ACS_VLINE, chtype h_char = ACS_HLINE) {
+DrawBox(bounds, v_char, h_char);
+MoveCursor(bounds.origin.x + 2, bounds.origin.y);
+PutChar('[');
+PutCString(title);
+PutChar(']');
+  }
+
   virtual void Draw(bool force) {
 if (m_delegate_sp && m_delegate_sp->WindowDelegateDraw(*this, force))
   return;
@@ -869,6 +909,540 @@
   const Window &operator=(const Window &) = delete;
 };
 
+/
+// Forms
+/
+
+class FieldDelegate {
+public:
+  virtual ~FieldDelegate() = default;
+
+  virtual Rect FieldDelegateGetBounds() = 0;
+
+  virtual void FieldDelegateDraw(Window &window, bool is_active) = 0;
+
+  virtual HandleCharResult FieldDelegateHandleChar(int key) {
+return eKeyNotHandled;
+  }
+};
+
+typedef std::shared_ptr FieldDelegateSP;
+
+class TextFieldDelegate : public FieldDelegate {
+public:
+  TextFieldDelegate(const char *label, int width, Point origin,
+const char *content)
+  : 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);
+  }
+
+  // Get the bounding box of the field. The text field has a height of 3, 2
+  // lines for borders and 1 for the content.
+  Rect FieldDelegateGetBounds() override {
+return Rect(m_origin, Size(m_width, 3));
+  }
+
+  // Get the start X position of the content in window space, without the
+  // borders.
+  int GetX() { return 

[Lldb-commits] [PATCH] D100243: [LLDB][GUI] Expand selected thread tree item by default

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 351547.
OmarEmaraDev added a comment.

- Handle review


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100243

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/test/API/commands/gui/expand-threads-tree/Makefile
  lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
  lldb/test/API/commands/gui/expand-threads-tree/main.c

Index: lldb/test/API/commands/gui/expand-threads-tree/main.c
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/main.c
@@ -0,0 +1,10 @@
+#include 
+
+void *thread_start_routine(void *arg) { return NULL; }
+
+int main() {
+  pthread_t thread;
+  pthread_create(&thread, NULL, thread_start_routine, NULL);
+  pthread_join(thread, NULL);
+  return 0;
+}
Index: lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
@@ -0,0 +1,55 @@
+"""
+Test the 'gui' default thread tree expansion.
+The root process tree item and the tree item corresponding to the selected
+thread should be expanded by default.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestGuiExpandThreadsTree(PExpectTest):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# PExpect uses many timeouts internally and doesn't play well
+# under ASAN on a loaded machine..
+@skipIfAsan
+@skipIfCursesSupportMissing
+def test_gui(self):
+self.build()
+
+self.launch(executable=self.getBuildArtifact("a.out"), dimensions=(100,500))
+self.expect("breakpoint set -r thread_start_routine", substrs=["Breakpoint 1", "address ="])
+self.expect("run", substrs=["stop reason ="])
+
+escape_key = chr(27).encode()
+
+# Start the GUI and close the welcome window.
+self.child.sendline("gui")
+self.child.send(escape_key)
+self.child.expect_exact("Threads")
+
+# The thread running thread_start_routine should be expanded.
+self.child.expect_exact("frame #0: thread_start_routine")
+
+# Exit GUI.
+self.child.send(escape_key)
+self.expect_prompt()
+
+# Select the main thread.
+self.child.sendline("thread select 1")
+
+# Start the GUI.
+self.child.sendline("gui")
+self.child.expect_exact("Threads")
+
+# The main thread should be expanded.
+self.child.expect("frame #\d+: main")
+
+# Quit the GUI
+self.child.send(escape_key)
+
+self.expect_prompt()
+self.quit()
Index: lldb/test/API/commands/gui/expand-threads-tree/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+ENABLE_THREADS := YES
+include Makefile.rules
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1608,8 +1608,13 @@
 
   virtual void TreeDelegateDrawTreeItem(TreeItem &item, Window &window) = 0;
   virtual void TreeDelegateGenerateChildren(TreeItem &item) = 0;
+  virtual void TreeDelegateUpdateSelection(TreeItem &root, int &selection_index,
+   TreeItem *&selected_item) {
+return;
+  }
   virtual bool TreeDelegateItemSelected(
   TreeItem &item) = 0; // Return true if we need to update views
+  virtual bool TreeDelegateExpandRootByDefault() { return false; }
 };
 
 typedef std::shared_ptr TreeDelegateSP;
@@ -1619,7 +1624,10 @@
   TreeItem(TreeItem *parent, TreeDelegate &delegate, bool might_have_children)
   : m_parent(parent), m_delegate(delegate), m_user_data(nullptr),
 m_identifier(0), m_row_idx(-1), m_children(),
-m_might_have_children(might_have_children), m_is_expanded(false) {}
+m_might_have_children(might_have_children), m_is_expanded(false) {
+if (m_parent == nullptr)
+  m_is_expanded = m_delegate.TreeDelegateExpandRootByDefault();
+  }
 
   TreeItem &operator=(const TreeItem &rhs) {
 if (this != &rhs) {
@@ -1848,6 +1856,8 @@
   const int num_visible_rows = NumVisibleRows();
   m_num_rows = 0;
   m_root.CalculateRowIndexes(m_num_rows);
+  m_delegate_sp->TreeDelegateUpdateSelection(m_root, m_selected_row_idx,
+ m_selected_item);
 
   // If we unexpanded while having something selected our total number of
   // rows is less than the num visible rows, then make sure we show all the
@@ -2149,7 +2159,7 @@
 public:
   Th

[Lldb-commits] [PATCH] D100243: [LLDB][GUI] Expand selected thread tree item by default

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev added inline comments.



Comment at: 
lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py:38
+# Exit GUI.
+self.child.send(escape_key)
+self.expect_prompt()

wallace wrote:
> create a method self.exit_gui to dedup some code, as you'll end up doing a 
> lot of them as you write your tests
> 
>   def exit_gui(self):
> self.child.send(chr(27).encode())
> self.expect_prompt()
> 
> you can also create a method start_gui
> 
>   def start_gui(self):
> self.child.sendline("gui")
> self.child.send(chr(27).encode())
> 
> you could also later create a base class lldbgui_testcase similar to 
> lldbvscode_testcase, where you can put all needed common code
Okay. I will do for all tests in a separate patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100243

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100243: [LLDB][GUI] Expand selected thread tree item by default

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 351263.
OmarEmaraDev added a comment.

- Merge branch 'main' into lldb-gui-expand-threads-tree
- Implement default selection


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100243

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/test/API/commands/gui/expand-threads-tree/Makefile
  lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
  lldb/test/API/commands/gui/expand-threads-tree/main.c

Index: lldb/test/API/commands/gui/expand-threads-tree/main.c
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/main.c
@@ -0,0 +1,10 @@
+#include 
+
+void *thread_start_routine(void *arg) { return NULL; }
+
+int main() {
+  pthread_t thread;
+  pthread_create(&thread, NULL, thread_start_routine, NULL);
+  pthread_join(thread, NULL);
+  return 0;
+}
Index: lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
@@ -0,0 +1,55 @@
+"""
+Test the 'gui' default thread tree expansion.
+The root process tree item and the tree item corresponding to the selected
+thread should be expanded by default.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestGuiExpandThreadsTree(PExpectTest):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# PExpect uses many timeouts internally and doesn't play well
+# under ASAN on a loaded machine..
+@skipIfAsan
+@skipIfCursesSupportMissing
+def test_gui(self):
+self.build()
+
+self.launch(executable=self.getBuildArtifact("a.out"), dimensions=(100,500))
+self.expect("breakpoint set -r thread_start_routine", substrs=["Breakpoint 1", "address ="])
+self.expect("run", substrs=["stop reason ="])
+
+escape_key = chr(27).encode()
+
+# Start the GUI and close the welcome window.
+self.child.sendline("gui")
+self.child.send(escape_key)
+self.child.expect_exact("Threads")
+
+# The thread running thread_start_routine should be expanded.
+self.child.expect_exact("frame #0: thread_start_routine")
+
+# Exit GUI.
+self.child.send(escape_key)
+self.expect_prompt()
+
+# Select the main thread.
+self.child.sendline("thread select 1")
+
+# Start the GUI.
+self.child.sendline("gui")
+self.child.expect_exact("Threads")
+
+# The main thread should be expanded.
+self.child.expect("frame #\d+: main")
+
+# Quit the GUI
+self.child.send(escape_key)
+
+self.expect_prompt()
+self.quit()
Index: lldb/test/API/commands/gui/expand-threads-tree/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+ENABLE_THREADS := YES
+include Makefile.rules
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1608,6 +1608,8 @@
 
   virtual void TreeDelegateDrawTreeItem(TreeItem &item, Window &window) = 0;
   virtual void TreeDelegateGenerateChildren(TreeItem &item) = 0;
+  virtual void SetDefaultSelectionIfNeeded(TreeItem &root, int *selection_index,
+   TreeItem *selected_item) = 0;
   virtual bool TreeDelegateItemSelected(
   TreeItem &item) = 0; // Return true if we need to update views
 };
@@ -1619,7 +1621,11 @@
   TreeItem(TreeItem *parent, TreeDelegate &delegate, bool might_have_children)
   : m_parent(parent), m_delegate(delegate), m_user_data(nullptr),
 m_identifier(0), m_row_idx(-1), m_children(),
-m_might_have_children(might_have_children), m_is_expanded(false) {}
+m_might_have_children(might_have_children), m_is_expanded(false) {
+// Expand root tree items by default.
+if (m_parent == nullptr)
+  m_is_expanded = true;
+  }
 
   TreeItem &operator=(const TreeItem &rhs) {
 if (this != &rhs) {
@@ -1848,6 +1854,8 @@
   const int num_visible_rows = NumVisibleRows();
   m_num_rows = 0;
   m_root.CalculateRowIndexes(m_num_rows);
+  m_delegate_sp->SetDefaultSelectionIfNeeded(m_root, &m_selected_row_idx,
+ m_selected_item);
 
   // If we unexpanded while having something selected our total number of
   // rows is less than the num visible rows, then make sure we show all the
@@ -2031,6 +2039,11 @@
 // No children for frames yet...
   }
 
+  void SetDefaultSele

[Lldb-commits] [PATCH] D100243: [LLDB][GUI] Expand selected thread tree item by default

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev marked 2 inline comments as done.
OmarEmaraDev added inline comments.



Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2211-2213
+  if (selected_thread->GetID() == thread->GetID()) {
+item[i].Expand();
+  }

clayborg wrote:
> clayborg wrote:
> > Remove parens for single statement if due to LLVM coding guidelines.
> It would be great if we can also select the currently selected stack frame 
> here. Something like:
> 
> ```
> if (selected_thread && selected_thread->GetID() == thread->GetID()) {
>   item[i].Expand();
>   StackFrameSP selected_frame = thread->GetSelectedFrame();
>   if (selected_frame) {
> // Add all frames and select the right one...
>   }
> }
> ```
@clayborg I didn't implement selection in this patch because my implementation 
didn't look great and I thought selection could be refactored to to make 
handling easier.

I haven't looked at this in more details yet, but I was thinking that maybe 
instead of having linear selection, we would have it hierarchical.
Each tree item declares which of its children is selected, so the process item 
will declare which thread item is selected and the selected thread item will 
declare which frame item is selected and so on.
It could be more conceptually correct because when selecting a frame, you are 
also selecting its parent thread. And selection can be checked by traversing 
parents and highlighting can be done by checking for leaf items. Something like 
that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100243

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100243: [LLDB][GUI] Expand selected thread tree item by default

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev updated this revision to Diff 338846.
OmarEmaraDev added a comment.

- Follow LLVM coding guidelines


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100243

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/test/API/commands/gui/expand-threads-tree/Makefile
  lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
  lldb/test/API/commands/gui/expand-threads-tree/main.c

Index: lldb/test/API/commands/gui/expand-threads-tree/main.c
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/main.c
@@ -0,0 +1,10 @@
+#include 
+
+void *thread_start_routine(void *arg) { return NULL; }
+
+int main() {
+  pthread_t thread;
+  pthread_create(&thread, NULL, thread_start_routine, NULL);
+  pthread_join(thread, NULL);
+  return 0;
+}
Index: lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
@@ -0,0 +1,55 @@
+"""
+Test the 'gui' default thread tree expansion.
+The root process tree item and the tree item corresponding to the selected
+thread should be expanded by default.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestGuiExpandThreadsTree(PExpectTest):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# PExpect uses many timeouts internally and doesn't play well
+# under ASAN on a loaded machine..
+@skipIfAsan
+@skipIfCursesSupportMissing
+def test_gui(self):
+self.build()
+
+self.launch(executable=self.getBuildArtifact("a.out"), dimensions=(100,500))
+self.expect("breakpoint set -r thread_start_routine", substrs=["Breakpoint 1", "address ="])
+self.expect("run", substrs=["stop reason ="])
+
+escape_key = chr(27).encode()
+
+# Start the GUI and close the welcome window.
+self.child.sendline("gui")
+self.child.send(escape_key)
+self.child.expect_exact("Threads")
+
+# The thread running thread_start_routine should be expanded.
+self.child.expect_exact("frame #0: thread_start_routine")
+
+# Exit GUI.
+self.child.send(escape_key)
+self.expect_prompt()
+
+# Select the main thread.
+self.child.sendline("thread select 1")
+
+# Start the GUI.
+self.child.sendline("gui")
+self.child.expect_exact("Threads")
+
+# The main thread should be expanded.
+self.child.expect("frame #\d+: main")
+
+# Quit the GUI
+self.child.send(escape_key)
+
+self.expect_prompt()
+self.quit()
Index: lldb/test/API/commands/gui/expand-threads-tree/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+ENABLE_THREADS := YES
+include Makefile.rules
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1619,7 +1619,11 @@
   TreeItem(TreeItem *parent, TreeDelegate &delegate, bool might_have_children)
   : m_parent(parent), m_delegate(delegate), m_user_data(nullptr),
 m_identifier(0), m_row_idx(-1), m_children(),
-m_might_have_children(might_have_children), m_is_expanded(false) {}
+m_might_have_children(might_have_children), m_is_expanded(false) {
+// Expand root tree items by default.
+if (m_parent == nullptr)
+  m_is_expanded = true;
+  }
 
   TreeItem &operator=(const TreeItem &rhs) {
 if (this != &rhs) {
@@ -2196,11 +2200,15 @@
 TreeItem t(&item, *m_thread_delegate_sp, false);
 ThreadList &threads = process_sp->GetThreadList();
 std::lock_guard guard(threads.GetMutex());
+ThreadSP selected_thread = threads.GetSelectedThread();
 size_t num_threads = threads.GetSize();
 item.Resize(num_threads, t);
 for (size_t i = 0; i < num_threads; ++i) {
-  item[i].SetIdentifier(threads.GetThreadAtIndex(i)->GetID());
+  ThreadSP thread = threads.GetThreadAtIndex(i);
+  item[i].SetIdentifier(thread->GetID());
   item[i].SetMightHaveChildren(true);
+  if (selected_thread->GetID() == thread->GetID())
+item[i].Expand();
 }
 return;
   }
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100243: [LLDB][GUI] Expand selected thread tree item by default

[Omar Emara via Phabricator via lldb-commits]

OmarEmaraDev created this revision.
OmarEmaraDev added a reviewer: clayborg.
OmarEmaraDev requested review of this revision.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

This patch expands the tree item that corresponds to the selected thread
by default in the Threads window. Additionally, the tree root item is
always expanded, which is the process in the Threads window.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100243

Files:
  lldb/source/Core/IOHandlerCursesGUI.cpp
  lldb/test/API/commands/gui/expand-threads-tree/Makefile
  lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
  lldb/test/API/commands/gui/expand-threads-tree/main.c

Index: lldb/test/API/commands/gui/expand-threads-tree/main.c
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/main.c
@@ -0,0 +1,10 @@
+#include 
+
+void *thread_start_routine(void *arg) { return NULL; }
+
+int main() {
+  pthread_t thread;
+  pthread_create(&thread, NULL, thread_start_routine, NULL);
+  pthread_join(thread, NULL);
+  return 0;
+}
Index: lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/TestGuiExpandThreadsTree.py
@@ -0,0 +1,55 @@
+"""
+Test the 'gui' default thread tree expansion.
+The root process tree item and the tree item corresponding to the selected
+thread should be expanded by default.
+"""
+
+import lldb
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+from lldbsuite.test.lldbpexpect import PExpectTest
+
+class TestGuiExpandThreadsTree(PExpectTest):
+
+mydir = TestBase.compute_mydir(__file__)
+
+# PExpect uses many timeouts internally and doesn't play well
+# under ASAN on a loaded machine..
+@skipIfAsan
+@skipIfCursesSupportMissing
+def test_gui(self):
+self.build()
+
+self.launch(executable=self.getBuildArtifact("a.out"), dimensions=(100,500))
+self.expect("breakpoint set -r thread_start_routine", substrs=["Breakpoint 1", "address ="])
+self.expect("run", substrs=["stop reason ="])
+
+escape_key = chr(27).encode()
+
+# Start the GUI and close the welcome window.
+self.child.sendline("gui")
+self.child.send(escape_key)
+self.child.expect_exact("Threads")
+
+# The thread running thread_start_routine should be expanded.
+self.child.expect_exact("frame #0: thread_start_routine")
+
+# Exit GUI.
+self.child.send(escape_key)
+self.expect_prompt()
+
+# Select the main thread.
+self.child.sendline("thread select 1")
+
+# Start the GUI.
+self.child.sendline("gui")
+self.child.expect_exact("Threads")
+
+# The main thread should be expanded.
+self.child.expect("frame #\d+: main")
+
+# Quit the GUI
+self.child.send(escape_key)
+
+self.expect_prompt()
+self.quit()
Index: lldb/test/API/commands/gui/expand-threads-tree/Makefile
===
--- /dev/null
+++ lldb/test/API/commands/gui/expand-threads-tree/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+ENABLE_THREADS := YES
+include Makefile.rules
Index: lldb/source/Core/IOHandlerCursesGUI.cpp
===
--- lldb/source/Core/IOHandlerCursesGUI.cpp
+++ lldb/source/Core/IOHandlerCursesGUI.cpp
@@ -1619,7 +1619,12 @@
   TreeItem(TreeItem *parent, TreeDelegate &delegate, bool might_have_children)
   : m_parent(parent), m_delegate(delegate), m_user_data(nullptr),
 m_identifier(0), m_row_idx(-1), m_children(),
-m_might_have_children(might_have_children), m_is_expanded(false) {}
+m_might_have_children(might_have_children), m_is_expanded(false) {
+// Expand root tree items by default.
+if (m_parent == nullptr) {
+  m_is_expanded = true;
+}
+  }
 
   TreeItem &operator=(const TreeItem &rhs) {
 if (this != &rhs) {
@@ -2196,11 +2201,16 @@
 TreeItem t(&item, *m_thread_delegate_sp, false);
 ThreadList &threads = process_sp->GetThreadList();
 std::lock_guard guard(threads.GetMutex());
+ThreadSP selected_thread = threads.GetSelectedThread();
 size_t num_threads = threads.GetSize();
 item.Resize(num_threads, t);
 for (size_t i = 0; i < num_threads; ++i) {
-  item[i].SetIdentifier(threads.GetThreadAtIndex(i)->GetID());
+  ThreadSP thread = threads.GetThreadAtIndex(i);
+  item[i].SetIdentifier(thread->GetID());
   item[i].SetMightHaveChildren(true);
+  if (selected_thread->GetID() == thread->GetID()) {
+item[i].Expand();
+  }
 }
 return;
   }
_