From a9a7dcec212c7932ee0886d2c0c848c0b7a8c614 Mon Sep 17 00:00:00 2001 From: John Thacker Date: Thu, 9 Feb 2023 07:21:55 -0500 Subject: [PATCH] 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 | 6 +- file.h | 4 + ui/qt/models/packet_list_model.cpp | 140 +++++++++++++++++++++++++++++ ui/qt/models/packet_list_model.h | 4 + ui/qt/models/packet_list_record.h | 3 + ui/qt/packet_list.cpp | 132 ++++++++------------------- 6 files changed, 191 insertions(+), 98 deletions(-) diff --git a/file.c b/file.c index 71cc5905f2..499472b85c 100644 --- a/file.c +++ b/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) diff --git a/file.h b/file.h index 53a4369d38..0b7d8c905e 100644 --- a/file.h +++ b/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); 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 #include +#include #include #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(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() << 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(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() << 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(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() << 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() << 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(); }