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

Clean up cell selection behavior #3414

Merged
merged 30 commits into from Dec 23, 2017
Merged

Clean up cell selection behavior #3414

merged 30 commits into from Dec 23, 2017

Conversation

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Dec 19, 2017

This PR:

  • disambiguates selected from active at the notebook widget level. There is a new function isSelectedOrActive that is the equivalent of the previous isSelected, and now isSelected and friends only refer to the selected property of a cell.
  • introduces a contiguous cell selection framework on the notebook widget. A contiguous selection is a single contiguous sequence of selected cells with the active cell at one end of the selection. The head of the selection is the active cell, and the anchor is the other end of the selection. Comprehensive tests are included for this framework.
  • Converts all selection UX to use the contiguous selection framework
  • Enhances the user interaction for selection so that it behaves more like text selection - shift-click and shift-drag updates a selection, clicking outside deselects all, etc.
  • Fixes a bug in updating the active cell index when a cell is moved
  • Other refactoring as necessary.

Fixes #3373.

Another PR should create comprehensive tests for the UX around selection, particularly with keyboard handling and mouse events.

@jasongrout jasongrout changed the title Add tests for selection [WIP] Add tests for selection Dec 19, 2017
@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 19, 2017

TDD baby!

Loading

@jasongrout jasongrout changed the title [WIP] Add tests for selection [WIP] Audit cell selection behavior Dec 20, 2017
@jasongrout jasongrout changed the title [WIP] Audit cell selection behavior [WIP] Clean up cell selection behavior Dec 20, 2017
@blink1073 blink1073 added this to the Beta milestone Dec 20, 2017
Copy link
Member

@blink1073 blink1073 left a comment

Nice!

Loading

if (index === this.activeCellIndex) {
// Already collapsed selection, nothing more to do.
return;
} else {
Copy link
Member

@blink1073 blink1073 Dec 20, 2017

Choose a reason for hiding this comment

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

else after a return

Loading

Copy link
Contributor Author

@jasongrout jasongrout Dec 21, 2017

Choose a reason for hiding this comment

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

Thanks!

Loading

Private.selectedProperty.set(this.widgets[head], false);
}

// Toggle everything strictly between head and index except anchor.
Copy link
Member

@blink1073 blink1073 Dec 20, 2017

Choose a reason for hiding this comment

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

Reverse order in comment for clarity? between index and head?

Loading

expect(selected(widget)).to.eql([]);
widget.select(widget.activeCell);
expect(selected(widget)).to.eql([widget.activeCellIndex]);
})
Copy link
Member

@blink1073 blink1073 Dec 20, 2017

Choose a reason for hiding this comment

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

Missing semicolon (from the lint report)

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 21, 2017

Tested locally, fixed one bug where shift+click would immediately deselect on mouseup. This LGTM, thanks!

Loading

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Dec 21, 2017

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 21, 2017

Tested locally, fixed one bug where shift+click would immediately deselect on mouseup. This LGTM, thanks!

I think I accidentally overwrote your fix with a force-push (since this was still a WIP PR), but I think I fixed the same thing.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 21, 2017

@ellisonbg - there still are some questions about the interaction of selection and dragging, but I think the selection by itself is ready for comment. One problem I did notice with the current PR is that selecting a different notebook resets the active cell for the notebook you are leaving to 0.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 22, 2017

@ellisonbg - I think this is ready for taking a spin. I haven't written tests for commits past fa914e3. I can work on writing those tests, or I can work on something more pressing for the release.

Loading

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Dec 22, 2017

Hmm, this builds but lab is pretty broken. Like the themes are not loading. @jasongrout have you rebased today? Could you? A lot has changed and I want to test it with everything else in place.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 22, 2017

Sure, I could try to rebase. I'm still not quite happy with how selection and dragging interplays, so I'm tackling that again.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 22, 2017

@ellisonbg - I'm much happier with how this works now. There still is a bug where it highlights too much when dragging a selection in the same document.

I'll rebase this tomorrow, but probably will not be able to do it in the morning.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 22, 2017

Note to self: some of the tests are out of date, and lots of new tests need to be written to clarify the behavior. Let's see how many we can get done before a release deadline :).

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 22, 2017

(Of note, just like in a text editor, now shift and dragging the mouse changes the selection)

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 22, 2017

The bug that I'm still tracking down is if I highlight a bunch of cells and drag to move them in the same document, the active cell after the move seems to be randomly set - sometimes it's at the top of the selected/moved cells, sometimes at the bottom, and sometimes in the middle (!).

I think it's a race condition with the cell widgets catching up to the model moves. I'll investigate more when I'm back online in a few hours.

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 22, 2017

I rebased on master.

Loading

We were not considering the case when a cell moves from before to behind the active cell, or vice versa.
@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 23, 2017

LGTM, no longer WIP?

Loading

@jasongrout jasongrout changed the title [WIP] Clean up cell selection behavior Clean up cell selection behavior Dec 23, 2017
@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 23, 2017

No longer WIP. Hopefully the tests pass (they do on my computer).

Loading

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Dec 23, 2017

Travis passes!

Loading

@blink1073
Copy link
Member

@blink1073 blink1073 commented Dec 23, 2017

Great work, thank you!

Loading

@blink1073 blink1073 merged commit f635072 into jupyterlab:master Dec 23, 2017
2 checks passed
Loading
@blink1073 blink1073 mentioned this pull request Dec 23, 2017
1 task
@lock lock bot locked as resolved and limited conversation to collaborators Aug 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants