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

Improve cell selection #92

Merged
merged 11 commits into from Jul 24, 2020
Merged

Conversation

jeanp413
Copy link
Contributor

@jeanp413 jeanp413 commented Jul 16, 2020

Improves the selection while dragging with the mouse, shift + click , shift + home, shift + end.
Took as reference the HxD editor

Fixes #78

@lramos15
Copy link
Member

lramos15 commented Jul 16, 2020

Thanks for the PR! I have a few pieces of work I need to get done first, but I promise to review this before the next release. Can you please ensure you've run eslint against your code and that you have updated all jsdoc comments I saw one which was missing the force variable that I didn't know what it meant. I would remove shift + page up, shift + page down as I feel like they have the most limited use cases and are definitely going to be the buggiest / hardest to implement. As for the shift + home and shift + end those shouldn't have to wait for render as the row you're working for is the current row which the user has focus which gurantees it is on the DOM.

@jeanp413
Copy link
Contributor Author

Sorry, I meant shift + ctrl + home, shift + ctrl + end (updated the original post).
Another related issue is when you select a lot of cells either with dragging + scrolling or shift + click + scrolling, the cells that go out of view (and therefore removed from the DOM) lost its selected state. Again, to fix this I need to run the select logic after the render method finished

Update toggleSelectOffset jsdoc
@lramos15
Copy link
Member

lramos15 commented Jul 17, 2020

I think the real fix is that the render method doesn't render the selection as well. If it did that it wouldn't have to worry about being waited on before making a selection. This used to be the case but since the addition of multiselect I didn't fix the webview state manager to hold more than one selection #95. I think the major fix would be some sort of decorations layer which holds whatever extra metadata related to the cell and then gets rendered on top. I don't really want to merge something that introduces more bugs so I would recommend pulling the more buggy things into a second PR and we can work on getting the first one merged.

@jeanp413
Copy link
Contributor Author

Reverted the selection with pg up, pg down, ctrl+home and ctrl+end. Let me know if more changes are needed.
BTW right now the rangeSelect logic is a bit complicated to avoid calling querySelectorAll used in getElementsWithGivenOffset. I guess this will be refactored in the future to hold the references to the dom nodes and the logic to select a range would be much simpler.

@lramos15 lramos15 mentioned this pull request Jul 20, 2020
10 tasks
@lramos15
Copy link
Member

Once merge conflicts are resolved I'm ready to review your PR whenever you're ready for my review. Sorry I was working on a large feature for this iteration and wanted to make sure it was complete in time. I added preservation of the entire selection range via the WebviewStateManager see media/selectHandler.ts for that. This should allow you to preserve your selections and not have to wait for the render method. Let me know if you're blocked on anything.

Merge remote-tracking branch 'upstream/master' into improve-selection
@jeanp413
Copy link
Contributor Author

@lramos15 it's ready for review, resolved the merge conflicts and updated the selection logic

Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

Sorry for all the comments, I tried to be thorough. This is really great work and seems to work well when I tested it, I see there were a lot of random tabs or spaces I had. One thing that feels really broken to me is pressing shift with moving the arrow keys. Let me know if you have any questions.

media/editHandler.ts Show resolved Hide resolved
media/selectHandler.ts Show resolved Hide resolved
media/selectHandler.ts Show resolved Hide resolved
media/selectHandler.ts Show resolved Hide resolved
media/selectHandler.ts Outdated Show resolved Hide resolved
this.searchHandler.searchKeybindingHandler();
} else if (event.keyCode >= 37 && event.keyCode <= 40) {
this.arrowKeyNavigate(event.keyCode, targetElement);
} else if ((event.keyCode >= 37 && event.keyCode <= 40 /*Arrows*/)
Copy link
Member

Choose a reason for hiding this comment

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

Ideally we would switch away from keycodes here as it's been deprecated. This is also a debt that I need to fix which will make it easier to understand what's going on here as event.keyCode === 35 doesn't convey home as well as event.key === "Home"

media/virtualDocument.ts Outdated Show resolved Hide resolved
media/virtualDocument.ts Show resolved Hide resolved
media/virtualDocument.ts Show resolved Hide resolved
media/virtualDocument.ts Show resolved Hide resolved
@jeanp413
Copy link
Contributor Author

Pushed some changes addressing the feedback

Copy link
Member

@lramos15 lramos15 left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for the PR!

@lramos15 lramos15 merged commit 091f010 into microsoft:master Jul 24, 2020
@jeanp413 jeanp413 deleted the improve-selection branch July 24, 2020 16:41
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.

Click and Drag Selection does not shrink
2 participants