From 434644b381cb1dc8d2080b19a9725bfe2660a217 Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Fri, 17 Apr 2020 23:18:58 +0200 Subject: [PATCH] undo: make picture (media) deletion undoable The code is rather complex. Firstly, we have different representations of pictures throughout the code. Secondly, this tries to do add the pictures in batches to the divepicture model and that is always rather tricky. Signed-off-by: Berthold Stoeger --- commands/command.cpp | 5 + commands/command.h | 10 ++ commands/command_pictures.cpp | 106 ++++++++++++++ commands/command_pictures.h | 13 ++ core/picture.c | 12 +- core/picture.h | 2 +- core/subsurface-qt/divelistnotifier.h | 3 + desktop-widgets/tab-widgets/TabDivePhotos.cpp | 3 +- profile-widget/divepixmapitem.cpp | 7 +- profile-widget/profilewidget2.cpp | 31 +++- profile-widget/profilewidget2.h | 4 +- qt-models/divepicturemodel.cpp | 134 ++++++++++++++---- qt-models/divepicturemodel.h | 10 +- 13 files changed, 286 insertions(+), 54 deletions(-) diff --git a/commands/command.cpp b/commands/command.cpp index 8195fbd22..d67009978 100644 --- a/commands/command.cpp +++ b/commands/command.cpp @@ -367,4 +367,9 @@ void setPictureOffset(dive *d, const QString &filename, offset_t offset) execute(new SetPictureOffset(d, filename, offset)); } +void removePictures(const std::vector &pictures) +{ + execute(new RemovePictures(pictures)); +} + } // namespace Command diff --git a/commands/command.h b/commands/command.h index 5d92eec03..e4c3b05d2 100644 --- a/commands/command.h +++ b/commands/command.h @@ -3,6 +3,7 @@ #define COMMAND_H #include "core/dive.h" +#include "core/pictureobj.h" #include #include #include @@ -120,7 +121,16 @@ void addGasSwitch(struct dive *d, int dcNr, int seconds, int tank); // 7) Picture (media) commands +struct PictureListForDeletion { + dive *d; + std::vector filenames; +}; +struct PictureListForAddition { + dive *d; + std::vector pics; +}; void setPictureOffset(dive *d, const QString &filename, offset_t offset); +void removePictures(const std::vector &pictures); } // namespace Command diff --git a/commands/command_pictures.cpp b/commands/command_pictures.cpp index b7f03b193..2e40c0362 100644 --- a/commands/command_pictures.cpp +++ b/commands/command_pictures.cpp @@ -47,4 +47,110 @@ bool SetPictureOffset::workToBeDone() return !!d; } +// Filter out pictures that don't exist and sort them according to the backend +static PictureListForDeletion filterPictureListForDeletion(const PictureListForDeletion &p) +{ + PictureListForDeletion res; + res.d = p.d; + res.filenames.reserve(p.filenames.size()); + for (int i = 0; i < p.d->pictures.nr; ++i) { + std::string fn = p.d->pictures.pictures[i].filename; + if (std::find(p.filenames.begin(), p.filenames.end(), fn) != p.filenames.end()) + res.filenames.push_back(fn); + } + return res; +} + +// Helper function to remove pictures. Clears the passed-in vector. +static std::vector removePictures(std::vector &picturesToRemove) +{ + std::vector res; + for (const PictureListForDeletion &list: picturesToRemove) { + QVector filenames; + PictureListForAddition toAdd; + toAdd.d = list.d; + for (const std::string &fn: list.filenames) { + int idx = get_picture_idx(&list.d->pictures, fn.c_str()); + if (idx < 0) { + fprintf(stderr, "removePictures(): picture disappeared!"); + continue; // Huh? We made sure that this can't happen by filtering out non-existant pictures. + } + filenames.push_back(QString::fromStdString(fn)); + toAdd.pics.emplace_back(list.d->pictures.pictures[idx]); + remove_from_picture_table(&list.d->pictures, idx); + } + if (!toAdd.pics.empty()) + res.push_back(toAdd); + invalidate_dive_cache(list.d); + emit diveListNotifier.picturesRemoved(list.d, filenames); + } + picturesToRemove.clear(); + return res; +} + +// Helper function to add pictures. Clears the passed-in vector. +static std::vector addPictures(std::vector &picturesToAdd) +{ + // We play it extra safe here and again filter out those pictures that + // are already added to the dive. There should be no way for this to + // happen, as we checked that before. + std::vector res; + for (const PictureListForAddition &list: picturesToAdd) { + QVector picsForSignal; + PictureListForDeletion toRemove; + toRemove.d = list.d; + for (const PictureObj &pic: list.pics) { + int idx = get_picture_idx(&list.d->pictures, pic.filename.c_str()); // This should *not* already exist! + if (idx >= 0) { + fprintf(stderr, "addPictures(): picture disappeared!"); + continue; // Huh? We made sure that this can't happen by filtering out existing pictures. + } + picsForSignal.push_back(pic); + add_picture(&list.d->pictures, pic.toCore()); + toRemove.filenames.push_back(pic.filename); + } + if (!toRemove.filenames.empty()) + res.push_back(toRemove); + invalidate_dive_cache(list.d); + emit diveListNotifier.picturesAdded(list.d, picsForSignal); + } + picturesToAdd.clear(); + return res; +} + +RemovePictures::RemovePictures(const std::vector &pictures) : picturesToRemove(pictures) +{ + // Filter out the pictures that don't actually exist. In principle this shouldn't be necessary. + // Nevertheless, let's play it save. This has the additional benefit of sorting the pictures + // according to the backend if, for some reason, the caller passed the pictures in in random order. + picturesToRemove.reserve(pictures.size()); + size_t count = 0; + for (const PictureListForDeletion &p: pictures) { + PictureListForDeletion filtered = filterPictureListForDeletion(p); + if (filtered.filenames.size()) + picturesToRemove.push_back(p); + count += filtered.filenames.size(); + } + if (count == 0) { + picturesToRemove.clear(); // This signals that nothing is to be done + return; + } + setText(Command::Base::tr("remove %n pictures(s)", "", count)); +} + +bool RemovePictures::workToBeDone() +{ + return !picturesToRemove.empty(); +} + +void RemovePictures::undo() +{ + picturesToRemove = addPictures(picturesToAdd); +} + +void RemovePictures::redo() +{ + picturesToAdd = removePictures(picturesToRemove); +} + } // namespace Command diff --git a/commands/command_pictures.h b/commands/command_pictures.h index f2381d111..802c63c6c 100644 --- a/commands/command_pictures.h +++ b/commands/command_pictures.h @@ -5,6 +5,7 @@ #define COMMAND_PICTURES_H #include "command_base.h" +#include "command.h" // for PictureListForDeletion/Addition // We put everything in a namespace, so that we can shorten names without polluting the global namespace namespace Command { @@ -22,5 +23,17 @@ private: bool workToBeDone() override; }; +class RemovePictures final : public Base { +public: + RemovePictures(const std::vector &pictures); +private: + std::vector picturesToRemove; // for redo + std::vector picturesToAdd; // for undo + + void undo() override; + void redo() override; + bool workToBeDone() override; +}; + } // namespace Command #endif diff --git a/core/picture.c b/core/picture.c index d7ba597f6..36953b8b1 100644 --- a/core/picture.c +++ b/core/picture.c @@ -30,7 +30,7 @@ static bool picture_less_than(struct picture a, struct picture b) static MAKE_GROW_TABLE(picture_table, struct picture, pictures) static MAKE_GET_INSERTION_INDEX(picture_table, struct picture, pictures, picture_less_than) MAKE_ADD_TO(picture_table, struct picture, pictures) -static MAKE_REMOVE_FROM(picture_table, pictures) +MAKE_REMOVE_FROM(picture_table, pictures) MAKE_SORT(picture_table, struct picture, pictures, comp_pictures) //MAKE_REMOVE(picture_table, struct picture, picture) MAKE_CLEAR_TABLE(picture_table, pictures, picture) @@ -66,13 +66,3 @@ int get_picture_idx(const struct picture_table *t, const char *filename) } return -1; } - -// Return true if picture was found and deleted -bool remove_picture(struct picture_table *t, const char *filename) -{ - int idx = get_picture_idx(t, filename); - if (idx < 0) - return false; - remove_from_picture_table(t, idx); - return true; -} diff --git a/core/picture.h b/core/picture.h index 999ef9681..61492fffc 100644 --- a/core/picture.h +++ b/core/picture.h @@ -33,7 +33,7 @@ extern void clear_picture_table(struct picture_table *); extern void add_to_picture_table(struct picture_table *, int idx, struct picture pic); extern void copy_pictures(const struct picture_table *s, struct picture_table *d); extern void add_picture(struct picture_table *, struct picture newpic); -extern bool remove_picture(struct picture_table *, const char *filename); +extern void remove_from_picture_table(struct picture_table *, int idx); extern int get_picture_idx(const struct picture_table *, const char *filename); /* Return -1 if not found */ extern void sort_picture_table(struct picture_table *); diff --git a/core/subsurface-qt/divelistnotifier.h b/core/subsurface-qt/divelistnotifier.h index 0e85d6f26..2931e443e 100644 --- a/core/subsurface-qt/divelistnotifier.h +++ b/core/subsurface-qt/divelistnotifier.h @@ -6,6 +6,7 @@ #define DIVELISTNOTIFIER_H #include "core/dive.h" +#include "core/pictureobj.h" #include @@ -118,6 +119,8 @@ signals: // Picture (media) related signals void pictureOffsetChanged(dive *d, QString filename, offset_t offset); + void picturesRemoved(dive *d, QVector filenames); + void picturesAdded(dive *d, QVector pics); // This signal is emited every time a command is executed. // This is used to hide an old multi-dives-edited warning message. diff --git a/desktop-widgets/tab-widgets/TabDivePhotos.cpp b/desktop-widgets/tab-widgets/TabDivePhotos.cpp index 731e58388..db1b943e4 100644 --- a/desktop-widgets/tab-widgets/TabDivePhotos.cpp +++ b/desktop-widgets/tab-widgets/TabDivePhotos.cpp @@ -84,7 +84,8 @@ QVector TabDivePhotos::getSelectedFilenames() const void TabDivePhotos::removeSelectedPhotos() { - DivePictureModel::instance()->removePictures(getSelectedFilenames()); + QModelIndexList indices = ui->photosView->selectionModel()->selectedRows(); + DivePictureModel::instance()->removePictures(indices); } void TabDivePhotos::openFolderOfSelectedFiles() diff --git a/profile-widget/divepixmapitem.cpp b/profile-widget/divepixmapitem.cpp index 611f3f76a..7b2c9cc87 100644 --- a/profile-widget/divepixmapitem.cpp +++ b/profile-widget/divepixmapitem.cpp @@ -1,12 +1,13 @@ // SPDX-License-Identifier: GPL-2.0 #include "profile-widget/divepixmapitem.h" #include "profile-widget/animationfunctions.h" -#include "qt-models/divepicturemodel.h" #include "core/pref.h" #include "core/qthelper.h" #include "core/settings/qPrefDisplay.h" #ifndef SUBSURFACE_MOBILE #include "desktop-widgets/preferences/preferencesdialog.h" +#include "core/dive.h" // for displayed_dive +#include "commands/command.h" #endif #include @@ -123,6 +124,8 @@ void DivePictureItem::mousePressEvent(QGraphicsSceneMouseEvent *event) void DivePictureItem::removePicture() { #ifndef SUBSURFACE_MOBILE - DivePictureModel::instance()->removePictures({ fileUrl }); + struct dive *d = get_dive_by_uniq_id(displayed_dive.id); + if (d) + Command::removePictures({ { d, { fileUrl.toStdString() } } }); #endif } diff --git a/profile-widget/profilewidget2.cpp b/profile-widget/profilewidget2.cpp index cea5a50f0..e1a0415ea 100644 --- a/profile-widget/profilewidget2.cpp +++ b/profile-widget/profilewidget2.cpp @@ -169,9 +169,8 @@ ProfileWidget2::ProfileWidget2(QWidget *parent) : QGraphicsView(parent), addActionShortcut(Qt::Key_Right, &ProfileWidget2::keyRightAction); connect(Thumbnailer::instance(), &Thumbnailer::thumbnailChanged, this, &ProfileWidget2::updateThumbnail, Qt::QueuedConnection); - connect(DivePictureModel::instance(), &DivePictureModel::rowsInserted, this, &ProfileWidget2::plotPictures); - connect(DivePictureModel::instance(), &DivePictureModel::picturesRemoved, this, &ProfileWidget2::removePictures); - connect(DivePictureModel::instance(), &DivePictureModel::modelReset, this, &ProfileWidget2::plotPictures); + connect(&diveListNotifier, &DiveListNotifier::picturesRemoved, this, &ProfileWidget2::picturesRemoved); + connect(&diveListNotifier, &DiveListNotifier::picturesAdded, this, &ProfileWidget2::picturesAdded); connect(&diveListNotifier, &DiveListNotifier::cylinderEdited, this, &ProfileWidget2::profileChanged); connect(&diveListNotifier, &DiveListNotifier::eventsChanged, this, &ProfileWidget2::profileChanged); connect(&diveListNotifier, &DiveListNotifier::pictureOffsetChanged, this, &ProfileWidget2::pictureOffsetChanged); @@ -2147,11 +2146,10 @@ void ProfileWidget2::plotPicturesInternal(const struct dive *d, bool synchronous } // Remove the pictures with the given filenames from the profile plot. -// TODO: This does not check for the fact that the same image may be attributed -// to different dives! Deleting the picture from one dive may therefore remove -// it from the profile of a different dive. -void ProfileWidget2::removePictures(const QVector &fileUrls) +void ProfileWidget2::picturesRemoved(dive *d, QVector fileUrls) { + if (d->id != displayed_dive.id) + return; // To remove the pictures, we use the std::remove_if() algorithm. // std::remove_if() does not actually delete the elements, but moves // them to the end of the given range. It returns an iterator to the @@ -2165,6 +2163,25 @@ void ProfileWidget2::removePictures(const QVector &fileUrls) calculatePictureYPositions(); } +void ProfileWidget2::picturesAdded(dive *d, QVector pics) +{ + if (d->id != displayed_dive.id) + return; + + for (const PictureObj &pic: pics) { + if (pic.offset.seconds > 0 && pic.offset.seconds <= d->duration.seconds) { + pictures.emplace_back(pic.offset, QString::fromStdString(pic.filename), scene(), false); + updateThumbnailXPos(pictures.back()); + } + } + + // Sort pictures by timestamp (and filename if equal timestamps). + // This will allow for proper location of the pictures on the profile plot. + std::sort(pictures.begin(), pictures.end()); + + calculatePictureYPositions(); +} + void ProfileWidget2::profileChanged(dive *d) { if (!d || d->id != displayed_dive.id) diff --git a/profile-widget/profilewidget2.h b/profile-widget/profilewidget2.h index 039424d87..e6f06a7b2 100644 --- a/profile-widget/profilewidget2.h +++ b/profile-widget/profilewidget2.h @@ -20,6 +20,7 @@ #include "profile-widget/diveprofileitem.h" #include "core/display.h" #include "core/color.h" +#include "core/pictureobj.h" #include "core/units.h" class RulerItem2; @@ -110,7 +111,8 @@ slots: // Necessary to call from QAction's signals. void setProfileState(); #ifndef SUBSURFACE_MOBILE void plotPictures(); - void removePictures(const QVector &fileUrls); + void picturesRemoved(dive *d, QVector filenames); + void picturesAdded(dive *d, QVector pics); void setPlanState(); void setAddState(); void pointInserted(const QModelIndex &parent, int start, int end); diff --git a/qt-models/divepicturemodel.cpp b/qt-models/divepicturemodel.cpp index 351396e5f..7c9fb18c2 100644 --- a/qt-models/divepicturemodel.cpp +++ b/qt-models/divepicturemodel.cpp @@ -1,16 +1,42 @@ // SPDX-License-Identifier: GPL-2.0 #include "qt-models/divepicturemodel.h" +#include "core/divelist.h" // for comp_dives #include "core/metrics.h" -#include "core/divelist.h" // for mark_divelist_changed() -#include "core/dive.h" #include "core/imagedownloader.h" #include "core/picture.h" #include "core/qthelper.h" #include "core/subsurface-qt/divelistnotifier.h" +#include "commands/command.h" #include #include +PictureEntry::PictureEntry(dive *dIn, const PictureObj &p) : d(dIn), + filename(p.filename), + offsetSeconds(p.offset.seconds), + length({ 0 }) +{ +} + +PictureEntry::PictureEntry(dive *dIn, const picture &p) : d(dIn), + filename(p.filename), + offsetSeconds(p.offset.seconds), + length({ 0 }) +{ +} + +// Note: it is crucial that this uses the same sorting as the core. +// Therefore, we use the C strcmp functions [std::string::operator<() +// should give the same result]. +bool PictureEntry::operator<(const PictureEntry &p2) const +{ + if (int cmp = comp_dives(d, p2.d)) + return cmp < 0; + if (offsetSeconds != p2.offsetSeconds) + return offsetSeconds < p2.offsetSeconds; + return strcmp(filename.c_str(), p2.filename.c_str()) < 0; +} + DivePictureModel *DivePictureModel::instance() { static DivePictureModel *self = new DivePictureModel(); @@ -23,6 +49,10 @@ DivePictureModel::DivePictureModel() : zoomLevel(0.0) this, &DivePictureModel::updateThumbnail, Qt::QueuedConnection); connect(&diveListNotifier, &DiveListNotifier::pictureOffsetChanged, this, &DivePictureModel::pictureOffsetChanged); + connect(&diveListNotifier, &DiveListNotifier::picturesRemoved, + this, &DivePictureModel::picturesRemoved); + connect(&diveListNotifier, &DiveListNotifier::picturesAdded, + this, &DivePictureModel::picturesAdded); } void DivePictureModel::setZoomLevel(int level) @@ -63,7 +93,7 @@ void DivePictureModel::updateDivePictures() if (dive->selected) { size_t first = pictures.size(); FOR_EACH_PICTURE(dive) - pictures.push_back({ dive, picture->filename, {}, picture->offset.seconds, {.seconds = 0}}); + pictures.push_back(PictureEntry(dive, *picture)); // Sort pictures of this dive by offset. // Thus, the list will be sorted by (dive, offset). @@ -111,43 +141,51 @@ QVariant DivePictureModel::data(const QModelIndex &index, int role) const return QVariant(); } -// Return true if we actually removed a picture -static bool removePictureFromSelectedDive(const char *fileUrl) +void DivePictureModel::removePictures(const QModelIndexList &indices) { - int i; - struct dive *dive; - for_each_dive (i, dive) { - if (dive->selected && remove_picture(&dive->pictures, fileUrl)) { - invalidate_dive_cache(dive); - return true; - } + // Collect pictures to remove by dive + std::vector pics; + for (const QModelIndex &idx: indices) { + if (!idx.isValid()) + continue; + const PictureEntry &item = pictures[idx.row()]; + // Check if we already have pictures for that dive. + auto it = find_if(pics.begin(), pics.end(), + [&item](const Command::PictureListForDeletion &list) + { return list.d == item.d; }); + // If not found, add a new list + if (it == pics.end()) + pics.push_back({ item.d, { item.filename }}); + else + it->filenames.push_back(item.filename); } - return false; + Command::removePictures(pics); } -void DivePictureModel::removePictures(const QVector &fileUrlsIn) +void DivePictureModel::picturesRemoved(dive *d, QVector filenamesIn) { // Transform vector of QStrings into vector of std::strings - std::vector fileUrls; - fileUrls.reserve(fileUrlsIn.size()); - std::transform(fileUrlsIn.begin(), fileUrlsIn.end(), std::back_inserter(fileUrls), + std::vector filenames; + filenames.reserve(filenamesIn.size()); + std::transform(filenamesIn.begin(), filenamesIn.end(), std::back_inserter(filenames), [] (const QString &s) { return s.toStdString(); }); - bool removed = false; - for (const std::string &fileUrl: fileUrls) - removed |= removePictureFromSelectedDive(fileUrl.c_str()); - if (!removed) + // Get range of pictures of the given dive. + // Note: we could be more efficient by either using a binary search or a two-level data structure. + auto from = std::find_if(pictures.begin(), pictures.end(), [d](const PictureEntry &e) { return e.d == d; }); + auto to = std::find_if(from, pictures.end(), [d](const PictureEntry &e) { return e.d != d; }); + if (from == pictures.end()) return; - copy_dive(current_dive, &displayed_dive); - mark_divelist_changed(true); - for (size_t i = 0; i < pictures.size(); ++i) { + size_t fromIdx = from - pictures.begin(); + size_t toIdx = to - pictures.begin(); + for (size_t i = fromIdx; i < toIdx; ++i) { // Find range [i j) of pictures to remove - if (std::find(fileUrls.begin(), fileUrls.end(), pictures[i].filename) == fileUrls.end()) + if (std::find(filenames.begin(), filenames.end(), pictures[i].filename) == filenames.end()) continue; size_t j; - for (j = i + 1; j < pictures.size(); ++j) { - if (std::find(fileUrls.begin(), fileUrls.end(), pictures[j].filename) == fileUrls.end()) + for (j = i + 1; j < toIdx; ++j) { + if (std::find(filenames.begin(), filenames.end(), pictures[j].filename) == filenames.end()) break; } @@ -156,8 +194,48 @@ void DivePictureModel::removePictures(const QVector &fileUrlsIn) beginRemoveRows(QModelIndex(), i, j - 1); pictures.erase(pictures.begin() + i, pictures.begin() + j); endRemoveRows(); + toIdx -= j - i; + } + copy_dive(current_dive, &displayed_dive); // TODO: Remove once displayed_dive is moved to the planner +} + +// Assumes that pics is sorted! +void DivePictureModel::picturesAdded(dive *d, QVector picsIn) +{ + // We only display pictures of selected dives + if (!d->selected || picsIn.empty()) + return; + + // Convert the picture-data into our own format + std::vector pics; + pics.reserve(picsIn.size()); + for (int i = 0; i < picsIn.size(); ++i) + pics.push_back(PictureEntry(d, picsIn[i])); + + // Insert batch-wise to avoid too many reloads + pictures.reserve(pictures.size() + pics.size()); + auto from = pics.begin(); + int dest = 0; + while (from != pics.end()) { + // Search for the insertion index. This supposes a lexicographical sort for the [dive, offset, filename] triple. + // TODO: currently this works, because all undo commands that manipulate the dive list also reset the selection + // and thus the model is rebuilt. However, we might catch the respective signals here and not rely on being + // called by the tab-widgets. + auto dest_it = std::lower_bound(pictures.begin() + dest, pictures.end(), *from); + int dest = dest_it - pictures.begin(); + auto to = dest_it == pictures.end() ? pics.end() : from + 1; // If at the end - just add the rest + while (to != pics.end() && *to < *dest_it) + ++to; + int batch_size = to - from; + beginInsertRows(QModelIndex(), dest, dest + batch_size - 1); + pictures.insert(pictures.begin() + dest, from, to); + // Get thumbnails of inserted pictures + for (auto it = pictures.begin() + dest; it < pictures.begin() + dest + batch_size; ++it) + it->image = Thumbnailer::instance()->fetchThumbnail(QString::fromStdString(it->filename), false); + endInsertRows(); + from = to; + dest += batch_size; } - emit picturesRemoved(fileUrlsIn); } int DivePictureModel::rowCount(const QModelIndex&) const diff --git a/qt-models/divepicturemodel.h b/qt-models/divepicturemodel.h index 395dbedd1..d7b043d44 100644 --- a/qt-models/divepicturemodel.h +++ b/qt-models/divepicturemodel.h @@ -3,6 +3,7 @@ #define DIVEPICTUREMODEL_H #include "core/units.h" +#include "core/pictureobj.h" #include #include @@ -16,6 +17,9 @@ struct PictureEntry { QImage image; int offsetSeconds; duration_t length; + PictureEntry(dive *, const PictureObj &); + PictureEntry(dive *, const picture &); + bool operator<(const PictureEntry &) const; }; class DivePictureModel : public QAbstractTableModel { @@ -26,13 +30,13 @@ public: QVariant data(const QModelIndex &index, int role = Qt::DisplayRole) const override; int rowCount(const QModelIndex &parent = QModelIndex()) const override; void updateDivePictures(); - void removePictures(const QVector &fileUrls); -signals: - void picturesRemoved(const QVector &fileUrls); + void removePictures(const QModelIndexList &); public slots: void setZoomLevel(int level); void updateThumbnail(QString filename, QImage thumbnail, duration_t duration); void pictureOffsetChanged(dive *d, const QString filename, offset_t offset); + void picturesRemoved(dive *d, QVector filenames); + void picturesAdded(dive *d, QVector pics); private: DivePictureModel(); std::vector pictures;