Qt: Ensure that add frame comments trigger recoloring, count updates

Add functions to PacketListRecord to invalidate a single record's
colorization and column strings, used for a record is modified in a
way that needs to trigger redrawing, but we don't need to redraw
all packets.

Move the functionality for adding, deleting, and setting frame comments
into PacketListModel, operating on QModelIndexes (or on all physical
rows in the case of deleting all comments from a file.) Trigger
recolorization of any record with an updated comment.

Only set a block as modified when deleting comments if we actually
deleted comments. This avoids marking a file as modified if we
delete all comments from all frames, or all comments from selected
frames, when those comments do not actually have frames.

If cf_set_modified_block is used to modify a block that is already
modified, it can't update the comment count. In that case, return
false and have the callers update the comment count. (It already
has a return value, which is always true.) This avoids having the
GUI warning about saving into a format that doesn't support comments
when comments have been added and then removed.

Note that, unlike with time references and time shifts, there
are no fields (and hence no columns nor color filters) that depend
on whether other fields have comments. If for some reason some
were added, then the model data for all frames would have to be
updated instead. Since there aren't, we don't need to
redrawVisiblePackets, but we do need to drawCurrentPacket to ensure
the packet details are redissected.

Fix #12519

4 files changed, 183 insertions, 96 deletions

diff --git a/ui/qt/models/packet_list_model.cpp b/ui/qt/models/packet_list_model.cpp
index aa865ee24c..483d9d3ccc 100644
--- a/ui/qt/models/packet_list_model.cpp
+++ b/ui/qt/models/packet_list_model.cpp
@@ -18,6 +18,7 @@
 
 #include <wsutil/nstime.h>
 #include <epan/column.h>
+#include <epan/expert.h>
 #include <epan/prefs.h>
 
 #include "ui/packet_list_utils.h"
@@ -333,6 +334,145 @@ void PacketListModel::unsetAllFrameRefTime()
     emit dataChanged(index(0, 0), index(rowCount() - 1, columnCount() - 1));
 }
 
+void PacketListModel::addFrameComment(const QModelIndexList &indices, const QByteArray &comment)
+{
+    int sectionMax = columnCount() - 1;
+    frame_data *fdata;
+    if (!cap_file_) return;
+
+    foreach (const auto &index, std::as_const(indices)) {
+        if (!index.isValid()) continue;
+
+        PacketListRecord *record = static_cast<PacketListRecord*>(index.internalPointer());
+        if (!record) continue;
+
+        fdata = record->frameData();
+        wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
+        wtap_block_add_string_option(pkt_block, OPT_COMMENT, comment.data(), comment.size());
+
+        if (!cf_set_modified_block(cap_file_, fdata, pkt_block)) {
+            cap_file_->packet_comment_count++;
+            expert_update_comment_count(cap_file_->packet_comment_count);
+        }
+
+        // In case there are coloring rules or columns related to comments.
+        // (#12519)
+        //
+        // XXX: "Does any active coloring rule relate to frame data"
+        // could be an optimization. For columns, note that
+        // "col_based_on_frame_data" only applies to built in columns,
+        // not custom columns based on frame data. (Should we prevent
+        // custom columns based on frame data from being created,
+        // substituting them with the other columns?)
+        //
+        // Note that there are not currently any fields that depend on
+        // whether other frames have comments, unlike with time references
+        // and time shifts ("frame.time_relative", "frame.offset_shift", etc.)
+        // If there were, then we'd need to reset data for all frames instead
+        // of just the frames changed.
+        record->invalidateColorized();
+        record->invalidateRecord();
+        emit dataChanged(index.sibling(index.row(), 0), index.sibling(index.row(), sectionMax),
+                QVector<int>() << Qt::BackgroundRole << Qt::ForegroundRole << Qt::DisplayRole);
+    }
+}
+
+void PacketListModel::setFrameComment(const QModelIndex &index, const QByteArray &comment, guint c_number)
+{
+    int sectionMax = columnCount() - 1;
+    frame_data *fdata;
+    if (!cap_file_) return;
+
+    if (!index.isValid()) return;
+
+    PacketListRecord *record = static_cast<PacketListRecord*>(index.internalPointer());
+    if (!record) return;
+
+    fdata = record->frameData();
+
+    wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
+    if (comment.isEmpty()) {
+        wtap_block_remove_nth_option_instance(pkt_block, OPT_COMMENT, c_number);
+        if (!cf_set_modified_block(cap_file_, fdata, pkt_block)) {
+            cap_file_->packet_comment_count--;
+            expert_update_comment_count(cap_file_->packet_comment_count);
+        }
+    } else {
+        wtap_block_set_nth_string_option_value(pkt_block, OPT_COMMENT, c_number, comment.data(), comment.size());
+        cf_set_modified_block(cap_file_, fdata, pkt_block);
+    }
+
+    record->invalidateColorized();
+    record->invalidateRecord();
+    emit dataChanged(index.sibling(index.row(), 0), index.sibling(index.row(), sectionMax),
+            QVector<int>() << Qt::BackgroundRole << Qt::ForegroundRole << Qt::DisplayRole);
+}
+
+void PacketListModel::deleteFrameComments(const QModelIndexList &indices)
+{
+    int sectionMax = columnCount() - 1;
+    frame_data *fdata;
+    if (!cap_file_) return;
+
+    foreach (const auto &index, std::as_const(indices)) {
+        if (!index.isValid()) continue;
+
+        PacketListRecord *record = static_cast<PacketListRecord*>(index.internalPointer());
+        if (!record) continue;
+
+        fdata = record->frameData();
+        wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
+        guint n_comments = wtap_block_count_option(pkt_block, OPT_COMMENT);
+
+        if (n_comments) {
+            for (guint i = 0; i < n_comments; i++) {
+                wtap_block_remove_nth_option_instance(pkt_block, OPT_COMMENT, 0);
+            }
+            if (!cf_set_modified_block(cap_file_, fdata, pkt_block)) {
+                cap_file_->packet_comment_count -= n_comments;
+                expert_update_comment_count(cap_file_->packet_comment_count);
+            }
+
+            record->invalidateColorized();
+            record->invalidateRecord();
+            emit dataChanged(index.sibling(index.row(), 0), index.sibling(index.row(), sectionMax),
+                    QVector<int>() << Qt::BackgroundRole << Qt::ForegroundRole << Qt::DisplayRole);
+        }
+    }
+}
+
+void PacketListModel::deleteAllFrameComments()
+{
+    int row;
+    int sectionMax = columnCount() - 1;
+    if (!cap_file_) return;
+
+    /* XXX: we might need a progressbar here */
+
+    foreach (PacketListRecord *record, physical_rows_) {
+        frame_data *fdata = record->frameData();
+        wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
+        guint n_comments = wtap_block_count_option(pkt_block, OPT_COMMENT);
+
+        if (n_comments) {
+            for (guint i = 0; i < n_comments; i++) {
+                wtap_block_remove_nth_option_instance(pkt_block, OPT_COMMENT, 0);
+            }
+            cf_set_modified_block(cap_file_, fdata, pkt_block);
+
+            record->invalidateColorized();
+            record->invalidateRecord();
+            row = packetNumberToRow(fdata->num);
+            if (row > -1) {
+                emit dataChanged(index(row, 0), index(row, sectionMax),
+                    QVector<int>() << Qt::BackgroundRole << Qt::ForegroundRole << Qt::DisplayRole);
+            }
+        }
+    }
+    cap_file_->packet_comment_count = 0;
+    expert_update_comment_count(cap_file_->packet_comment_count);
+}
+
 void PacketListModel::setMaximumRowHeight(int height)
 {
     max_row_height_ = height;
diff --git a/ui/qt/models/packet_list_model.h b/ui/qt/models/packet_list_model.h
index 6f44dd5752..11b42f5d72 100644
--- a/ui/qt/models/packet_list_model.h
+++ b/ui/qt/models/packet_list_model.h
@@ -74,6 +74,10 @@ public:
     void setDisplayedFrameIgnore(gboolean set);
     void toggleFrameRefTime(const QModelIndex &rt_index);
     void unsetAllFrameRefTime();
+    void addFrameComment(const QModelIndexList &indices, const QByteArray &comment);
+    void setFrameComment(const QModelIndex &index, const QByteArray &comment, guint c_number);
+    void deleteFrameComments(const QModelIndexList &indices);
+    void deleteAllFrameComments();
 
     void setMaximumRowHeight(int height);
 
diff --git a/ui/qt/models/packet_list_record.h b/ui/qt/models/packet_list_record.h
index 20399f3d87..47aa5621d1 100644
--- a/ui/qt/models/packet_list_record.h
+++ b/ui/qt/models/packet_list_record.h
@@ -44,6 +44,9 @@ public:
     unsigned int conversation() { return conv_index_; }
 
     int columnTextSize(const char *str);
+
+    void invalidateColorized() { colorized_ = false; }
+    void invalidateRecord() { col_text_cache_.remove(fdata_->num); }
     static void invalidateAllRecords() { col_text_cache_.clear(); }
     /* In Qt 6, QCache maxCost is a qsizetype, but the QAbstractItemModel
      * number of rows is still an int, so we're limited to INT_MAX anyway.
diff --git a/ui/qt/packet_list.cpp b/ui/qt/packet_list.cpp
index 87988f7a39..b56b29c180 100644
--- a/ui/qt/packet_list.cpp
+++ b/ui/qt/packet_list.cpp
@@ -1421,78 +1421,52 @@ QString PacketList::getPacketComment(guint c_number)
 
 void PacketList::addPacketComment(QString new_comment)
 {
-    frame_data *fdata;
-
     if (!cap_file_ || !packet_list_model_) return;
     if (new_comment.isEmpty()) return;
 
     QByteArray ba = new_comment.toUtf8();
 
-    for (int i = 0; i < selectedRows().size(); i++) {
-        int row = selectedRows().at(i);
-
-        fdata = packet_list_model_->getRowFdata(row);
-
-        if (!fdata) continue;
-
-        wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
-
-        /*
-         * Make sure this would fit in a pcapng option.
-         *
-         * XXX - 65535 is the maximum size for an option in pcapng;
-         * what if another capture file format supports larger
-         * comments?
-         */
-        if (ba.size() > 65535) {
-            simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK,
-                          "That comment is too large to save in a capture file.");
-            return;
-        }
-        wtap_block_add_string_option(pkt_block, OPT_COMMENT, ba.data(), ba.size());
-
-        cf_set_modified_block(cap_file_, fdata, pkt_block);
+    /*
+     * Make sure this would fit in a pcapng option.
+     *
+     * XXX - 65535 is the maximum size for an option in pcapng;
+     * what if another capture file format supports larger
+     * comments?
+     */
+    if (ba.size() > 65535) {
+        simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK,
+                      "That comment is too large to save in a capture file.");
+        return;
     }
 
