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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added a comment.
This revision now requires changes to proceed.

Many minor things, but looking good! I look forward to seeing this get checked in



================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1611-1612
   virtual void TreeDelegateGenerateChildren(TreeItem &item) = 0;
+  virtual void SetDefaultSelectionIfNeeded(TreeItem &root, int *selection_index,
+                                           TreeItem *selected_item) = 0;
   virtual bool TreeDelegateItemSelected(
----------------
Don't make this pure virtual unless all subclasses must implement this. I see a few implementations below that do nothing, so just make a default implementation so classes that don't need this don't have to implement it. Also renamed to TreeDelegateUpdateSelection to stay consistent with the other delegate methods.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1626-1627
+    // Expand root tree items by default.
+    if (m_parent == nullptr)
+      m_is_expanded = true;
+  }
----------------
We might want to opt into this? We don't want all variables in a variable view to be expanded by default. But we do want the process tree to always expand, so we need to be able to opt in in the tree delegates. 


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:1857-1858
       m_root.CalculateRowIndexes(m_num_rows);
+      m_delegate_sp->SetDefaultSelectionIfNeeded(m_root, &m_selected_row_idx,
+                                                 m_selected_item);
 
----------------
I assume we want this method to update both m_selected_row_idx and m_selected_item right? If so, make sure we switch to references on line 1611 above so both do get updated.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2042-2046
+  void SetDefaultSelectionIfNeeded(TreeItem &root, int *selection_index,
+                                   TreeItem *selected_item) override {
+    return;
+  }
+
----------------
Remove and make a default implementation in the base class and don't make it pure virtual


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2133-2137
+  void SetDefaultSelectionIfNeeded(TreeItem &root, int *selection_index,
+                                   TreeItem *selected_item) override {
+    return;
+  }
+
----------------
Remove and make a default implementation in the base class and don't make it pure virtual


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2197
   void TreeDelegateGenerateChildren(TreeItem &item) override {
     ProcessSP process_sp = GetProcess();
     if (process_sp && process_sp->IsAlive()) {
----------------
See comment below where you used to set m_need_to_set_default_selection to false.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2254
+      if (selected_thread->GetID() == thread->GetID()) {
+        selected_item = &root[i][thread->GetSelectedFrameIndex()];
+        *selection_index = selected_item->GetRowIndex();
----------------
This just sets the local variable "selected_item" to a value. Make sure we switch to using references on 1611 as mentioned before.


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2256
+        *selection_index = selected_item->GetRowIndex();
+        m_need_to_set_default_selection = false;
+        return;
----------------
You can remove this and just always set this in the TreeDelegateGenerateChildren


================
Comment at: lldb/source/Core/IOHandlerCursesGUI.cpp:2268
   uint32_t m_stop_id;
+  bool m_need_to_set_default_selection;
   FormatEntity::Entry m_format;
----------------
Maybe renamed to m_update_selection so this variable is a bit shorter?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100243