Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extended Selection & Deletion of Path Points #179

Merged
merged 8 commits into from
Sep 1, 2020
Merged

Conversation

RobertBColton
Copy link
Contributor

@RobertBColton RobertBColton commented Sep 1, 2020

  • Overload RowRemovalOperation::RemoveRows to accept a QModelIndexList of all selected rows.
  • Set QAbstractItemView::ExtendedSelection on Path editor points list view so we can select to delete many points at once. This does not change the fact that there is a current index inside the selection which has the focus if an edit is requested.
  • Set QAbstractItemView::SelectRows on Path editors points list view so we are actually selecting the entire point, which is similar to GM/LGM and feels less awkward. There is still a current index inside the selected row which has the focus if an edit is requested in a specific column item (aka the field).
  • Use the new RowRemovalOperation overload to bulk delete all the selected points in the path editor. I used QItemSelectionModel::selectedRows so that we only delete points where every column (aka the entire row) is selected. This should always be the case since I've also changed the selection behavior to select rows instead of items.
  • Remove selection after setting the current index, which already does a correct selection. As I've stated, the current index resides in the selection and belongs to the selection model.
  • Add clarifying comments to the selection update slot about interaction with the above changes.
  • The selected indexes passed to selection change slot may be empty and yet there is still a selection and so enabling of the delete button now needs to check the global selection model of the points table to determine if anything is selected. It should also check if there are selected rows and not just selected items.
  • The selected point in the preview area is now the last, most recently selected point in the global, cumulative list selection.
  • Deleting a point now keeps the same row selected, unless the last point is being deleted. This is how GM and the main Qt examples work with as well. Bounds checking is done against the row count so when the last point is deleted we do go to the previous row.

Extended Path Point Selection

Understand that I am still not entirely sure the interaction of this with the current index. It seems that it is possible for the current index (which has the focus?) to reside outside of the selection if we deselect the current index. I am not sure how this interacts with QTreeView::allColumnsShowFocus but I suspect it's related. Regardless, other than being weird and not sure of the expected behavior, it doesn't seem to cause any issues.

In a view, there is always a current item and a selected item - two independent states. An item can be the current item and selected at the same time. The view is responsible for ensuring that there is always a current item as keyboard navigation, for example, requires a current item.
https://doc.qt.io/qt-5/model-view-programming.html#current-item-and-selected-items

Current Index Outside Selection

@RobertBColton RobertBColton changed the title Bulk Selection & Deletion of Path Points Extended Selection & Deletion of Path Points Sep 1, 2020
Comment on lines 217 to 219
// if new points were added to the selection, select them in the preview
if (!selected.indexes().empty()) selectIndex = selected.indexes()[0].row();
_ui->roomView->selectedPointIndex = selectIndex;
Copy link
Contributor Author

@RobertBColton RobertBColton Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally and eventually I want these previews to share a selection model with the list views and stuff. The selected points should be the same as those selected in the list so they can be moved together logically. We should also probably be painting the selection using the current style so it's consistent with platform conventions. The views should also maybe maintain a current index with focus indication like QGraphicsView/QGraphicsScene does.
https://doc.qt.io/qt-5/qtwidgets-itemviews-chart-example.html

@@ -234,15 +238,15 @@ void PathEditor::on_deletePointButton_pressed() {
int deleteIndex = _ui->pointsTableView->selectionModel()->currentIndex().row();
{
RepeatedMessageModel::RowRemovalOperation remover(_pointsModel);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought this was going to be a for-loop, now. Write it like this, please:

RepeatedMessageModel::RowRemovalOperation(_pointsModel)
    .RemoveRows(_ui->pointsTableView->selectionModel()->selectedRows());

Copy link
Contributor Author

@RobertBColton RobertBColton Sep 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aight 25f71db and it is a for loop, it's a for loop in the class so we don't duplicate it everywhere.
https://github.com/enigma-dev/RadialGM/pull/179/files#diff-9223a9db2dfd4809200a0a8574385ed1R147

@RobertBColton RobertBColton merged commit 33a6d74 into master Sep 1, 2020
@RobertBColton RobertBColton deleted the remove-selection branch September 1, 2020 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants