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 _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits