PacketList column fixes.

Add a columnsChanged slot to PacketList and move the column update code
from redrawVisiblePackets there. Make sure we call
packet_list_model_->recreateVisibleRows, which should fix the behavior
described in bug 11324. Call columnsChanged when we, uh, change columns.

Add a sectionMoved slot to handle column reordering.

Don't rebuild the column list when we update the widgets in the column
preferences frame.  Do enable and disable the "remove" button as needed.

Try to keep the user from removing all of the columns in both the packet
list and column preferences.

Left as an exercise for the reader: The GTK+ UI also fails when you
remove all of the columns via the preferences:

  packet_list.c:377:packet_list_sort_column: assertion failed: (col)

Bug: 11324
Change-Id: Id58cf98e42cbda9aa2fc370ea06b8bcc6098c8ca
Reviewed-on: https://code.wireshark.org/review/9591
Petri-Dish: Gerald Combs <gerald@wireshark.org>
Tested-by: Petri Dish Buildbot <buildbot-no-reply@wireshark.org>
Reviewed-by: Gerald Combs <gerald@wireshark.org>

6 files changed, 92 insertions, 25 deletions

diff --git a/ui/qt/column_preferences_frame.cpp b/ui/qt/column_preferences_frame.cpp
index cb5727382d..a83111015f 100644
--- a/ui/qt/column_preferences_frame.cpp
+++ b/ui/qt/column_preferences_frame.cpp
@@ -70,6 +70,17 @@ ColumnPreferencesFrame::ColumnPreferencesFrame(QWidget *parent) :
     ui->columnTreeWidget->setDropIndicatorShown(true);
     ui->columnTreeWidget->setDragDropMode(QAbstractItemView::InternalMove);
 
+    for (GList *cur = g_list_first(prefs.col_list); cur != NULL && cur->data != NULL; cur = cur->next) {
+        fmt_data *cfmt = (fmt_data *) cur->data;
+        addColumn(cfmt->visible, cfmt->title, cfmt->fmt, cfmt->custom_field, cfmt->custom_occurrence);
+    }
+
+    connect(ui->columnTreeWidget, SIGNAL(itemSelectionChanged()), this, SLOT(updateWidgets()));
+
+    if (prefs.num_cols > 0) {
+        ui->columnTreeWidget->topLevelItem(0)->setSelected(true);
+    }
+
     updateWidgets();
 }
 
@@ -194,27 +205,22 @@ void ColumnPreferencesFrame::addColumn(bool visible, const char *title, int fmt,
         item->setText(custom_field_col_, custom_field);
         item->setText(custom_occurrence_col_, QString::number(custom_occurrence));
     }
+
+    updateWidgets();
 }
 
 void ColumnPreferencesFrame::updateWidgets()
 {
-    ui->columnTreeWidget->clear();
-
-    for (GList *cur = g_list_first(prefs.col_list); cur != NULL && cur->data != NULL; cur = cur->next) {
-        fmt_data *cfmt = (fmt_data *) cur->data;
-        addColumn(cfmt->visible, cfmt->title, cfmt->fmt, cfmt->custom_field, cfmt->custom_occurrence);
-    }
-
     ui->columnTreeWidget->resizeColumnToContents(visible_col_);
     ui->columnTreeWidget->resizeColumnToContents(title_col_);
     ui->columnTreeWidget->resizeColumnToContents(type_col_);
+
+    ui->deleteToolButton->setEnabled(ui->columnTreeWidget->selectedItems().count() > 0 && ui->columnTreeWidget->topLevelItemCount() > 1);
 }
 
 
