From 487974ea91381c3dfeb31d6ccc0374e1ce85e85c Mon Sep 17 00:00:00 2001 From: Berthold Stoeger Date: Sat, 21 May 2022 20:38:17 +0200 Subject: [PATCH] selection: avoid select_dive() and deselect_dive calls in dive list Each of these calls recalculates the current dive and divecomputer. Instead, collect the dives to be selected/deselected and (de)select them at once. This needs some code refactoring in the core, because we need a function that 1) doesn't send a signal by itself. 2) doesn't clear the trip-selection. This contains some reorganization of the selection functions signatures: The filter code is the only caller that keeps the selected dive and the only caller that cares about whether the current dive changed. So let only the function that keeps the selected dive return whether the current dive changed. It's all very fragile. Signed-off-by: Berthold Stoeger --- core/divefilter.cpp | 4 +-- core/selection.cpp | 61 +++++++++++++++++++++----------- core/selection.h | 10 ++++-- core/trip.c | 20 ----------- core/trip.h | 3 -- desktop-widgets/divelistview.cpp | 35 ++++++++++++------ 6 files changed, 76 insertions(+), 57 deletions(-) diff --git a/core/divefilter.cpp b/core/divefilter.cpp index 15e880613..2c72c1ac9 100644 --- a/core/divefilter.cpp +++ b/core/divefilter.cpp @@ -67,7 +67,7 @@ ShownChange DiveFilter::update(const QVector &dives) const updateDiveStatus(d, newStatus, res, removeFromSelection); } updateSelection(selection, std::vector(), removeFromSelection); - res.currentChanged = setSelection(selection); + res.currentChanged = setSelectionKeepCurrent(selection); return res; } @@ -107,7 +107,7 @@ ShownChange DiveFilter::updateAll() const } } updateSelection(selection, std::vector(), removeFromSelection); - res.currentChanged = setSelection(selection); + res.currentChanged = setSelectionKeepCurrent(selection); return res; } diff --git a/core/selection.cpp b/core/selection.cpp index 9baa79c53..840965127 100644 --- a/core/selection.cpp +++ b/core/selection.cpp @@ -177,21 +177,17 @@ static void setClosestCurrentDive(timestamp_t when, const std::vector &s } } -// Reset the selection to the dives of the "selection" vector and send the appropriate signals. + +// Reset the selection to the dives of the "selection" vector. // Set the current dive to "currentDive". "currentDive" must be an element of "selection" (or -// null if "selection" is empty). Return true if the current dive changed. -bool setSelection(const std::vector &selection, dive *currentDive, int currentDc) +// null if "selection" is empty). +// Does not send signals or clear the trip selection. +QVector setSelectionCore(const std::vector &selection, dive *currentDive, int currentDc) { // To do so, generate vectors of dives to be selected and deselected. // We send signals batched by trip, so keep track of trip/dive pairs. QVector divesToSelect; divesToSelect.reserve(selection.size()); - const dive *oldCurrent = current_dive; - - // Since we select only dives, there are no selected trips! - amount_trips_selected = 0; - for (int i = 0; i < divelog.trips->nr; ++i) - divelog.trips->trips[i]->selected = false; // TODO: We might want to keep track of selected dives in a more efficient way! int i; @@ -213,11 +209,6 @@ bool setSelection(const std::vector &selection, dive *currentDive, int c ++amount_selected; divesToSelect.push_back(d); } - // TODO: Instead of using select_dive() and deselect_dive(), we set selected directly. - // The reason is that deselect() automatically sets a new current dive, which we - // don't want, as we set it later anyway. - // There is other parts of the C++ code that touches the innards directly, but - // ultimately this should be pushed down to C. d->selected = newState; } @@ -232,18 +223,48 @@ bool setSelection(const std::vector &selection, dive *currentDive, int c if (currentDc >= 0) dc_number = currentDc; fixup_current_dc(); - - // Send the new selection - emit diveListNotifier.divesSelected(divesToSelect); - return current_dive != oldCurrent; + return divesToSelect; } -bool setSelection(const std::vector &selection) +static void clear_trip_selection() { + amount_trips_selected = 0; + for (int i = 0; i < divelog.trips->nr; ++i) + divelog.trips->trips[i]->selected = false; +} + +// Reset the selection to the dives of the "selection" vector and send the appropriate signals. +// Set the current dive to "currentDive". "currentDive" must be an element of "selection" (or +// null if "selection" is empty). +// If "currentDc" is negative, an attempt will be made to keep the current computer number. +void setSelection(const std::vector &selection, dive *currentDive, int currentDc) +{ + // Since we select only dives, there are no selected trips! + clear_trip_selection(); + + auto selectedDives = setSelectionCore(selection, currentDive, currentDc); + + // Send the new selection to the UI. + emit diveListNotifier.divesSelected(selectedDives); +} + +// Set selection, but try to keep the current dive. If current dive is not in selection, +// find the nearest current dive in the selection +// Returns true if the current dive changed. +// Do not send a signal. +bool setSelectionKeepCurrent(const std::vector &selection) +{ + // Since we select only dives, there are no selected trips! + clear_trip_selection(); + + const dive *oldCurrent = current_dive; + dive *newCurrent = current_dive; if (current_dive && std::find(selection.begin(), selection.end(), current_dive) == selection.end()) newCurrent = closestInSelection(current_dive->when, selection); - return setSelection(selection, newCurrent, -1); + setSelectionCore(selection, newCurrent, -1); + + return current_dive != oldCurrent; } extern "C" void select_single_dive(dive *d) diff --git a/core/selection.h b/core/selection.h index 81c647f27..f1c70dc9b 100644 --- a/core/selection.h +++ b/core/selection.h @@ -40,18 +40,24 @@ extern void dump_selection(void); #ifdef __cplusplus #include +#include // Reset the selection to the dives of the "selection" vector and send the appropriate signals. // Set the current dive to "currentDive" and the current dive computer to "currentDc". // "currentDive" must be an element of "selection" (or null if "seletion" is empty). // If "currentDc" is negative, an attempt will be made to keep the current computer number. +// Returns the list of selected dives +QVector setSelectionCore(const std::vector &selection, dive *currentDive, int currentDc); + +// As above, but sends a signal to inform the frontend of the changed selection. // Returns true if the current dive changed. -bool setSelection(const std::vector &selection, dive *currentDive, int currentDc); +void setSelection(const std::vector &selection, dive *currentDive, int currentDc); // Set selection, but try to keep the current dive. If current dive is not in selection, // find the nearest current dive in the selection // Returns true if the current dive changed. -bool setSelection(const std::vector &selection); +// Does not send a signal. +bool setSelectionKeepCurrent(const std::vector &selection); // Get currently selected dives std::vector getDiveSelection(); diff --git a/core/trip.c b/core/trip.c index c60cde049..d57937360 100644 --- a/core/trip.c +++ b/core/trip.c @@ -287,26 +287,6 @@ dive_trip_t *get_dives_to_autogroup(struct dive_table *table, int start, int *fr return NULL; } -void deselect_dives_in_trip(struct dive_trip *trip) -{ - if (!trip) - return; - for (int i = 0; i < trip->dives.nr; ++i) - deselect_dive(trip->dives.dives[i]); -} - -void select_dives_in_trip(struct dive_trip *trip) -{ - struct dive *dive; - if (!trip) - return; - for (int i = 0; i < trip->dives.nr; ++i) { - dive = trip->dives.dives[i]; - if (!dive->hidden_by_filter) - select_dive(dive); - } -} - /* Out of two strings, copy the string that is not empty (if any). */ static char *copy_non_empty_string(const char *a, const char *b) { diff --git a/core/trip.h b/core/trip.h index 174beebdb..260bf3234 100644 --- a/core/trip.h +++ b/core/trip.h @@ -49,9 +49,6 @@ extern dive_trip_t *get_trip_for_new_dive(struct dive *new_dive, bool *allocated extern dive_trip_t *get_trip_by_uniq_id(int tripId); extern bool trips_overlap(const struct dive_trip *t1, const struct dive_trip *t2); -extern void select_dives_in_trip(struct dive_trip *trip); -extern void deselect_dives_in_trip(struct dive_trip *trip); - extern dive_trip_t *combine_trips(struct dive_trip *trip_a, struct dive_trip *trip_b); extern bool is_trip_before_after(const struct dive *dive, bool before); extern bool trip_is_single_day(const struct dive_trip *trip); diff --git a/desktop-widgets/divelistview.cpp b/desktop-widgets/divelistview.cpp index 46b4a15fb..bad5048e4 100644 --- a/desktop-widgets/divelistview.cpp +++ b/desktop-widgets/divelistview.cpp @@ -548,17 +548,18 @@ void DiveListView::selectionChanged(const QItemSelection &selected, const QItemS QItemSelection newSelected = selected.size() ? selected : selectionModel()->selection(); + std::vector addToSelection, removeFromSelection; Q_FOREACH (const QModelIndex &index, newDeselected.indexes()) { if (index.column() != 0) continue; const QAbstractItemModel *model = index.model(); struct dive *dive = model->data(index, DiveTripModelBase::DIVE_ROLE).value(); - if (!dive) { // it's a trip! - dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value(); + if (dive) { + removeFromSelection.push_back(dive); + } else if (dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value()) { deselect_trip(trip); - deselect_dives_in_trip(trip); - } else { - deselect_dive(dive); + for (int i = 0; i < trip->dives.nr; ++i) + removeFromSelection.push_back(trip->dives.dives[i]); } } Q_FOREACH (const QModelIndex &index, newSelected.indexes()) { @@ -567,10 +568,12 @@ void DiveListView::selectionChanged(const QItemSelection &selected, const QItemS const QAbstractItemModel *model = index.model(); struct dive *dive = model->data(index, DiveTripModelBase::DIVE_ROLE).value(); - if (!dive) { // it's a trip! - dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value(); + if (dive) { + addToSelection.push_back(dive); + } else if (dive_trip *trip = model->data(index, DiveTripModelBase::TRIP_ROLE).value()) { select_trip(trip); - select_dives_in_trip(trip); + for (int i = 0; i < trip->dives.nr; ++i) + addToSelection.push_back(trip->dives.dives[i]); if (model->rowCount(index)) { QItemSelection selection; selection.select(model->index(0, 0, index), model->index(model->rowCount(index) - 1, 0, index)); @@ -579,11 +582,23 @@ void DiveListView::selectionChanged(const QItemSelection &selected, const QItemS if (!isExpanded(index)) expand(index); } - } else { - select_dive(dive); } } + // Funny: It can happen that dives were added to the add and remove lists. + // For example, when switching from a trip to a single dive in the trip. + // To avoid ill-define situations, clean that up. + for (dive *d: addToSelection) { + auto it = std::find(removeFromSelection.begin(), removeFromSelection.end(), d); + if (it != removeFromSelection.end()) + removeFromSelection.erase(it); + } + + std::vector selection = getDiveSelection(); + updateSelection(selection, addToSelection, removeFromSelection); + dive *newCurrent = selection.empty() ? nullptr : selection.front(); + setSelectionCore(selection, newCurrent, -1); + // Display the new, processed, selection QTreeView::selectionChanged(selectionModel()->selection(), newDeselected); }