Fix the selection after a column insert

Author: juzzlin

CHANGELOG

@@ -15,6 +15,11 @@ New features:
 
 Bug fixes:
 
+* Fix the selection covering the wrong columns after a column was inserted or
+  deleted: a column's index is its identity, not its place on screen, so the
+  selection now spans the columns actually dragged across
+  - Velocity interpolation over a selection was reaching the wrong columns too
+
 * Sort the effects gallery alphabetically: the list had drifted out of order as
   effects were added over the years
 

src/application/service/editor_service.cpp

@@ -75,6 +75,18 @@ EditorService::EditorService(SelectionServiceS selectionService, SettingsService
     initialize();
     m_undoStack->setCanUndoChangedCallback([this] { emit canUndoChanged(); });
     m_undoStack->setCanRedoChangedCallback([this] { emit canRedoChanged(); });
+
+    // The selection spans places on screen, and only the song knows which index sits in which place.
+    // Read through the song on every call so that it stays right when a new one is loaded. Cleared
+    // in the destructor, because the selection service can be shared and outlive this.
+    if (m_selectionService) {
+        m_selectionService->setColumnOrderResolver([this](size_t trackIndex) {
+            SelectionService::ColumnIndexList order;
+            const auto indices = columnIndices(static_cast<quint64>(trackIndex));
+            std::ranges::transform(indices, std::back_inserter(order), [](quint64 index) { return static_cast<size_t>(index); });
+            return order;
+        });
+    }
 }
 
 void EditorService::initialize()
@@ -2508,7 +2520,11 @@ void EditorService::requestLinearVelocityInterpolationOnSelection(quint64 startL
 {
     if (m_selectionService->isValidSelection()) {
         std::map<Position, NoteData> oldNoteDataMap;
-        for (auto column = m_selectionService->minColumn(); column <= m_selectionService->maxColumn(); column++) {
+        // The selected columns rather than a count from the lowest index to the highest: an index is
+        // a column's identity, so after an insert they no longer run left to right and counting
+        // through them would reach columns the selection never covered.
+        const auto selectedColumns = m_selectionService->selectedColumns();
+        for (auto && column : selectedColumns) {
             auto start = position();
             start.column = column;
             for (quint64 line = startLine; line <= endLine; ++line) {
@@ -2522,7 +2538,7 @@ void EditorService::requestLinearVelocityInterpolationOnSelection(quint64 startL
 
         NoteEditCommand::ChangeList changes;
 
-        for (auto column = m_selectionService->minColumn(); column <= m_selectionService->maxColumn(); column++) {
+        for (auto && column : selectedColumns) {
 
             auto start = position();
             start.column = column;
@@ -3186,6 +3202,11 @@ quint64 EditorService::totalTicks() const
     return 0;
 }
 
-EditorService::~EditorService() = default;
+EditorService::~EditorService()
+{
+    if (m_selectionService) {
+        m_selectionService->setColumnOrderResolver({});
+    }
+}
 
 } // namespace noteahead

src/application/service/selection_service.cpp

@@ -17,6 +17,9 @@
 
 #include "../../contrib/SimpleLogger/src/simple_logger.hpp"
 
+#include <algorithm>
+#include <utility>
+
 namespace noteahead {
 
 static const auto TAG = "SelectionService";
@@ -60,14 +63,49 @@ size_t SelectionService::track() const
     return m_startPosition.has_value() ? m_startPosition->track : 0;
 }
 
+void SelectionService::setColumnOrderResolver(ColumnOrderResolver resolver)
+{
+    m_columnOrderResolver = std::move(resolver);
+}
+
+SelectionService::ColumnIndexList SelectionService::selectedColumns(size_t trackIndex, size_t startColumn, size_t endColumn) const
+{
+    if (m_columnOrderResolver) {
+        if (const auto order = m_columnOrderResolver(trackIndex); !order.empty()) {
+            const auto startPosition = std::ranges::find(order, startColumn);
+            const auto endPosition = std::ranges::find(order, endColumn);
+            if (startPosition != order.end() && endPosition != order.end()) {
+                const auto first = std::min(startPosition, endPosition);
+                const auto last = std::max(startPosition, endPosition);
+                return { first, last + 1 };
+            }
+        }
+    }
+
+    // No order to go by: the indices are the only one there is.
+    ColumnIndexList columns;
+    for (size_t column = std::min(startColumn, endColumn); column <= std::max(startColumn, endColumn); column++) {
+        columns.push_back(column);
+    }
+    return columns;
+}
+
+SelectionService::ColumnIndexList SelectionService::selectedColumns() const
+{
+    if (!isValidSelection()) {
+        return {};
+    }
+    return selectedColumns(m_startPosition->track, m_startPosition->column, m_endPosition->column);
+}
+
 SelectionService::PositionList SelectionService::selectedPositions() const
 {
     PositionList positions;
 
     if (isValidSelection()) {
         auto start = *m_startPosition;
         auto end = *m_endPosition;
-        for (size_t column = std::min(start.column, end.column); column <= std::max(start.column, end.column); column++) {
+        for (auto && column : selectedColumns(start.track, start.column, end.column)) {
             for (size_t line = std::min(start.line, end.line); line <= std::max(start.line, end.line); line++) {
                 positions.push_back({ start.pattern, start.track, column, line, start.lineColumn });
             }

src/application/service/selection_service.hpp

@@ -18,7 +18,9 @@
 
 #include <QObject>
 
+#include <functional>
 #include <optional>
+#include <vector>
 
 #include "../position.hpp"
 
@@ -52,12 +54,36 @@ class SelectionService : public QObject
     using PositionList = std::vector<Position>;
     PositionList selectedPositions() const;
 
+    using ColumnIndexList = std::vector<size_t>;
+    //! Gives back a track's column indices in the order they are drawn.
+    using ColumnOrderResolver = std::function<ColumnIndexList(size_t trackIndex)>;
+
+    //! Teaches the selection what the columns' order on screen is.
+    //!
+    //! A column's index is its identity, not its place: inserting one gives it the next free index
+    //! wherever it lands, so after an insert the indices no longer run left to right. A selection
+    //! spans what the user dragged across, which is a range of places, and without this there is no
+    //! way to tell which indices those places hold.
+    //!
+    //! Unset -- and for a track the resolver does not know -- the indices are taken as the order,
+    //! which is what they are until something is inserted or deleted.
+    void setColumnOrderResolver(ColumnOrderResolver resolver);
+
+    //! The selected columns' indices, left to right on screen. What anything walking a selection
+    //! across columns has to iterate: the indices themselves are not a range and cannot be counted
+    //! through once a column has been inserted or deleted.
+    Q_INVOKABLE ColumnIndexList selectedColumns() const;
+
 signals:
     void isValidSelectionChanged();
     void selectionCleared(const Position & startPosition, const Position & endPosition);
     void selectionChanged(const Position & startPosition, const Position & endPosition);
 
 private:
+    //! The indices between the two ends of the selection, in display order, both ends included.
+    ColumnIndexList selectedColumns(size_t trackIndex, size_t startColumn, size_t endColumn) const;
+
+    ColumnOrderResolver m_columnOrderResolver;
     std::optional<Position> m_startPosition;
     std::optional<Position> m_endPosition;
 };

src/unit_tests/editor_service_test/editor_service_test.cpp

@@ -194,6 +194,39 @@ void EditorServiceTest::test_removePatternFromMiddle_shouldNotRemoveLastPattern(
     QCOMPARE(editorService.patternAtSongPosition(1), 2);
 }
 
+void EditorServiceTest::test_selection_afterColumnInsert_shouldFollowDisplayOrder()
+{
+    // The reported bug, against a real song rather than a stand-in order: insert a column and the
+    // selection rectangle stops matching the columns dragged across.
+    const auto selectionService = std::make_shared<SelectionService>();
+    EditorService editorService { selectionService, std::make_shared<SettingsService>(), std::make_shared<AutomationService>(std::make_shared<PropertyService>()), std::make_shared<DataService>() };
+
+    editorService.requestNewColumn(0);
+    editorService.requestNewColumn(0);
+    QCOMPARE(editorService.columnCount(0), static_cast<quint64>(3));
+
+    // A new column on the left of the track. It takes the next free index and lands at the front,
+    // so the indices stop running left to right.
+    editorService.requestPosition(0, 0, 0, 0, 0);
+    editorService.requestNewColumnToLeft();
+
+    const auto order = editorService.columnIndices(0);
+    QCOMPARE(order.size(), 4);
+
+    // Drag across the first three columns on screen.
+    selectionService->requestSelectionStart(0, 0, static_cast<size_t>(order.at(0)), 0);
+    selectionService->requestSelectionEnd(0, 0, static_cast<size_t>(order.at(2)), 0);
+
+    for (int i = 0; i < 3; i++) {
+        QVERIFY2(selectionService->isSelected(0, 0, static_cast<size_t>(order.at(i)), 0),
+                 qPrintable(QString { "Column at display position %1 (index %2) is not selected" }.arg(i).arg(order.at(i))));
+    }
+
+    // And the fourth, which the drag never reached, must stay out of it.
+    QVERIFY2(!selectionService->isSelected(0, 0, static_cast<size_t>(order.at(3)), 0),
+             qPrintable(QString { "Column at display position 3 (index %1) should not be selected" }.arg(order.at(3))));
+}
+
 void EditorServiceTest::test_columnCutPaste_equalSizes_shouldCopyColumn()
 {
     EditorService editorService { std::make_shared<SelectionService>(), std::make_shared<SettingsService>(), std::make_shared<AutomationService>(std::make_shared<PropertyService>()), std::make_shared<DataService>() };

src/unit_tests/editor_service_test/editor_service_test.hpp

@@ -36,6 +36,8 @@ private slots:
     void test_removePattern_shouldRemovePattern();
     void test_removePatternFromMiddle_shouldNotRemoveLastPattern();
 
+    void test_selection_afterColumnInsert_shouldFollowDisplayOrder();
+
     void test_columnCutPaste_equalSizes_shouldCopyColumn();
     void test_columnCutPaste_shorterTarget_shouldCopyColumn();
     void test_columnCopyPaste_equalSizes_shouldCopyColumn();

src/unit_tests/selection_service_test/selection_service_test.cpp

@@ -54,6 +54,46 @@ void SelectionServiceTest::test_selectedPositions_reversed_shouldReturnCorrectRa
     QCOMPARE(positions.at(2).line, 6);
 }
 
+void SelectionServiceTest::test_selectedPositions_reorderedColumns_shouldFollowDisplayOrder()
+{
+    SelectionService service;
+
+    // A track that had columns 0, 1, 2 and then got a new one inserted on the left. The new column
+    // takes the next free index, 3, and sits at display position 0: an index is an identity, not a
+    // place on screen, so after an insert the two stop agreeing.
+    service.setColumnOrderResolver([](size_t) { return SelectionService::ColumnIndexList { 3, 0, 1, 2 }; });
+
+    // Dragging across the first three columns on screen, which are indices 3, 0 and 1.
+    service.requestSelectionStart(0, 0, 3, 0);
+    service.requestSelectionEnd(0, 0, 1, 0);
+
+    QVERIFY(service.isSelected(0, 0, 3, 0));
+    QVERIFY(service.isSelected(0, 0, 0, 0));
+    QVERIFY(service.isSelected(0, 0, 1, 0));
+
+    // Index 2 is the fourth column on screen, past the end of the drag. Walking indices numerically
+    // from 1 to 3 would have taken it in and left out index 0, which is what made the rectangle
+    // cover the wrong columns.
+    QVERIFY(!service.isSelected(0, 0, 2, 0));
+
+    // Left to right on screen, which is what anything walking the selection has to follow.
+    const SelectionService::ColumnIndexList expected { 3, 0, 1 };
+    QCOMPARE(service.selectedColumns(), expected);
+}
+
+void SelectionServiceTest::test_selectedPositions_noResolver_shouldFallBackOnIndexOrder()
+{
+    // Without a resolver there is nothing to say what the display order is, so the indices are the
+    // only order there is. Keeps a bare SelectionService behaving as it always did.
+    SelectionService service;
+    service.requestSelectionStart(0, 0, 1, 0);
+    service.requestSelectionEnd(0, 0, 3, 0);
+
+    QVERIFY(service.isSelected(0, 0, 1, 0));
+    QVERIFY(service.isSelected(0, 0, 2, 0));
+    QVERIFY(service.isSelected(0, 0, 3, 0));
+}
+
 void SelectionServiceTest::test_isValidSelection_shouldReturnFalseForIncompleteSelection()
 {
     SelectionService service;

src/unit_tests/selection_service_test/selection_service_test.hpp

@@ -28,6 +28,8 @@ private slots:
     void test_selectedPositions_shouldReturnEmptyIfInvalid();
     void test_selectedPositions_shouldReturnCorrectRange();
     void test_selectedPositions_reversed_shouldReturnCorrectRange();
+    void test_selectedPositions_reorderedColumns_shouldFollowDisplayOrder();
+    void test_selectedPositions_noResolver_shouldFallBackOnIndexOrder();
 
     void test_isValidSelection_shouldReturnFalseForIncompleteSelection();
     void test_isValidSelection_shouldReturnTrueForValidSelection();