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

Implement rectangular selection editing #4326

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@GKFX
Contributor

GKFX commented Feb 23, 2016

Fixes #4250. There was a bit of code for working with column selections—obviously you could make them—but the code for editing with them was just unfinished. This makes ordinary typing, pasting, backspace and delete work, even with overtype mode on.

@GKFX

This comment has been minimized.

Show comment
Hide comment
@GKFX

GKFX Feb 24, 2016

Contributor

NB: for undoability, this really depends on #4310. Ideally, that would be checked and merged first. I've tested them together on my machine.

Contributor

GKFX commented Feb 24, 2016

NB: for undoability, this really depends on #4310. Ideally, that would be checked and merged first. I've tested them together on my machine.

// making it input handler-specific.
char c = event.getKeyChar();
if (!rectSelect || (c != '\n' && c != '\r') ||
(event.getModifiers() & InputEvent.META_MASK) != 0) {

This comment has been minimized.

@JakubValtar

JakubValtar May 9, 2016

Contributor

@GKFX Quick question, why do you check META here?

@JakubValtar

JakubValtar May 9, 2016

Contributor

@GKFX Quick question, why do you check META here?

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 9, 2016

Member

Is this something we even want to support? It seems like the sort of thing that's going to open up many more headaches (especially as we try to support Chinese, Japanese, Korean...)

Member

benfry commented May 9, 2016

Is this something we even want to support? It seems like the sort of thing that's going to open up many more headaches (especially as we try to support Chinese, Japanese, Korean...)

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 9, 2016

Contributor

I'm not sure whether it's worth it. I tried this PR today and it still has some quirks which would need to be ironed out.

However, having multiple cursors (selections) or rectangular selection is the thing right now and can be very useful when done properly. I'm using it in IDEA from time to time. Not sure how many people, easpecially beginners, can make use of it though.

Maybe it would be better to first focus on making Undo and Find/Replace solid, then think about rect mode and whether we can implement it neatly without headaches.

Contributor

JakubValtar commented May 9, 2016

I'm not sure whether it's worth it. I tried this PR today and it still has some quirks which would need to be ironed out.

However, having multiple cursors (selections) or rectangular selection is the thing right now and can be very useful when done properly. I'm using it in IDEA from time to time. Not sure how many people, easpecially beginners, can make use of it though.

Maybe it would be better to first focus on making Undo and Find/Replace solid, then think about rect mode and whether we can implement it neatly without headaches.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 9, 2016

Member

If this patch isn't perfect, let's not do it. We can wait until we either have a better patch and someone who wants to actively support it. The half-implemented support (that I wasn't aware of) probably needs to be disabled if it exists so that it's "not implemented," not a "bug."

(The mess created by the comment/uncomment change we're discussing right now is a good example of why this sort of half-implemented feature is a huge headache.)

Member

benfry commented May 9, 2016

If this patch isn't perfect, let's not do it. We can wait until we either have a better patch and someone who wants to actively support it. The half-implemented support (that I wasn't aware of) probably needs to be disabled if it exists so that it's "not implemented," not a "bug."

(The mess created by the comment/uncomment change we're discussing right now is a good example of why this sort of half-implemented feature is a huge headache.)

@GKFX

This comment has been minimized.

Show comment
Hide comment
@GKFX

GKFX May 9, 2016

Contributor

@JakubValtar Besides the meta key thing (which I'll check; I think I had a reason but it's all a bit hazy) what were the quirks you found? I can go through them and try to improve it.

Contributor

GKFX commented May 9, 2016

@JakubValtar Besides the meta key thing (which I'll check; I think I had a reason but it's all a bit hazy) what were the quirks you found? I can go through them and try to improve it.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 9, 2016

Contributor

@benfry Maybe we should wait then.

@GKFX If we are going full in, we should not have separate code for single cursor and for rect selection (which probably means multiple cursors). The code for multiple cursors should be able to handle single cursor, because it's basically multiple cursors with the size of 1. I want to avoid having the same code (and related bugs) twice in the code and also I don't want the uncertainty which code will run when I call some method setting/getting text.

I think most of the problems/ideas described below will simply disappear once you start treating the rect selection as multiple single cursors (multiple single selections). The act of rect selecting will then be just a special way to place multiple cursors (multiple selections).

The existing code where feeding in multiple cursors/selections does not make sense would get only the last cursor/selection.

To implement this, cursorOffset, selctionStart and selectionEnd fields in the JEditTextArea would have to go from int to int[]

Some things which I think should be fixed/handled:

  • Ctrl interferes with Ctrl+Click (Go to definition), maybe we should consider changing it to Alt and I would love to see mouse wheel drag to be added too (I still need to polish Ctrl+Click to ignore when mouse is dragged)
  • When I press enter while in rect select, I'm left only with a single cursor
  • Undo does not group individual letters into undo group, undos letter by letter (not a big deal, just annoying)
  • Undo leaves you with a single cursor, which is placed at different location than where the edits occurred (confusing and you can't continue typing multiline after that)
  • If the first line of the selection is ending at the offset where the multiline cursor is, delete key does nothing - it should keep deleting chars on lines where there are some chars to the right
  • Starting dragging vertically to the right of the line endings should place cursors at the ends of lines; now works only when I start dragging on a long line (scenario: you want to collapse stuff from more lines onto single line)
  • Check the way how the rect selection interacts with the existing code - what do I get in selectionStart and selectionStop? What do I get when I call getSelection?

Things I would expect from "perfect" implementation

  • Holding Alt and clicking should place multiple cursors on all places where you click
  • Deleting line ending should pop the following line at the end and leave you with two cursors on that line (scenario: collapsing multiple line onto one line and adding commas in between)
  • Cursors should not be placed on shorter lines when selection is at least one character wide (scenario: changing int to float in front of bunch of variables separated by empty lines)

Let's ask @benfry if he thinks I'm being reasonable here and this is something we want before starting working on this (if you would still like to work on it of course) :)

Contributor

JakubValtar commented May 9, 2016

@benfry Maybe we should wait then.

@GKFX If we are going full in, we should not have separate code for single cursor and for rect selection (which probably means multiple cursors). The code for multiple cursors should be able to handle single cursor, because it's basically multiple cursors with the size of 1. I want to avoid having the same code (and related bugs) twice in the code and also I don't want the uncertainty which code will run when I call some method setting/getting text.

I think most of the problems/ideas described below will simply disappear once you start treating the rect selection as multiple single cursors (multiple single selections). The act of rect selecting will then be just a special way to place multiple cursors (multiple selections).

The existing code where feeding in multiple cursors/selections does not make sense would get only the last cursor/selection.

To implement this, cursorOffset, selctionStart and selectionEnd fields in the JEditTextArea would have to go from int to int[]

Some things which I think should be fixed/handled:

  • Ctrl interferes with Ctrl+Click (Go to definition), maybe we should consider changing it to Alt and I would love to see mouse wheel drag to be added too (I still need to polish Ctrl+Click to ignore when mouse is dragged)
  • When I press enter while in rect select, I'm left only with a single cursor
  • Undo does not group individual letters into undo group, undos letter by letter (not a big deal, just annoying)
  • Undo leaves you with a single cursor, which is placed at different location than where the edits occurred (confusing and you can't continue typing multiline after that)
  • If the first line of the selection is ending at the offset where the multiline cursor is, delete key does nothing - it should keep deleting chars on lines where there are some chars to the right
  • Starting dragging vertically to the right of the line endings should place cursors at the ends of lines; now works only when I start dragging on a long line (scenario: you want to collapse stuff from more lines onto single line)
  • Check the way how the rect selection interacts with the existing code - what do I get in selectionStart and selectionStop? What do I get when I call getSelection?

Things I would expect from "perfect" implementation

  • Holding Alt and clicking should place multiple cursors on all places where you click
  • Deleting line ending should pop the following line at the end and leave you with two cursors on that line (scenario: collapsing multiple line onto one line and adding commas in between)
  • Cursors should not be placed on shorter lines when selection is at least one character wide (scenario: changing int to float in front of bunch of variables separated by empty lines)

Let's ask @benfry if he thinks I'm being reasonable here and this is something we want before starting working on this (if you would still like to work on it of course) :)

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 9, 2016

Member

For something this invasive, I'd rather see the effort put into replacing JEditSyntax with something less buggy, more modern, more full-featured (that has features like this already). Or even, better integration between Processing and other full-featured editors that people like to use (Sublime, IntelliJ, etc). Both seem like time better spent priority-wise.

In the meantime, are there things that need to be disabled so that we don't have half-working rectangular selection in the current code base?

Member

benfry commented May 9, 2016

For something this invasive, I'd rather see the effort put into replacing JEditSyntax with something less buggy, more modern, more full-featured (that has features like this already). Or even, better integration between Processing and other full-featured editors that people like to use (Sublime, IntelliJ, etc). Both seem like time better spent priority-wise.

In the meantime, are there things that need to be disabled so that we don't have half-working rectangular selection in the current code base?

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 9, 2016

Contributor

In terms of invasiveness... I think most of it will be just using the same code as now, but looping through multiple cursors instead of one. Rest of my complaints will be handled by improving the method which handles the act of rect selection and by fixing Undo (which I will be doing anyway).

Though you are probably right that spending those hours on the integration with other editors would directly benefit more people. It looks like many people prefer to use different editors.

Contributor

JakubValtar commented May 9, 2016

In terms of invasiveness... I think most of it will be just using the same code as now, but looping through multiple cursors instead of one. Rest of my complaints will be handled by improving the method which handles the act of rect selection and by fixing Undo (which I will be doing anyway).

Though you are probably right that spending those hours on the integration with other editors would directly benefit more people. It looks like many people prefer to use different editors.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry May 9, 2016

Member

The switch from int to int[] for those fields is much worse than you might expect. It would affect most code inside Editor, and break most Tools.

(Closing this, per discussion)

Member

benfry commented May 9, 2016

The switch from int to int[] for those fields is much worse than you might expect. It would affect most code inside Editor, and break most Tools.

(Closing this, per discussion)

@benfry benfry closed this May 9, 2016

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar May 9, 2016

Contributor

@benfry Fair enough.

@GKFX Can I ask you to remove the rect selection code which is currently present in the codebase? You probably know better where it is. Otherwise let me know and I'll do it. Sorry that we are not going to use your code, but supporting this feature would likely create more headaches for us and take more hours to maintain than we can afford.

Contributor

JakubValtar commented May 9, 2016

@benfry Fair enough.

@GKFX Can I ask you to remove the rect selection code which is currently present in the codebase? You probably know better where it is. Otherwise let me know and I'll do it. Sorry that we are not going to use your code, but supporting this feature would likely create more headaches for us and take more hours to maintain than we can afford.

@GKFX GKFX deleted the GKFX:block-select branch Aug 29, 2017

@GKFX GKFX restored the GKFX:block-select branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment