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

file.c

@@ -4289,9 +4289,11 @@ cf_set_modified_block(capture_file *cf, frame_data *fd, const wtap_block_t new_b
     if (pkt_block == new_block) {
         /* No need to save anything here, the caller changes went right
          * onto the block.
-         * Unfortunately we don't have a way to know how many comments were in the block
-         * before the caller modified it.
+         * Unfortunately we don't have a way to know how many comments were
+         * in the block before the caller modified it, so tell the caller
+         * it is its responsibility to update the comment count.
          */
+        return FALSE;
     }
     else {
         if (pkt_block)

file.h

@@ -702,6 +702,10 @@ wtap_block_t cf_get_packet_block(capture_file *cf, const frame_data *fd);
  * @param cf the capture file
  * @param fd the frame_data structure for the frame
  * @param new_block the block replacing the old block
+ *
+ * @return TRUE if the block is modified for the first time. FALSE if
+ * the block was already modified before, in which case the caller is
+ * responsible for updating the comment count.
  */
 gboolean cf_set_modified_block(capture_file *cf, frame_data *fd, const wtap_block_t new_block);
 

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;

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);
 

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.

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();
 }