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

Paredit and highlight fixes and improvements. #456

Merged
merged 36 commits into from
Nov 14, 2019
Merged

Paredit and highlight fixes and improvements. #456

merged 36 commits into from
Nov 14, 2019

Conversation

cfehse
Copy link
Contributor

@cfehse cfehse commented Nov 7, 2019

What has Changed?

  • Changed the paredit.open() method to wrap around the current selection ([2.0] Paredit inconsistencies #170 ).
  • Keep the selection after wrap on open() and wrapSexpr().
  • Changed insertString() to use changeRange() directly.
  • Added a dblclick handler to the webview to execute growSelection() on mouse double click starting at the current position of the mouse pointer. This allows the selection of words with a double click like in the editor.

calva-paredit-dblclick-repl-window

Fixes #170 #417 #403 #458

My Calva PR Checklist

I have:

  • Read How to Contribute.
  • Made sure I am directing this pull request at the dev branch. (Or have specific reasons to target some other branch.)
  • Made sure I am changed the default PR base branch, so that it is not master. (Sorry for the nagging.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.) You'll find the artifacts by clicking Show all checks in the CI section of the PR page, and then Details on the ci/circleci: build test. (For now you'll need to opt in to the CircleCI New Experience UI to see the Artifacts tab, because bug.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • Referenced the issue I am fixing/addressing in a commit message for the pull request.
    • If I am fixing the issue, I have used GitHub's fixes/closes syntax
    • If I am fixing just part of the issue, I have just referenced it w/o any of the "fixes” keywords.
  • Created the issue I am fixing/addressing, if it was not present.
  • Updated the [Unreleased] entry in CHANGELOG.md, linking the issue(s) that the PR is addressing.

The Calva Team PR Checklist:

Before merging we (at least one of us) have:

  • Made sure the PR is directed at the dev branch (unless reasons).
  • Read the source changes.
  • Given feedback and guidance on source changes, if needed. (Please consider noting extra nice stuff as well.)
  • Tested the VSIX built from the PR (well, if this is a PR that changes the source code.)
    • Tested the particular change
    • Figured if the change might have some side effects and tested those as well.
    • Smoke tested the extension as such.
  • If need be, had a chat within the team about particular changes.

Ping @PEZ, @kstehn, @bpringe

@cfehse cfehse changed the title Implemented paredit open to wrap around selection. paredit and highlight fixes and improvements. Nov 9, 2019
@cfehse cfehse changed the title paredit and highlight fixes and improvements. Paredit and highlight fixes and improvements. Nov 9, 2019
@PEZ
Copy link
Collaborator

PEZ commented Nov 9, 2019

That double-click is great! We should add it to the editor as well. (Not in this PR, though. 😄 )

@PEZ
Copy link
Collaborator

PEZ commented Nov 9, 2019

Double-clicking inside a string should probably not use grow-selection. It makes it hard to select words in the string.

@PEZ
Copy link
Collaborator

PEZ commented Nov 9, 2019

Undoing wrap selection deletes the selection. I think this might be an old bug.

@PEZ
Copy link
Collaborator

PEZ commented Nov 9, 2019

Those are the only things I find when testing. The rest works wonderfully as advertised.

@cfehse
Copy link
Contributor Author

cfehse commented Nov 9, 2019

Double-clicking inside a string should probably not use grow-selection. It makes it hard to select words in the string.

You mean clicking in something like this "This is a documentation string"?

@cfehse
Copy link
Contributor Author

cfehse commented Nov 9, 2019

Undoing wrap selection deletes the selection. I think this might be an old bug.

I take a look at this as well.

@PEZ
Copy link
Collaborator

PEZ commented Nov 10, 2019

You mean clicking in something like this "This is a documentation string"?

Yes. It is both good and bad that the whole string is selected, I guess. But it ”feels” weird.

@cfehse
Copy link
Contributor Author

cfehse commented Nov 10, 2019

Undoing wrap selection deletes the selection. I think this might be an old bug.

I can not reproduce this for the wrap around commands. The behavior with ctrl+z does not delete the selection but removes the inserted backets/parens/curlies. What I noticed that the shortcuts for paredit.wrapAroundSquare and paredit.wrapAroundCurly can not be applied on a german keyboard.

@cfehse
Copy link
Contributor Author

cfehse commented Nov 10, 2019

@PEZ, @kstehn, @bpringe

I changed the double click behavior in the webview to select the current word in a string.

/**
* Indicates if the current token is inside a string (e.g. a documentation string)
*/
isInString() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought we already had a method for checking if we're inside a string? Or maybe that is only while parsing... But even if it is, maybe add that fact to the token?

Copy link
Contributor Author

@cfehse cfehse Nov 10, 2019

Choose a reason for hiding this comment

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

If so I didn't found it and the decision if in a string may depend on the next and previous token so I think this method is correctly placed in the token cursor because only the cursor has the whole picture of the model.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was remembering correctly 😄

withinString() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey cool! But I think the code is not sufficient. I will test it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

What seems a bit odd to me is that double-clicking words work everywhere on in the REPL window, except the prompt. Is there a reason for that, you think? (I'm talking about the released Calva.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's because outside the line model the default behavior of the double click is in place. We override the default for mouse events to set the blinking cursor on the right place. My new code takes care that this default behavior is preserved outside the line model.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My new code takes care that this default behavior is preserved outside the line model.

Yes. I saw that.

@PEZ
Copy link
Collaborator

PEZ commented Nov 11, 2019

I can not reproduce this for the wrap around commands.

I now tried this a bit more and realise we have quite a few different behaviours. If I use the Wrap around ()` command on a left->right selection, the right thing is selected and I can undo the wrap around w/o undoing something else as well. However:

  • If the selection is right->left (either by dragging the mouse in that direction, or by holding down the shift-key and expanding the selection with key-left) then the wrap around command does really funny things, and undoing it doesn't bring the cursor back where it was before the wrap.
  • If I wrap around parens around a left->right selection by typing an opening bracket, like (, the selection is wrapped and keep being correctly selected, but undoing also deletes the selection. (This is probably what I had done when reporting on the behaviour).
  • The really crazy things start to happen if I have a right-left selection and wrap using an opening bracket. Then:
    1. the selection is trippled!
    2. the middle clone is wrapped
    3. the leftmost clone is selected, offset by one character
    4. undoing here:
      1. removes the parens around the middle clone of the original selection
      2. keeps all three clones
      3. undoing here:
        1. creates yet another clone of the selection 😂
        2. selects the leftmost clone
          1. you might think this is the end of the madness, but you'd be wrong!
          2. undoing now:
            1. does a bit of different things everytime I try, including creating new content that is a mix of the current content and some other entry in the repl history. I haven't found a pattern to it.
            2. keeps the selection range where the original selection was.

OK. So complicated to follow, here's a gif:

repl-prompt-wrap-issues

Crazy, huh? 🤔

@PEZ
Copy link
Collaborator

PEZ commented Nov 11, 2019

Much less crazy now! 😄

I think a new issue arrived. I can't move the cursor right with key-right. It instead selects the character to the right w/o moving the cursor, and then the cursor won't move either left or right.

Also:

  • Having the cursor adjacent to the left of a form and then using the Wrap around commands places the brackets adjacent to the right of the form.
  • Having the cursor adjacent to the left of a form and then typing an opening bracket, places the brackets adjacent to the left of the form.
  • Typing a bracket when there is a selection, wraps it correctly, but undo will delete the selection and misplace the cursor
    • undoing again gives you the selection back and the cursor position correct.
    • (yes, I've already reported this, but including for completeness).

no selection to restore previous behavior.
@cfehse
Copy link
Contributor Author

cfehse commented Nov 11, 2019

I think a new issue arrived. I can't move the cursor right with key-right. It instead selects the character to the right w/o moving the cursor, and then the cursor won't move either left or right.

Yes the cursor movement depends on the "direction" of the selection. I fixed that.

Having the cursor adjacent to the left of a form and then using the Wrap around commands places the brackets adjacent to the right of the form.

Yes that was wrong.

Having the cursor adjacent to the left of a form and then typing an opening bracket, places the brackets adjacent to the left of the form.

I am really not sure, what you doing. I do this:

problem-one

Typing a bracket when there is a selection, wraps it correctly, but undo will delete the selection and misplace the cursor

And the same for this one: I am doing this - selecting both directions and execute open and wrapSexpr for each type of selection followed by undo (Ctrl+Z):

problem-two

@PEZ
Copy link
Collaborator

PEZ commented Nov 11, 2019

I am really not sure, what you doing

I type the character (. So, on my Swedish keyboard that is shift+8.

@cfehse
Copy link
Contributor Author

cfehse commented Nov 11, 2019

type the character (. So, on my Swedish keyboard that is shift+8.

Okay then we are doing the same - it is Shift+8 on the german keyboard as well.

Having the cursor adjacent to the left of a form and then typing an opening bracket, places the brackets adjacent to the left of the form.

Typing a bracket when there is a selection, wraps it correctly, but undo will delete the selection and misplace the cursor

Is this still present for you with the latest commits?

@PEZ
Copy link
Collaborator

PEZ commented Nov 11, 2019

Yes. With some few variations on what happens. Here's a session where select something, then type (, then undo several times. And (this I now noticed) redo many times:

wrap-typing-bracket-issue

@cfehse
Copy link
Contributor Author

cfehse commented Nov 11, 2019

Did you typed that repeat just before selecting and wrap it with paredit.open().? Do you edit in strict mode or not.

On my system the changes in the model are played correctly back in order (Windows and Linux).

@PEZ
Copy link
Collaborator

PEZ commented Nov 11, 2019

It doesn't matter if I am in strict mode or not. Or if I am doing this on something in the repl history or something I have just typed.

The redo bug is not new, btw. I just tried it in the released Calva. I'll file an issue about it.

@PEZ
Copy link
Collaborator

PEZ commented Nov 11, 2019

Here's that issue. #462

In this PR I still have this issue:

Typing a bracket when there is a selection, wraps it correctly, but undo will delete the selection and misplace the cursor

Too bad you can't reproduce it! How about @kstehn and @bpringe ?

@cfehse
Copy link
Contributor Author

cfehse commented Nov 11, 2019

On my systems the undo/redo works like a charm in both directions. Looking further into this tomorrow.

@cfehse
Copy link
Contributor Author

cfehse commented Nov 11, 2019

@PEZ By the way is there a specific reason that the redo command in the REPL is Ctrl+Shift+Z and not Ctrl+Y like in the editor?

@PEZ
Copy link
Collaborator

PEZ commented Nov 11, 2019

Maybe because it is ctrl+shift+z in the editor in any sane setup? 😄 No, we should probably try to figure out what the setting is in the editor and use the same in at the repl prompt.

On my machine redo is cmd+shitft+z, everywhere, and I think it might be quite common with that shift ”reverses” the meaning of ctrl/cmd+z.

@cfehse
Copy link
Contributor Author

cfehse commented Nov 11, 2019

Mayby. gg On my stock vscode installation on Windows redo is Ctrl+Y. Was just a question.

@cfehse
Copy link
Contributor Author

cfehse commented Nov 14, 2019

@PEZ @bpringe

I think this PR is ready for merging now.

@PEZ
Copy link
Collaborator

PEZ commented Nov 14, 2019

Yes. And I have reached the limit to my testing of this since things are crazy on macOS anyway. Let's merge and we will find out. Especically together with my lexing fix things will work pretty much better than they do today with this PR.

After that, we're almost ready to take on Paredit in the editor. Haha.

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.

3 participants