Fork me on GitHub
#parinfer
<
2017-07-09
>
cfleming07:07:55

@shaunlebron I’m definitely interested in updating the JVM version.

rgdelato19:07:39

Also currently playing with a proof-of-concept to get cursorDx working in Atom: https://github.com/rgdelato/atom-parinfer/commit/ab2c8c643900de8c8fb309393aa4d5b6d275d027 ...I definitely need to test it a bit more, but it looks like something in this ballpark could potentially work? ("It works on my machine." :P)

shaunlebron19:07:38

@rgdelato ha! i was just trying this today, lemme read what you have

shaunlebron19:07:28

this looks great

shaunlebron19:07:58

I was looking at buffer.onDidChange, but decided to use editor.onDidChangeText like you did

shaunlebron20:07:26

@rgdelato I just tried it out, works well 👍

shaunlebron20:07:51

i pushed a small cursorDx fix for indent mode last night - 2.5.1

rgdelato20:07:00

Good to hear that it worked for you as well! When I was very first trying it out, I was getting an infinite loop of onChange -> run Parinfer -> setText -> setText triggers onChange -> etc ...and honestly, I'm not quite sure what's preventing that infinite loop from happening in the current version of the code

shaunlebron20:07:37

since the apply-parinfer will set the full text of one or more top-level forms, I was going to store that and check on future change events for reactions to that value and break out

rgdelato20:07:00

Doing that might still be a good idea.

shaunlebron20:07:34

i’ll console.log to see what events are chaining

shaunlebron20:07:37

might want to use ocall for these functions so we don’t need externs right?

shaunlebron20:07:09

oh, and we can’t loop through the changes in a forEach, because the coordinates change on every iteration

shaunlebron20:07:00

i think we should only respond to the first change for now, which handles the single cursor case

shaunlebron20:07:19

i’ve only been able to reproduce multiple changes using multiple cursors

rgdelato20:07:46

same for me, unless I'm using multiple cursors, I've only gotten 0 or 1 changes

rgdelato20:07:28

also, it looks like there's only one cljsbuild config and it has :optimizations set to :simple

shaunlebron20:07:43

ah, nevermind then

shaunlebron20:07:36

@rgdelato it’s not firing a second onChangeText when the callback changes the text synchronously

shaunlebron20:07:56

you were probably getting the infinite loop when debouncing

shaunlebron20:07:18

minor edge case I can fix—cursorDx doesn’t shift comment lines

cfleming21:07:56

@rgdelato @shaunlebron Looks like Atom is assuming that the caret is at the end of the changed range - I don’t think that’s always true.

cfleming21:07:17

old-range-x (some-> change .-oldRange .-end .-column)
new-range-x (some-> change .-newRange .-end .-column)

cfleming21:07:05

Is the cursorDx value really a delta between the range lengths, or should it actually represent the caret movement?

cfleming21:07:24

(example of when that wouldn’t be true: I have the caret at the end of an identifier. I use shift-left arrow to select to the start of the identifier, so the identifier is selected but the caret is at the beginning. Then I paste some replacement text. cursorDx should be the length of the replacement text, not the difference between the old and new text).

cfleming22:07:40

@shaunlebron I saw your github comment, thanks. I still don’t quite understand what their oldRange represents, i.e. I don’t see how their code works. I’ll have to dig into the atom code to see if I can figure out how they’re calculating that.

cfleming22:07:57

Unless their changes are ordered by text offset rather than the order in which they occurred.

cfleming22:07:12

Which doesn’t really make sense.

shaunlebron23:07:26

@cfleming: cursorDx might be misdescribed then. We only want cursorDx to be 2. If it were 4, the second line would not shift correctly

cfleming23:07:59

Right, so it’s really the difference between the old and new lengths.

shaunlebron23:07:27

I suppose it doesn’t matter where the cursor is in the initial selection

cfleming23:07:06

Hmm, thinking about it, that’s not right either. It’s the difference between the final column of the old and new. If you paste multi-line code you can’t use the lengths.

shaunlebron23:07:07

it should behave the same no matter what, since the cursor should be after the replacement selection

cfleming23:07:39

@shaunlebron So with this new behaviour, is it your expectation that Paren mode is officially obsolete?

shaunlebron23:07:20

it certainly feels obsolete so far

shaunlebron23:07:49

let me try with multiline code

shaunlebron23:07:59

yeah! I think ryan got it right by just subtracting the end column of the old from the new

cfleming23:07:08

Right - I guess what you really want to know is how far following code should be indented/outdented

cfleming23:07:20

This is good news for me, because I don’t have access to the actual caret when calculating this.

shaunlebron23:07:36

yeah, and rather than specifying the full change, we use the cursor point as reference for the amount of shift that should happen to the expressions starting after it

cfleming23:07:26

I’ll still need to think about the case of multiple changes, and try to work out how to apply those.

cfleming23:07:27

I guess that the cursorDx applies to all code following a particular change inside its enclosing form, right?

cfleming23:07:09

Does atom have a “wrap selection in parens” action? What happens if you select the if and then wrap it?

cfleming23:07:22

In that case, is parinfer run once or twice?

shaunlebron23:07:48

i’d just describe it as shifting all lines inside the expressions with open-parens after the cursor

cfleming23:07:27

Well, it’s after the change, right? The cursor doesn’t actually matter.

shaunlebron23:07:49

right, we’re using cursorDx because I haven’t finished support for the change option yet

cfleming23:07:02

I’m thinking about the case where there are two changes (i.e. wrapping in parens). In that case, it seems like the two dx’es should be summed after the second change.

shaunlebron23:07:04

which will help with inline paren inference later

shaunlebron23:07:35

i can install paredit plugin to see in atom

cfleming23:07:07

Actually, is this new version available in the online editor yet? Or is there a test version somewhere?

shaunlebron23:07:25

you can clone ryan’s fork: [email protected]:rgdelato/atom-parinfer.git

shaunlebron23:07:43

and git checkout cursor-dx-poc

rgdelato23:07:50

wrapping something in parens seems to only create one change in onDidTextChange

shaunlebron23:07:15

using paredit?

cfleming23:07:21

@rgdelato Right, so you receive an array of changes, right?

cfleming23:07:33

Do you run parinfer after each one?

rgdelato23:07:44

I just highlighted a block and pressed left paren (shift + 9), seems that wrapping behavior is built into Atom?

cfleming23:07:32

Ok, so the wrapping is creating the wrapped text and replacing the selection with it, looks like.

cfleming23:07:44

Rather than just inserting the two parens individually.

cfleming23:07:36

Is there another way we could create a transaction with multiple changes?

cfleming23:07:50

Can you rename elements in Atom?

rgdelato23:07:12

I've only been able to make it occur with multiple cursors/selections. Shaun was saying earlier that we should only take the first change in the changes array

cfleming23:07:36

I won’t be able to do that in Cursive, at least.

rgdelato23:07:48

> oh, and we can’t loop through the changes in a forEach, because the coordinates change on every iteration > [1:19] > i think we should only respond to the first change for now, which handles the single cursor case

cfleming23:07:20

There you go - looks like the paredit support works like the Cursive one would.

cfleming23:07:47

I think you’ll have to handle that.

shaunlebron23:07:10

the renaming case will be problematic too