Skip to content

Keep instance lenses stable even if parsed results are unavailable #3545

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

Merged
merged 5 commits into from
Apr 8, 2023

Conversation

July541
Copy link
Collaborator

@July541 July541 commented Apr 1, 2023

Not function signature lenses only(#3541), instance signatures have the same issue.

This pr uses staled results to prevent flicker.

I take hls-class-plugin considered first just because I'm more familiar with this than familiar with function type lenses😉

stale

@July541 July541 requested a review from Ailrun as a code owner April 1, 2023 10:18
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

Seems like there's a general principle: code lenses should always use useWithStale, since otherwise they flicker. Doesn't really matter for other stuff so much. It would be nice to write that down somewhere, not sure where.

$ liftIO
$ runAction "classplugin.insertPragmaIfNotPresent.GetParsedModuleWithComments" state
$ use GetParsedModuleWithComments nfp
$ useWithStale GetParsedModuleWithComments nfp
Copy link
Collaborator

Choose a reason for hiding this comment

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

and here

Copy link
Collaborator

Choose a reason for hiding this comment

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

Interestingly, this seems like somewhere we shouldn't do it. We really want the up-to-date module to compute an edit! But because we return the edit as part of the code lens, we have to. So the thing to do in the long run would be to implment codeLens/resolve so only the resolution method would need this. Worth a comment!

Also relevant for #3536

@July541
Copy link
Collaborator Author

July541 commented Apr 5, 2023

Should be pointing out that we have three lens behaviors now:

  1. import lens
  2. top-level signature lens
  3. class instance lens

import lens has been using useWithStale, and I'm applying the substitution for the other two.

That means we'd have similar comments for every lens provider :(

@michaelpj
Copy link
Collaborator

That means we'd have similar comments for every lens provider :(

I think it's an important comment in all those places, unfortunately. Unless we want to adopt the GHC Note convention and just write a Note once and reference it from all of them.

@July541
Copy link
Collaborator Author

July541 commented Apr 7, 2023

So the thing to do in the long run would be to implment codeLens/resolve so only the resolution method would need this. Worth a comment!

@michaelpj I'm a little confused, do you mean we compute lenses by fresh type checked result, and resolve it with the staled result?

And, what's the meaning of resolve, does it mean apply the lens or refresh the lens, or anything else?

@July541 July541 requested a review from pepeiborra as a code owner April 7, 2023 15:20
@July541 July541 force-pushed the stale-class-lens branch from 9bee194 to bbc365e Compare April 7, 2023 15:22
@michaelpj
Copy link
Collaborator

I'm a little confused, do you mean we compute lenses by fresh type checked result, and resolve it with the staled result?

Other way around 😅 I think that's reasonable? It means that when you click on the lens it might be a bit out of date, but then you'll get the right then when its inserted.

And, what's the meaning of resolve, does it mean apply the lens or refresh the lens, or anything else?

The codeLens/resolve method lets you fill in some information in the lens result later. In particular, you can fill in the edit later! So you can quickly/inaccurately produce the initial lens results, and then when someone clicks on it fill in the full correct edit.

But this is future work anyway!

@michaelpj michaelpj merged commit 30bcab5 into haskell:master Apr 8, 2023
@July541 July541 deleted the stale-class-lens branch April 9, 2023 14:05
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.

2 participants