nwolverson / purescript-language-server

MIT License
183 stars 41 forks source link

Modes for displaying export lenses #158

Open wclr opened 2 years ago

wclr commented 2 years ago

Looking at new features, suggestions for displaying export lenses.

1) Remove function names from the display, I don't see how those names are of use here, but they pollute the space, so it could be:

exported (remove myCoolFunction from exports) -> exported (remove from exports)

private (add myCoolFunction to exports) -> private (add to exports)

exported (export only myCoolFunction) | (export evertything but myCoolFunction) -> exported (export only this) | (export evertything else)

2) To have a short mode which will display only exported/private one-word notice - just to keep things even cleaner and less distrustful. For implicitly exported things I would propose to leave the notice the same: exported (export only this) | (export everything else) because it is usually no good to have everything implicitly exported.

What do you think?

@i-am-the-slime

nwolverson commented 2 years ago

Agree on the removal of superfluous names. Not convinced on the addition of additional config just to change the wording of something, feels like excessive config at this point rather than say settling on a good middle ground in terms of wording.

wclr commented 2 years ago

I probably would prefer this:

image

to this:

image

Because I don't like unnecessary repeating noise in the code meaning actually nothing, and in this case proposing me to take some action that probably I not going to do in most cases when I look at it (I mean: remove from exports or add to exports), but the notice of exported/private can be useful anyway.

feels like excessive config

Well, this config costs really little and makes things more convenient, at least for some users. We may ask people what they think.

nwolverson commented 2 years ago

I initially feel like I prefer the second, because it is not clear to me what the random "exported" words lying around mean, or what clicking on them does - but I think the style of text should maybe be influenced by the conventions around code lenses.

After some reflection the form of the word "exported" makes it clear that it is a noun and not a verb, the state is clear and hence the toggle action - which can often be a UX problem when displaying toggle state, is this describing the current state or what is going to happen.

Reading a long-ago post collecting code lens examples from vscode: https://code.visualstudio.com/blogs/2017/02/12/code-lens-roundup

I'm happy to leave this open for a while and canvas opinion.

wclr commented 2 years ago

Found a setting for changing color for codeLens:

"editorCodeLens.foreground": "#666"

So it can be made less distractive:

image

But I think I still would prefer a shorter version (because exessive explicitness here doesn't give anything, and it is repeated too often).

After some reflection the form of the word "exported" makes it clear that it is a noun and not a verb, the state is clear and hence the toggle action - which can often be a UX problem when displaying toggle state, is this describing the current state or what is going to happen.

Besides this can be considered a kind of advanced case (for users that know what it means).

private could be replaced with not exported (though I like the word private here, it just reflects state, without any negation, though need to think...)

image

wclr commented 2 years ago

So with short notice, we just give a user an indication of the state.

But when we actually want to give a hint that probably something wrong here, you may need to take some action, we are more verbose and explicit anyway:

image

Also quite important: a long message increases the chance that it will be clicked unintentionally.

i-am-the-slime commented 2 years ago

I think it should be possible to toggle the lenses at the top of the file near the module definition. Managing exports is something that probably doesn't need to be done all the time but at specific times.

Then I think the lenses should have the long names, so it's clear that they're clickable. Otherwise we lack a signifier for the action.

wclr commented 2 years ago

I think it should be possible to toggle the lenses at the top of the file near the module definition.

Could you elaborate on what actions should be available near the module definition?

Then I think the lenses should have long names, so it's clear that they're clickable.

As for lenses above functions, as I noticed above, long names not only bring more distraction, but also increase the chance that they will be clicked unintentionally, and this could be a source of annoyance and even mistakes.

nwolverson commented 2 years ago

Some updates in latest, moving to the "intermediate length" text that doesn't mention the identifier as I think that is at least some kind of starting point. A couple of additional actions/lenses around data constructor stuff, that I missed when grabbing these original changes

wclr commented 2 years ago

Just for notice, this is how Elm's lenses look like:

image

i-am-the-slime commented 2 years ago

@wclr what happens when you click on them?

wclr commented 2 years ago

@wclr what happens when you click on them?

Unexpose/expose action.