-void ColumnPreferencesFrame::on_columnTreeWidget_currentItemChanged(QTreeWidgetItem *current, QTreeWidgetItem *previous)
+void ColumnPreferencesFrame::on_columnTreeWidget_currentItemChanged(QTreeWidgetItem *, QTreeWidgetItem *previous)
 {
-    ui->deleteToolButton->setEnabled(current ? true : false);
-
     if (previous && ui->columnTreeWidget->itemWidget(previous, title_col_)) {
         ui->columnTreeWidget->removeItemWidget(previous, title_col_);
     }
@@ -228,6 +234,8 @@ void ColumnPreferencesFrame::on_columnTreeWidget_currentItemChanged(QTreeWidgetI
     if (previous && ui->columnTreeWidget->itemWidget(previous, custom_occurrence_col_)) {
         ui->columnTreeWidget->removeItemWidget(previous, custom_occurrence_col_);
     }
+
+    updateWidgets();
 }
 
 void ColumnPreferencesFrame::on_columnTreeWidget_itemActivated(QTreeWidgetItem *item, int column)
@@ -380,10 +388,14 @@ void ColumnPreferencesFrame::on_newToolButton_clicked()
 
 void ColumnPreferencesFrame::on_deleteToolButton_clicked()
 {
+    if (ui->columnTreeWidget->topLevelItemCount() < 2) return;
+
     QTreeWidgetItem *item = ui->columnTreeWidget->currentItem();
     if (item) {
         ui->columnTreeWidget->invisibleRootItem()->removeChild(item);
     }
+
+    updateWidgets();
 }
 
 /*
diff --git a/ui/qt/column_preferences_frame.h b/ui/qt/column_preferences_frame.h
index adca04c0b3..c5c45e2895 100644
--- a/ui/qt/column_preferences_frame.h
+++ b/ui/qt/column_preferences_frame.h
@@ -55,10 +55,10 @@ private:
     int saved_combo_idx_;
 
     void addColumn(bool visible, const char *title, int fmt, const char *custom_field, int custom_occurrence);
-    void updateWidgets(void);
 
 private slots:
-    void on_columnTreeWidget_currentItemChanged(QTreeWidgetItem *current, QTreeWidgetItem *previous);
+    void updateWidgets(void);
+    void on_columnTreeWidget_currentItemChanged(QTreeWidgetItem *, QTreeWidgetItem *previous);
     void on_columnTreeWidget_itemActivated(QTreeWidgetItem *item, int column);
     void lineEditDestroyed();
     void comboDestroyed();
diff --git a/ui/qt/main_window.cpp b/ui/qt/main_window.cpp
index 085139cc35..bd60aef7d7 100644
--- a/ui/qt/main_window.cpp
+++ b/ui/qt/main_window.cpp
@@ -408,7 +408,7 @@ MainWindow::MainWindow(QWidget *parent) :
     connect(wsApp, SIGNAL(recentFilesRead()),
             packet_list_, SLOT(applyRecentColumnWidths()));
     connect(wsApp, SIGNAL(columnsChanged()),
-            packet_list_, SLOT(redrawVisiblePackets()));
+            packet_list_, SLOT(columnsChanged()));
     connect(wsApp, SIGNAL(recentFilesRead()),
             this, SLOT(applyRecentPaneGeometry()));
     connect(wsApp, SIGNAL(packetDissectionChanged()),
diff --git a/ui/qt/main_window_slots.cpp b/ui/qt/main_window_slots.cpp
index b80b0a012d..c4b73e77bf 100644
--- a/ui/qt/main_window_slots.cpp
+++ b/ui/qt/main_window_slots.cpp
@@ -2402,7 +2402,7 @@ void MainWindow::on_actionAnalyzeCreateAColumn_triggered()
         colnr = column_prefs_add_custom(COL_CUSTOM, capture_file_.capFile()->finfo_selected->hfinfo->name,
                     capture_file_.capFile()->finfo_selected->hfinfo->abbrev,0);
 
-        packet_list_->redrawVisiblePackets();
+        packet_list_->columnsChanged();
         packet_list_->resizeColumnToContents(colnr);
 
         prefs_main_write();
diff --git a/ui/qt/packet_list.cpp b/ui/qt/packet_list.cpp
index 2e48b21a39..76377bfbf7 100644
--- a/ui/qt/packet_list.cpp
+++ b/ui/qt/packet_list.cpp
@@ -65,7 +65,6 @@
 #include <QTreeWidget>
 
 // To do:
-// - Catch column reordering and rebuild the column list accoringly.
 // - Use a timer to trigger automatic scrolling.
 
 // If we ever add the ability to open multiple capture files we might be
@@ -416,6 +415,8 @@ PacketList::PacketList(QWidget *parent) :
             this, SLOT(showHeaderMenu(QPoint)));
     connect(header(), SIGNAL(sectionResized(int,int,int)),
             this, SLOT(sectionResized(int,int,int)));
+    connect(header(), SIGNAL(sectionMoved(int,int,int)),
+            this, SLOT(sectionMoved(int,int,int)));
 
     connect(verticalScrollBar(), SIGNAL(actionTriggered(int)), this, SLOT(vScrollBarActionTriggered(int)));
 
@@ -685,8 +686,7 @@ void PacketList::initHeaderContextMenu()
     }
 }
 
-// Redraw the packet list and detail. Called from many places, including
-// columnsChanged.
+// Redraw the packet list and detail. Called from many places.
 // XXX We previously re-selected the packet here, but that seems to cause
 // automatic scrolling problems.
 void PacketList::redrawVisiblePackets() {
@@ -696,13 +696,21 @@ void PacketList::redrawVisiblePackets() {
         proto_tree_->fillProtocolTree(cap_file_->edt->tree);
     }
 
+    update();
+    header()->update();
+}
+
+// prefs.col_list has changed.
+void PacketList::columnsChanged()
+{
+    if (!cap_file_) return;
+
     prefs.num_cols = g_list_length(prefs.col_list);
     col_cleanup(&cap_file_->cinfo);
     build_column_format_array(&cap_file_->cinfo, prefs.num_cols, FALSE);
+    packet_list_model_->recreateVisibleRows(); // Calls PacketListRecord::resetColumns
     setColumnVisibility();
-
-    update();
-    header()->update();
+    redrawVisiblePackets();
 }
 
 // Column widths should
@@ -1157,6 +1165,8 @@ void PacketList::showHeaderMenu(QPoint pos)
     header_actions_[caResolveNames]->setChecked(can_resolve && get_column_resolved(header_ctx_column_));
     header_actions_[caResolveNames]->setEnabled(can_resolve);
 
+    header_actions_[caRemoveColumn]->setEnabled(header_ctx_column_ >= 0 && header()->count() > 2);
+
     foreach (QAction *action, show_hide_actions_) {
         header_ctx_menu_.removeAction(action);
         delete action;
@@ -1214,13 +1224,16 @@ void PacketList::headerMenuTriggered()
         hideColumn(header_ctx_column_);
         break;
     case caRemoveColumn:
-        column_prefs_remove_nth(header_ctx_column_);
-        if (!prefs.gui_use_pref_save) {
-            prefs_main_write();
+    {
+        if (header()->count() > 2) {
+            column_prefs_remove_nth(header_ctx_column_);
+            columnsChanged();
+            if (!prefs.gui_use_pref_save) {
+                prefs_main_write();
+            }
         }
-        setColumnVisibility();
-        redraw = true;
         break;
+    }
     default:
         break;
     }
@@ -1250,6 +1263,46 @@ void PacketList::sectionResized(int col, int, int new_width)
     }
 }
 
+// The user moved a column. Make sure prefs.col_list, the column format
+// array, and the header's visual and logical indices all agree.
+// gtk/packet_list.c:column_dnd_changed_cb
+void PacketList::sectionMoved(int, int, int)
+{
+    GList *new_col_list = NULL;
+    QList<int> saved_sizes;
+
+    // Build a new column list based on the header's logical order.
+    for (int vis_idx = 0; vis_idx < header()->count(); vis_idx++) {
+        int log_idx = header()->logicalIndex(vis_idx);
+        saved_sizes << header()->sectionSize(log_idx);
+
+        void *pref_data = g_list_nth_data(prefs.col_list, log_idx);
+        if (!pref_data) continue;
+
+        new_col_list = g_list_append(new_col_list, pref_data);
+    }
+
+    // Clear and rebuild our (and the header's) model. There doesn't appear
+    // to be another way to reset the logical index.
+    freeze();
+
+    g_list_free(prefs.col_list);
+    prefs.col_list = new_col_list;
+
+    thaw();
+
+    for (int i = 0; i < saved_sizes.length(); i++) {
+        if (saved_sizes[i] < 1) continue;
+        header()->resizeSection(i, saved_sizes[i]);
+    }
+
+    if (!prefs.gui_use_pref_save) {
+        prefs_main_write();
+    }
+
+    wsApp->emitAppSignal(WiresharkApplication::ColumnsChanged);
+}
+
 // We need to tell when the user has scrolled the packet list, either to
 // the end or anywhere other than the end.
 void PacketList::vScrollBarActionTriggered(int)
diff --git a/ui/qt/packet_list.h b/ui/qt/packet_list.h
index de08a228be..715a4ebb04 100644
--- a/ui/qt/packet_list.h
+++ b/ui/qt/packet_list.h
@@ -135,6 +135,7 @@ public slots:
     void setTimeReference();
     void unsetAllTimeReferences();
     void redrawVisiblePackets();
+    void columnsChanged();
     void applyRecentColumnWidths();
 
 private slots:
@@ -142,6 +143,7 @@ private slots:
     void headerMenuTriggered();
     void columnVisibilityTriggered();
     void sectionResized(int col, int, int new_width);
+    void sectionMoved(int, int, int);
     void vScrollBarActionTriggered(int);
 };