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

"LineLens" #510

Merged
merged 1 commit into from Sep 4, 2017

Conversation

Projects
None yet
4 participants
@vbfox
Contributor

vbfox commented Aug 26, 2017

Mostly opening the PR to get some feedback :) The name is not very original ("On the line CodeLens" -> "LineLens") i'm open to suggestion if it's not good

The concept is to show type signatures at the end of the line like the vscode debugger is showing variable values (See Microsoft/vscode#16129)

linelens1

How to test:

For now it's enabled by default when code lenses are disabled :

{
    "editor.codeLens": false,
    "FSharp.lineLens.enabled": "replaceCodeLens",
    "FSharp.lineLens.color": "theme(editorCodeLens.foreground)",
    "FSharp.lineLens.prefix": "  // ",
    "FSharp.lineLens.refreshAfter": 1000,
    "workbench.colorCustomizations": {
        "editorCodeLens.foreground": "#4C4B4B"
    }
}

A few things I need to do / know for this PR :

  • Dispose the decoration type when disabled instead of the current hack
  • Cleanup line in CodeLens.fs
  • Remove the hack to disable npm update during build. I don't think update should run during build but I can open another PR/issue for that.
  • Wait for ionide/ionide-vscode-helpers#11 & update paket lock
  • Disable them by default until we're sure it's working well ?
  • Determine if I need to call parse on FSAC or can wait for a parse somehow

Regarding the last one it's mostly a question for @Krzysztof-Cieslak I think, i'm currently calling LanguageService.parse manually but isn't it done automatically somehow when file change ? (CodeLens seem to get the change directly) I added this step as otherwise when a file is opened for the first time LanguageService.declarations return an error that the file is not yet parsed.

Is it the correct way or will I trigger extra parsing of the opened file ? What I would like is an event from FSAC telling me that the file has changed and been parsed but I didn't find one.

@vbfox vbfox changed the title from [WIP] "LineLens" to "LineLens" Aug 27, 2017

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Aug 29, 2017

Collaborator

Love the idea, awesome @vbfox !

Because doenst move the layout adding lines like codelens (this just add characters to existing lines), can be really nice to trigger with a keyboard shortcut/command.

Collaborator

enricosada commented Aug 29, 2017

Love the idea, awesome @vbfox !

Because doenst move the layout adding lines like codelens (this just add characters to existing lines), can be really nice to trigger with a keyboard shortcut/command.

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Aug 29, 2017

Contributor

@enricosada On the trigger with a keyboard shortcut I wondered about that myself.

Maybe a command that would also be present on the top right of the editor (Along with open change & split) if doable. Anyway it'll be for latter for now i'd like to lend it as a CodeLens replacement :)

Contributor

vbfox commented Aug 29, 2017

@enricosada On the trigger with a keyboard shortcut I wondered about that myself.

Maybe a command that would also be present on the top right of the editor (Along with open change & split) if doable. Anyway it'll be for latter for now i'd like to lend it as a CodeLens replacement :)

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Aug 30, 2017

Contributor

Ok new take, mostly a rewrite of the core at this point.

A few points;

  • It uses the parse event from Error.fs but I Inverted the logic of the event for LineLenses compared to CodeLens. Instead of Error.fs calling into LineLens, it expose a vscode event and LineLens subscribe to it.
  • I removed all handling of typing in the current line, it was buggy and I discovered that most of my problems were due to a bad handling of the fact that when the TextDocument version evolve it invalidate all ranges, and anything computed on the previous version should be dropped.
    The fix can be seen in how updateDecorationsForDocument check the document version essentially after each async operation and bail out if it changed.
    It also massively simplify the code, so not a bad thing 😉
  • I now cache text editors where I set the decorations to never set them twice for the same editor/version combination
  • When the editors are split it can mean a new TextEditor instance (Re-opening an already opened document a second time) , it's now handled and the decorations are correctly set
  • In the same vein the lenses are now correctly set when a document is in more than 1 panes at a time and being edited
  • There is now "time before update" anymore it follow the 1s settimeout of Error.fs
  • I disabled LineLens by default (Instead of the previous replaceCodeLens mode) so that it can be merged and tested by a few guinea pigs nice users before a more general release.

Oh I activated CodeLens to see if I didn't break them and it seem I forgot how annoying hey are, let's say that I prefer my little hack :)

Contributor

vbfox commented Aug 30, 2017

Ok new take, mostly a rewrite of the core at this point.

A few points;

  • It uses the parse event from Error.fs but I Inverted the logic of the event for LineLenses compared to CodeLens. Instead of Error.fs calling into LineLens, it expose a vscode event and LineLens subscribe to it.
  • I removed all handling of typing in the current line, it was buggy and I discovered that most of my problems were due to a bad handling of the fact that when the TextDocument version evolve it invalidate all ranges, and anything computed on the previous version should be dropped.
    The fix can be seen in how updateDecorationsForDocument check the document version essentially after each async operation and bail out if it changed.
    It also massively simplify the code, so not a bad thing 😉
  • I now cache text editors where I set the decorations to never set them twice for the same editor/version combination
  • When the editors are split it can mean a new TextEditor instance (Re-opening an already opened document a second time) , it's now handled and the decorations are correctly set
  • In the same vein the lenses are now correctly set when a document is in more than 1 panes at a time and being edited
  • There is now "time before update" anymore it follow the 1s settimeout of Error.fs
  • I disabled LineLens by default (Instead of the previous replaceCodeLens mode) so that it can be merged and tested by a few guinea pigs nice users before a more general release.

Oh I activated CodeLens to see if I didn't break them and it seem I forgot how annoying hey are, let's say that I prefer my little hack :)

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Aug 30, 2017

Contributor

Ah also 2 small notes / gotchas:

  • replaceCodeLens doesn't replace CodeLens if it's globally enabled but disabled only for FSharp language. I tried but can't seem to get the setting. Maybe I should ask vscode team
  • sometimes LineLenses produce TextEditor disposed errors. It's not really a problem and caused by the fact that "SetDecoration" is actually asynchronous and that the editor can have disappeared between when we call it and when it really execute. Does not affect any functionality.
    2017-08-30 23_07_28-settings json ionide-fs visual studio code
Contributor

vbfox commented Aug 30, 2017

Ah also 2 small notes / gotchas:

  • replaceCodeLens doesn't replace CodeLens if it's globally enabled but disabled only for FSharp language. I tried but can't seem to get the setting. Maybe I should ask vscode team
  • sometimes LineLenses produce TextEditor disposed errors. It's not really a problem and caused by the fact that "SetDecoration" is actually asynchronous and that the editor can have disappeared between when we call it and when it really execute. Does not affect any functionality.
    2017-08-30 23_07_28-settings json ionide-fs visual studio code
@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Sep 1, 2017

Contributor

Turns out I found how to get language specific settings. It's a little manual / weird but works :)

Contributor

vbfox commented Sep 1, 2017

Turns out I found how to get language specific settings. It's a little manual / weird but works :)

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Sep 3, 2017

Contributor

I like the IDEA approach more.

Contributor

vasily-kirichenko commented Sep 3, 2017

I like the IDEA approach more.

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Sep 3, 2017

Contributor

@vasily-kirichenko Any more constructive comments ? I don't use IDEA so don't know about their approach to type signature display or how it differ from any of the options available today in ionide...

Contributor

vbfox commented Sep 3, 2017

@vasily-kirichenko Any more constructive comments ? I don't use IDEA so don't know about their approach to type signature display or how it differ from any of the options available today in ionide...

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Sep 3, 2017

Contributor

@vbfox Sorry that I was unclear. IDEA shows inferred types and parameter names like this:

image

Kotlin cannot infer parameter types, so the hint makes sense for return types and local vars only.

Contributor

vasily-kirichenko commented Sep 3, 2017

@vbfox Sorry that I was unclear. IDEA shows inferred types and parameter names like this:

image

Kotlin cannot infer parameter types, so the hint makes sense for return types and local vars only.

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Sep 3, 2017

Contributor

Ah, very interesting and actually totally doable UI-wise via vscode API.

