Qt: Always make the packet list row heights uniform.

In tests here using GTK+ 2.24 and 3.10, GtkTreeView handles multi-line
items by adjusting the height for all rows, but only after the number of
multi-line items exceeds some sort of threshold. For a packet capture
which contains a few DNS packets and a lot of TCP packets, if I change
"Standard query" to "Standard\nquery" in packet-dns.c I get
single-height packet list items. If I change "[TCP segment of a
reassembled PDU]" to "[TCP segment of a\nreassembled PDU]" in
packet-tcp.c (which results in more multi-line column strings) I get
double-height packet list items.

The current Qt code initially sets the uniformRowHeights property then
falls back to variable row heights if we run across a multi-line column
string. This adds a lot of logic which can impact other functionality
(e.g. column widths) and recalculating row heights is painfully slow for
large numbers of packets.

Instead of trying to manage variable row heights, always enable
uniformRowHeights. Track the maximum newline count and trigger a row
height adjustment when it changes. This mimics the GTK+ UI behavior,
although it should be more reliable.

Note that we need to adjust some numbers in RelatedPacketDelegate.

Change-Id: I289e963b6f00338c4374e602fa3fc83d04554519
Ping-Bug: 11515
Ping-Bug: 10924
Reviewed-on: https://code.wireshark.org/review/10628
Reviewed-by: Gerald Combs <gerald@wireshark.org>

diff --git a/ui/qt/packet_list.cpp b/ui/qt/packet_list.cpp
index 1407390bc662500c7cdbfbd313968ab512d93758..328c609f1bba3717da2ce6a85b4a3b24fc4847d6 100644 (file)
--- a/ui/qt/packet_list.cpp
+++ b/ui/qt/packet_list.cpp
@@ -256,6 +256,7 @@ PacketList::PacketList(QWidget *parent) :
     setItemsExpandable(false);
     setRootIsDecorated(false);
     setSortingEnabled(true);
+    setUniformRowHeights(true);
     setAccessibleName("Packet list");
     setItemDelegateForColumn(0, &related_packet_delegate_);
 
@@ -375,8 +376,8 @@ PacketList::PacketList(QWidget *parent) :
     g_assert(gbl_cur_packet_list == NULL);
     gbl_cur_packet_list = this;
 
-    connect(packet_list_model_, SIGNAL(rowHeightsVary()), this, SLOT(rowHeightsVary()));
     connect(packet_list_model_, SIGNAL(goToPacket(int)), this, SLOT(goToPacket(int)));
+    connect(packet_list_model_, SIGNAL(itemHeightChanged(const QModelIndex&)), this, SLOT(updateRowHeights(const QModelIndex&)));
     connect(wsApp, SIGNAL(addressResolutionChanged()), this, SLOT(redrawVisiblePackets()));
 
     header()->setContextMenuPolicy(Qt::CustomContextMenu);
@@ -574,9 +575,7 @@ int PacketList::sizeHintForColumn(int column) const
         // on OS X and Linux. We might want to add Q_OS_... #ifdefs accordingly.
         size_hint = itemDelegateForColumn(column)->sizeHint(viewOptions(), QModelIndex()).width();
     }
-    packet_list_model_->setSizeHintEnabled(false);
     size_hint += QTreeView::sizeHintForColumn(column); // Decoration padding
-    packet_list_model_->setSizeHintEnabled(true);
     return size_hint;
 }
 
@@ -775,7 +774,6 @@ void PacketList::clear() {
     create_near_overlay_ = true;
     create_far_overlay_ = true;
 
-    setUniformRowHeights(true);
     setColumnVisibility();
 }
 
@@ -963,12 +961,6 @@ void PacketList::setMonospaceFont(const QFont &mono_font)
 {
     setFont(mono_font);
     header()->setFont(wsApp->font());
-
-    // qtreeview.cpp does something similar in Qt 5 so this *should* be
-    // safe...
-    int row_height = itemDelegate()->sizeHint(viewOptions(), QModelIndex()).height();
-    packet_list_model_->setMonospaceFont(mono_font, row_height);
-    redrawVisiblePackets();
 }
 
 void PacketList::goNextPacket(void) {
@@ -1237,12 +1229,20 @@ void PacketList::sectionMoved(int, int, int)
     wsApp->emitAppSignal(WiresharkApplication::ColumnsChanged);
 }
 
-void PacketList::rowHeightsVary()
+void PacketList::updateRowHeights(const QModelIndex &ih_index)
 {
-    // This impairs performance considerably for large numbers of packets.
-    // We should probably move a bunch of the code in ::data to
-    // RelatedPacketDelegate and make it the delegate for everything.
-    setUniformRowHeights(false);
+    QStyleOptionViewItem option = viewOptions();
+    int max_height = 0;
+
+    // One of our columns increased the maximum row height. Find out which one.
+    for (int col = 0; col < packet_list_model_->columnCount(); col++) {
+        QSize size_hint = itemDelegate()->sizeHint(option, packet_list_model_->index(ih_index.row(), col));
+        max_height = qMax(max_height, size_hint.height());
+    }
+
+    if (max_height > 0) {
+        packet_list_model_->setMaximiumRowHeight(max_height);
+    }
 }
 
 void PacketList::copySummary()

diff --git a/ui/qt/packet_list.h b/ui/qt/packet_list.h
index 207601e4c9618732416f3012f1da67de08859ac9..3a3dffb8f03cd4fef707454bfb4f002b46d2178b 100644 (file)
--- a/ui/qt/packet_list.h
+++ b/ui/qt/packet_list.h
@@ -159,7 +159,7 @@ private slots:
     void columnVisibilityTriggered();
     void sectionResized(int col, int, int new_width);
     void sectionMoved(int, int, int);
-    void rowHeightsVary();
+    void updateRowHeights(const QModelIndex &ih_index);
     void copySummary();
     void vScrollBarActionTriggered(int);
     void drawFarOverlay();

diff --git a/ui/qt/packet_list_model.cpp b/ui/qt/packet_list_model.cpp
index 6757fd75e2b23391d0f03b2796ac6a6f3c5b4b0d..296709e5252f7833cbfa3361adbfb2c474c4fd72 100644 (file)
--- a/ui/qt/packet_list_model.cpp
+++ b/ui/qt/packet_list_model.cpp
@@ -47,14 +47,13 @@
 
 PacketListModel::PacketListModel(QObject *parent, capture_file *cf) :
     QAbstractItemModel(parent),
-    uniform_row_heights_(true),
-    row_height_(-1),
-    line_spacing_(0),
+    max_row_height_(0),
+    max_line_count_(1),
     idle_dissection_row_(0)
 {
     setCaptureFile(cf);
     PacketListRecord::clearStringPool();
-    connect(this, SIGNAL(itemHeightChanged(QModelIndex)),
+    connect(this, SIGNAL(maxLineCountChanged(QModelIndex)),
             this, SLOT(emitItemHeightChanged(QModelIndex)),
             Qt::QueuedConnection);
     idle_dissection_timer_ = new QElapsedTimer();
@@ -110,8 +109,8 @@ guint PacketListModel::recreateVisibleRows()
         }
     }
     endInsertRows();
-    return visible_rows_.count();
     idle_dissection_row_ = 0;
+    return visible_rows_.count();
 }
 
 void PacketListModel::clear() {
@@ -122,7 +121,8 @@ void PacketListModel::clear() {
     number_to_row_.clear();
     PacketListRecord::clearStringPool();
     endResetModel();
-    uniform_row_heights_ = true;
+    max_row_height_ = 0;
+    max_line_count_ = 1;
     idle_dissection_row_ = 0;
 }
 
@@ -244,14 +244,23 @@ void PacketListModel::unsetAllFrameRefTime()
     dataChanged(index(0, 0), index(rowCount() - 1, columnCount() - 1));
 }
 
-void PacketListModel::setMonospaceFont(const QFont &mono_font, int row_height)
+void PacketListModel::setMaximiumRowHeight(int height)
 {
-    QFontMetrics fm(mono_font_);
-    mono_font_ = mono_font;
-    row_height_ = row_height;
-    line_spacing_ = fm.lineSpacing();
+    max_row_height_ = height;
+    // As the QTreeView uniformRowHeights documentation says,
+    // "The height is obtained from the first item in the view. It is
+    //  updated when the data changes on that item."
+    dataChanged(index(0, 0), index(0, columnCount() - 1));
 }
 
+//void PacketListModel::setMonospaceFont(const QFont &mono_font, int row_height)
+//{
+//    QFontMetrics fm(mono_font_);
+//    mono_font_ = mono_font;
+//    row_height_ = row_height;
+//    line_spacing_ = fm.lineSpacing();
+//}
+
 // The Qt MVC documentation suggests using QSortFilterProxyModel for sorting
 // and filtering. That seems like overkill but it might be something we want
 // to do in the future.
@@ -402,11 +411,15 @@ bool PacketListModel::recordLessThan(PacketListRecord *r1, PacketListRecord *r2)
 // ::data is const so we have to make changes here.
 void PacketListModel::emitItemHeightChanged(const QModelIndex &ih_index)
 {
-    if (uniform_row_heights_) {
-        uniform_row_heights_ = false;
-        emit rowHeightsVary();
+    if (!ih_index.isValid()) return;
+
+    PacketListRecord *record = static_cast<PacketListRecord*>(ih_index.internalPointer());
+    if (!record) return;
+
+    if (record->lineCount() > max_line_count_) {
+        max_line_count_ = record->lineCount();
+        emit itemHeightChanged(ih_index);
     }
-    emit dataChanged(ih_index, ih_index);
 }
 
 int PacketListModel::rowCount(const QModelIndex &parent) const
@@ -435,8 +448,6 @@ QVariant PacketListModel::data(const QModelIndex &d_index, int role) const
         return QVariant();
 
     switch (role) {
-    case Qt::FontRole:
-        return mono_font_;
     case Qt::TextAlignmentRole:
         switch(recent_get_column_xalign(d_index.column())) {
         case COLUMN_XALIGN_RIGHT:
@@ -490,16 +501,20 @@ QVariant PacketListModel::data(const QModelIndex &d_index, int role) const
         // Assume each line count is 1. If the line count changes, emit
         // itemHeightChanged which triggers another redraw (including a
         // fetch of SizeHintRole and DisplayRole) in the next event loop.
-        if (column == 0 && record->lineCountChanged()) {
-            emit itemHeightChanged(d_index);
+        if (column == 0 && record->lineCountChanged() && record->lineCount() > max_line_count_) {
+            emit maxLineCountChanged(d_index);
         }
         return column_string;
     }
     case Qt::SizeHintRole:
     {
-        // We assume that inter-line spacing is 0.
-        QSize size = QSize(-1, row_height_ + ((record->lineCount() - 1) * line_spacing_));
-        return size;
+        // If this is the first row and column, return the maximum row height...
+        if (d_index.row() < 1 && d_index.column() < 1 && max_row_height_ > 0) {
+            QSize size = QSize(-1, max_row_height_);
+            return size;
+        }
+        // ...otherwise punt so that the item delegate can correctly calculate the item width.
+        return QVariant();
     }
     default:
         return QVariant();

diff --git a/ui/qt/packet_list_model.h b/ui/qt/packet_list_model.h
index 7d5af306636c2cbdeb0d4d8949e5207b7587e593..5ef6e6cc2fcdb77cc4843e89f4c0e3352c10d42a 100644 (file)
--- a/ui/qt/packet_list_model.h
+++ b/ui/qt/packet_list_model.h
@@ -72,12 +72,13 @@ public:
     void setDisplayedFrameIgnore(gboolean set);
     void toggleFrameRefTime(const QModelIndex &rt_index);
     void unsetAllFrameRefTime();
-    void setSizeHintEnabled(bool enable) { uniform_row_heights_ = enable; }
+
+    void setMaximiumRowHeight(int height);
 
 signals:
     void goToPacket(int);
-    void itemHeightChanged(const QModelIndex &ih_index) const;
-    void rowHeightsVary();
+    void maxLineCountChanged(const QModelIndex &ih_index) const;
+    void itemHeightChanged(const QModelIndex &ih_index);
     void pushBusyStatus(const QString &status);
     void popBusyStatus();
 
@@ -86,23 +87,20 @@ signals:
     void popProgressStatus();
 
 public slots:
-    void setMonospaceFont(const QFont &mono_font, int row_height);
     void sort(int column, Qt::SortOrder order = Qt::AscendingOrder);
     void flushVisibleRows();
     void dissectIdle(bool reset = false);
 
 private:
     capture_file *cap_file_;
-    QFont mono_font_;
     QList<QString> col_names_;
     QVector<PacketListRecord *> physical_rows_;
     QVector<PacketListRecord *> visible_rows_;
     QVector<PacketListRecord *> new_visible_rows_;
     QMap<int, int> number_to_row_;
 
-    bool uniform_row_heights_;
-    int row_height_;
-    int line_spacing_;
+    int max_row_height_; // px
+    int max_line_count_;
 
     static int sort_column_;
     static int text_sort_column_;

diff --git a/ui/qt/related_packet_delegate.cpp b/ui/qt/related_packet_delegate.cpp
index f9e154e2bd96512b739b70b82b19d81c653faa3a..75281c97812c025f1080bd25b92c676a6512701f 100644 (file)
--- a/ui/qt/related_packet_delegate.cpp
+++ b/ui/qt/related_packet_delegate.cpp
@@ -143,6 +143,7 @@ void RelatedPacketDelegate::paint(QPainter *painter, const QStyleOptionViewItem
 
     // Related packet indicator. Rightward arrow for requests, leftward
     // arrow for responses, circle for others.
+    // XXX These are comically oversized when we have multi-line rows.
     if (related_frames_.contains(fd->num)) {
         painter->setBrush(fg);
         switch (related_frames_[fd->num]) {