-    redrawVisiblePackets();
+    if (selectionModel() && selectionModel()->hasSelection()) {
+        packet_list_model_->addFrameComment(selectionModel()->selectedRows(), ba);
+        drawCurrentPacket();
+    }
 }
 
 void PacketList::setPacketComment(guint c_number, QString new_comment)
 {
-    int row = currentIndex().row();
-    frame_data *fdata;
+    QModelIndex curIndex = currentIndex();
 
     if (!cap_file_ || !packet_list_model_) return;
 
-    fdata = packet_list_model_->getRowFdata(row);
-
-    if (!fdata) return;
-
-    wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
-
-    /* Check if we are clearing the comment */
-    if (new_comment.isEmpty()) {
-        wtap_block_remove_nth_option_instance(pkt_block, OPT_COMMENT, c_number);
-    } else {
-        QByteArray ba = new_comment.toUtf8();
-        /*
-         * Make sure this would fit in a pcapng option.
-         *
-         * XXX - 65535 is the maximum size for an option in pcapng;
-         * what if another capture file format supports larger
-         * comments?
-         */
-        if (ba.size() > 65535) {
-            simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK,
-                          "That comment is too large to save in a capture file.");
-            return;
-        }
-        wtap_block_set_nth_string_option_value(pkt_block, OPT_COMMENT, c_number, ba.data(), ba.size());
+    QByteArray ba = new_comment.toUtf8();
+    /*
+     * Make sure this would fit in a pcapng option.
+     *
+     * XXX - 65535 is the maximum size for an option in pcapng;
+     * what if another capture file format supports larger
+     * comments?
+     */
+    if (ba.size() > 65535) {
+        simple_dialog(ESD_TYPE_ERROR, ESD_BTN_OK,
+                      "That comment is too large to save in a capture file.");
+        return;
     }
 
-    cf_set_modified_block(cap_file_, fdata, pkt_block);
-
-    redrawVisiblePackets();
+    packet_list_model_->setFrameComment(curIndex, ba, c_number);
+    drawCurrentPacket();
 }
 
 QString PacketList::allPacketComments()
@@ -1528,54 +1502,20 @@ QString PacketList::allPacketComments()
 
 void PacketList::deleteCommentsFromPackets()
 {
-    frame_data *fdata;
-
     if (!cap_file_ || !packet_list_model_) return;
 
-    for (int i = 0; i < selectedRows().size(); i++) {
-        int row = selectedRows().at(i);
-
-        fdata = packet_list_model_->getRowFdata(row);
-
-        if (!fdata) continue;
-
-        wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
-        guint n_comments = wtap_block_count_option(pkt_block, OPT_COMMENT);
-
-        for (guint j = 0; j < n_comments; j++) {
-            wtap_block_remove_nth_option_instance(pkt_block, OPT_COMMENT, 0);
-        }
-
-        cf_set_modified_block(cap_file_, fdata, pkt_block);
+    if (selectionModel() && selectionModel()->hasSelection()) {
+        packet_list_model_->deleteFrameComments(selectionModel()->selectedRows());
+        drawCurrentPacket();
     }
-
-    redrawVisiblePackets();
 }
 
 void PacketList::deleteAllPacketComments()
 {
-    guint32 framenum;
-    frame_data *fdata;
-    QString buf_str;
-    guint i;
-
-    if (!cap_file_)
-        return;
-
-    for (framenum = 1; framenum <= cap_file_->count ; framenum++) {
-        fdata = frame_data_sequence_find(cap_file_->provider.frames, framenum);
-        wtap_block_t pkt_block = cf_get_packet_block(cap_file_, fdata);
-        guint n_comments = wtap_block_count_option(pkt_block, OPT_COMMENT);
-
-        for (i = 0; i < n_comments; i++) {
-            wtap_block_remove_nth_option_instance(pkt_block, OPT_COMMENT, 0);
-        }
-        cf_set_modified_block(cap_file_, fdata, pkt_block);
-    }
+    if (!cap_file_ || !packet_list_model_) return;
 
-    cap_file_->packet_comment_count = 0;
-    expert_update_comment_count(cap_file_->packet_comment_count);
-    redrawVisiblePackets();
+    packet_list_model_->deleteAllFrameComments();
+    drawCurrentPacket();
 }