Skip to content
This repository has been archived by the owner on Jul 23, 2019. It is now read-only.

Add mouse "click and drag" to select text #107

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

pranaygp
Copy link

@pranaygp pranaygp commented Jun 9, 2018

I think we can make it way more performant by debouncing "SelectTo" dispatches since we don't need to run that logic to find row/col everytime, however that'll come at the cost of not being as responsive.

What do you think about implementing some sort or "rough estimate" to determin row/column that's faster, and we can debounce an accurate measurement to make everything faster and responsive.

gif

@pranaygp
Copy link
Author

pranaygp commented Jun 9, 2018

I have left to implement alt-click to add a new cursor and use the drag only on that "current" cursor. I will do did that in a different PR (#108). What else is left to implement on the "multi-cursor"/selection front so we can tick the Further optimize multi-cursor editing checkbox off the README?

Copy link
Contributor

@as-cii as-cii left a comment

Choose a reason for hiding this comment

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

This looks like a great start, @pranaygp. Thanks for jumping on this!

My main question about this pull request is related to how we handle scrolling when dragging the cursor near the edges of the window. After a quick test, it seems like we could make the UX much more intuitive by using an approach similar to what Atom does today. In particular, I think we should implement something like the autoscrollOnMouseDrag function, which changes the scroll top position with a speed that's based on the vertical drag delta.

What do you think? Once this gets merged, I'll be happy to review #108 as well.

@pranaygp
Copy link
Author

@as-cii I agree, that does seem a lot more intuitive. Can we still merge this (and possibly #108) while I begin to implement autoscrollOnMouseDrag in a new PR that build on top of these two. This way I can build and test the function on top of this AND #108 to make sure it works as smoothly as I'd like with multi cursor selections too 😄

@as-cii
Copy link
Contributor

as-cii commented Jun 13, 2018

I'd prefer merging clean and well-working units into master and, if I understand correctly, #108 is somewhat orthogonal to the changes proposed in this pull request. How would you feel about changing this and then adapting #108 if necessary? I am not entirely opposed to it, but it'd be nice to have exactly what we want on master before going ahead and merging.

@pranaygp
Copy link
Author

Ah. Sure. Ideally, the changes from #108 don't affect the scrolling behaviour by much so making any changes necessary isn't too much of a hassle.

I'll update this and ping you @as-cii 😄

@as-cii
Copy link
Contributor

as-cii commented Jun 13, 2018

Awesome, thanks for your hard work on this @pranaygp. ⚡️

@as-cii
Copy link
Contributor

as-cii commented Jul 18, 2018

Hey @pranaygp, sorry for the late follow-up. I noticed you pushed a commit since the last time I commented on the issue. Do you think this is ready to review now?

@pranaygp
Copy link
Author

@as-cii I haven't touched the codebase since that commit but iirc, it implements the edge scrolling behavior based on Atom as you recommended. However, the experience seems rather laggy.

I'd love for you to try it out and perhaps recommended places I can look at improving to make it smoother

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants