Qt: fix erratic expansion of tree item when switching packets

ProtoTree::setRootNode() is designed to update the model with the new
packet tree, and additionally expand tree items in its view. When the
current selected packet is changed, it must use this method to ensure
that collapsed trees are properly expanded. Fix this regression.

It was not entirely clear that framesSelected can no longer use previous
state, so document it explicitly. Remove the call to QTreeView::reset(),
it ends up calling QAbstractItemView::reset() which touches the
selection model that refers invalidated proto_node memory. The reset
function of the view is automatically called the model is reset, so the
call was not needed anyway.

Test: open test/captures/tls13-rfc8446.pcap, expand TLS, TLS Record, and
select "Content Type". Change from frame 1 to 2, and then 3. Observe
that the expanded state remains constant with no flickering. In frame 3,
observe that the tree remains expanded even if no item is selected.

Change-Id: I0c820711f1a62aa51ac100f8ac5c89265c51eb18
Fixes: v3.3.0rc0-6-gcfee0f8082 ("Qt: Remove frameSelect signal")
Reviewed-on: https://code.wireshark.org/review/35230
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Roland Knall <rknall@gmail.com>

2 files changed, 15 insertions, 11 deletions

diff --git a/ui/qt/packet_list.cpp b/ui/qt/packet_list.cpp
index 61aea7de74..53b88ae23d 100644
--- a/ui/qt/packet_list.cpp
+++ b/ui/qt/packet_list.cpp
@@ -531,6 +531,8 @@ void PacketList::selectionChanged (const QItemSelection & selected, const QItemS
 
     related_packet_delegate_.clear();
 
+    // The previous dissection state has been invalidated by cf_select_packet
+    // above, receivers must clear the previous state and apply the updated one.
     emit framesSelected(QList<int>() << row);
 
     if (!cap_file_->edt) {
diff --git a/ui/qt/proto_tree.cpp b/ui/qt/proto_tree.cpp
index eb9858fefc..d515a1adfd 100644
--- a/ui/qt/proto_tree.cpp
+++ b/ui/qt/proto_tree.cpp
@@ -433,13 +433,15 @@ void ProtoTree::foreachTreeNode(proto_node *node, gpointer proto_tree_ptr)
     proto_tree_children_foreach(node, foreachTreeNode, proto_tree_ptr);
 }
 
-// We track item expansion using proto.c:tree_is_expanded. QTreeView
-// tracks it using QTreeViewPrivate::expandedIndexes. When we're handed
-// a new tree, clear expandedIndexes and repopulate it by walking the
-// tree and calling QTreeView::expand above.
+// setRootNode sets the new contents for the protocol tree and subsequently
+// restores the previously expanded state.
 void ProtoTree::setRootNode(proto_node *root_node) {
+    // XXX why do we have this call here?
     setFont(mono_font_);
-    reset(); // clears expandedIndexes.
+
+    // We track item expansion using proto.c:tree_is_expanded.
+    // Replace any existing (possibly invalidated) proto tree by the new tree.
+    // The expanded state will be reset as well and will be re-expanded below.
     proto_tree_model_->setRootNode(root_node);
 
     disconnect(this, SIGNAL(expanded(QModelIndex)), this, SLOT(syncExpanded(QModelIndex)));
@@ -608,12 +610,12 @@ void ProtoTree::itemDoubleClicked(const QModelIndex &index) {
 
 void ProtoTree::selectedFrameChanged(QList<int> frames)
 {
-    clear();
-
-    if (frames.count() != 1)
-        proto_tree_model_->setRootNode(Q_NULLPTR);
-    else if (cap_file_ && cap_file_->edt && cap_file_->edt->tree)
-        proto_tree_model_->setRootNode(cap_file_->edt->tree);
+    if (frames.count() == 1 && cap_file_ && cap_file_->edt && cap_file_->edt->tree) {
+        setRootNode(cap_file_->edt->tree);
+    } else {
+        // Clear the proto tree contents as they have become invalid.
+        proto_tree_model_->setRootNode(NULL);
+    }
 }
 
 // Select a field and bring it into view. Intended to be called by external