Personally I don't think i'll like it as one of the reason I dislike codelens is that it mess with my view of the spacing of the code (vertically, but here it would change it horizontally).

But it might be fun to implement, also showing types only when not fully specified seem good, maybe I should do that for LineLens too.

let x: float = 0. // float seem like a pretty useless annotation

Contributor

vbfox commented Sep 3, 2017

Ah, very interesting and actually totally doable UI-wise via vscode API.

Personally I don't think i'll like it as one of the reason I dislike codelens is that it mess with my view of the spacing of the code (vertically, but here it would change it horizontally).

But it might be fun to implement, also showing types only when not fully specified seem good, maybe I should do that for LineLens too.

let x: float = 0. // float seem like a pretty useless annotation

@vasily-kirichenko

This comment has been minimized.

Show comment
Hide comment
@vasily-kirichenko

vasily-kirichenko Sep 3, 2017

Contributor

I agree that the IDEA hints add a lot of noise and changes the structure of code. I ended up assigning a keyboard shortcut to turn the feature on and off and it works pretty well: I turn it off while writing new code and turn it on while exploring existing codebase. However, it has some settings:

image

I find it especially useful not as type hints, but as parameter name ones (which would work nicely for tupled functions and methods only in F# though).

Anyway, such a feature would be a great addition to Ionide, especially if provided with a decent set of user settings.

Contributor

vasily-kirichenko commented Sep 3, 2017

I agree that the IDEA hints add a lot of noise and changes the structure of code. I ended up assigning a keyboard shortcut to turn the feature on and off and it works pretty well: I turn it off while writing new code and turn it on while exploring existing codebase. However, it has some settings:

image

I find it especially useful not as type hints, but as parameter name ones (which would work nicely for tupled functions and methods only in F# though).

Anyway, such a feature would be a great addition to Ionide, especially if provided with a decent set of user settings.

@enricosada

This comment has been minimized.

Show comment
Hide comment
@enricosada

enricosada Sep 4, 2017

Collaborator

Also IDEA seems nice, for some scenario.

Having the full signature at end // string -> int seems faster to read for me, for small functions/values. And never mess with scroll or code position (big bonus point)

But IDEA version can help when there are multiple parameters, like (fun x y z -> x) or if multiple signature are needed in same line, so both have points.
About parameters, depends also if these have parens or not, because without parans can be a bit messy

I use that the same as @vasily-kirichenko , to inspect code, so is not a big drawback the moving if i can quick enable/disable. But i think i'll leave @vbfox version always enabled.

Collaborator

enricosada commented Sep 4, 2017

Also IDEA seems nice, for some scenario.

Having the full signature at end // string -> int seems faster to read for me, for small functions/values. And never mess with scroll or code position (big bonus point)

But IDEA version can help when there are multiple parameters, like (fun x y z -> x) or if multiple signature are needed in same line, so both have points.
About parameters, depends also if these have parens or not, because without parans can be a bit messy

I use that the same as @vasily-kirichenko , to inspect code, so is not a big drawback the moving if i can quick enable/disable. But i think i'll leave @vbfox version always enabled.

@Krzysztof-Cieslak

This comment has been minimized.

Show comment
Hide comment
@Krzysztof-Cieslak

Krzysztof-Cieslak Sep 4, 2017

Member

@vasily-kirichenko: Good suggestion, I'll create feature-request issue when I'll be back in place with decent internet.

@vbfox: Is it ready to merge? If so, do you want to rebase it to create some nice history, or should I squash everything?

Member

Krzysztof-Cieslak commented Sep 4, 2017

@vasily-kirichenko: Good suggestion, I'll create feature-request issue when I'll be back in place with decent internet.

@vbfox: Is it ready to merge? If so, do you want to rebase it to create some nice history, or should I squash everything?

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Sep 4, 2017

Contributor

@Krzysztof-Cieslak squashed & rebased ready to merge for me 👍

Contributor

vbfox commented Sep 4, 2017

@Krzysztof-Cieslak squashed & rebased ready to merge for me 👍

@Krzysztof-Cieslak Krzysztof-Cieslak merged commit 51726f5 into ionide:master Sep 4, 2017

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