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

Try fixing lsp display error #12

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Try fixing lsp display error #12

wants to merge 1 commit into from

Conversation

jcs090218
Copy link
Member

as title.

@aaronjensen Can you test this branch and see if this works for you!?

I have changed to use pre/post command hook instead of before/after change hook. And this kind of solve the issue as I mentioned it in #11.

@jcs090218
Copy link
Member Author

Sorry, I think this patch still has error in there. Let me pause and think for a better method to resolve this issue! Sorry to ping you on this!

@aaronjensen
Copy link
Contributor

I don't know that it's better to use the pre-command hook because it will fire off much more often (with every keyboard navigation, for example) which can increase latency if the pre-command is slow at all.

Did you have a problem on the other branch still? Can you show me a repro?

@jcs090218
Copy link
Member Author

I think when you have one before-change-functions and post-command-hook then auto-rename-tag--record-prev-word wouldn't get the correct result. That is why I switched over to pre-command-hook which will guarantee you get the correct result (I guess). But that causes more problems which make this patch doesn't work well.

And I am not sure why this happens on my machine, I think it is because I have my own kill-word related functions. And lsp-mode limited other package to not to use before/after change functions is kind of frustrating. 😕 Especially, when we only want the hook to be specific on changes and not the command.

Anyway, I just need auto-rename-tag--record-prev-word (in before-change-functions) to match to correct data so it doesn't mess up it's behaviour. 😩

@aaronjensen
Copy link
Contributor

Does it fail on everything for you? I wonder if you could provide an example code that's failing and instructions.

In terms of using before-change-function with post-command-hook, since auto-rename-tag--record-prev-word is set in before-change-function, it shouldn't make any difference where it's used (in post command or after change).

FWIW, I don't think it's just lsp-mode imposing this limitation, I think it's probably generally not a good idea to change the buffer in an after-change hook as it prevents any after-change hook from being notified of the actual change. Imagine if an after-change hook from another package changed a tag, that could prevent this package from doing its work (of course, this package relies on the point's position at the beginning of a change rather than the actual beginning and end of the change, so there are other issues there).

@jcs090218
Copy link
Member Author

Does it fail on everything for you? I wonder if you could provide an example code that's failing and instructions.

Some fail and some don't. I think example code would not help because it fails in general behaviour. 😕

In terms of using before-change-function with post-command-hook, since auto-rename-tag--record-prev-word is set in before-change-function, it shouldn't make any difference where it's used (in post command or after change).

That was my first thought too! But in fact, when I tested before/after change functions have run multiple times and some time it can run after post command hook. I think before/after hook will catch all the addition/deletion between commands (pre/post) which I think is more complete because then you can literally design any command with a correct output. post command wouldn't do the trick unless all users don't config their commands (sorry, but I do!). And see here.

FWIW, I don't think it's just lsp-mode imposing this limitation, I think it's probably generally not a good idea to change the buffer in an after-change hook as it prevents any after-change hook from being notified of the actual change. Imagine if an after-change hook from another package changed a tag, that could prevent this package from doing its work (of course, this package relies on the point's position at the beginning of a change rather than the actual beginning and end of the change, so there are other issues there).

hmmm, Cool! I didn't notice this until now. But as I said, if we don't use before/after change functions then the control will be very limited. Of course, with the package that doesn't helps you modified buffer (or catch changes) wouldn't ever need to run the these two hooks! Otherwise, with just pre/post command would be very hard to know what things are going on? And yet this is why I need to record each (buffer-string) for each command to prevent unnecessary calls (this patch), because weather you modified the buffer or not, it eventually get executed for each command!

@aaronjensen
Copy link
Contributor

post command wouldn't do the trick unless all users don't config their commands (sorry, but I do!)

What do you mean by this? I'm not sure what configing commands means.

And see here.

What am I looking for on the docs page?

@aaronjensen
Copy link
Contributor

I just tested this out, it does not capture the change if I insert a letter and then hit backspace to delete what I just inserted. The matching tag only gets the newly inserted letter and does not get the deletion.

@jcs090218
Copy link
Member Author

What do you mean by this? I'm not sure what configing commands means.

Oh, I mean I have a lot configuration while I am editing buffer (coding).

What am I looking for on the docs page?

I have the link to just inform the differences between change-functions and command-hook.

I just tested this out, it does not capture the change if I insert a letter and then hit backspace to delete what I just inserted. The matching tag only gets the newly inserted letter and does not get the deletion.

Let me explain it this way, if I have a command that delete 5 letters at a time. command-hook wouldn't notice unless you compare the buffer in pre and post command hook. Vice versa, I think change-functions would know the addition/deletion going on in the COMMAND like this. Overall, is my implementation isn't command base because it doesn't make sense putting into command-hook. 😩 And that is why change-functions kind of hook would ran multiple times because it all happens in one command... ? 😕 This is my assumption.

@aaronjensen
Copy link
Contributor

Let me explain it this way, if I have a command that delete 5 letters at a time. command-hook wouldn't notice unless you compare the buffer in pre and post command hook.

Okay, so I think you're saying that if you have a command that makes 5 separate changes you'd see:

pre
before
after
before
after
before
after
before
after
before
after
post

And in that case, just taking the last before's idea of what the pre tag is won't work. That makes sense. So a compound command (that issues multiple separate changes) would be broken.

Okay, so I think that leads us to either the method I switched to in #11, or a method of remembering what changes need to be made and then making them all at once in post. So:

pre
before: tag is foo
after: tag is foob (remember to add a "b" to closing tag)
before: tag is foob
after: tag is fooba (remember to add a "a" to closing tag)
before: tag is r
after: tag is foobar (remember to add a "r" to closing tag)
post: add "bar" to closing tag

I don't know how easy that would be to implement, and it seems like there would be some edge cases, but that may be the most robust method.

@jcs090218 jcs090218 changed the title Try fixing lsp display error. Try fixing lsp display error Apr 18, 2021
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.

None yet

2 participants