Softwrapping improvements #5893

divarvel · 2023-02-09T18:03:08Z

This is a continuation of #2423

rebase on top of a recent master
apply feedback from Add max_line_len to config.toml #2423
merge use max_line_width + 1 during softwrap to account for newline char #5813
~~make global text-length optional~~
add a way to ignore text-width when softwrapping
add a way to ignore text-width when softwrapping (per-language)

Helix softwrap implementation always wraps lines so that the newline character doesn't get cut off so he line wraps one chars earlier then in other editors. This is necessary, because newline chars are always selecatble in helix and must never be hidden. However That means that `max_line_width` currently wraps one char earlier than expected. The typical definition of line width does not include the newline character and other helix commands like `:reflow` also don't count the newline character here. This commit makes softwrap use `max_line_width + 1` instead of `max_line_width` to correct the impedance missmatch.

Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>

pascalkuthe · 2023-02-09T18:09:12Z

looks like some checks are failing, something todo with xtask, that probably didn't exist yet when the original PR was made :D
You could merge #5813 here if you want to. It's a pretty trivial change (and was prettymuch an oversight to begin with) and as you are touching that code anyway it would be nice to avoid merge conflicts.

divarvel · 2023-02-09T19:01:42Z

Alright, i've done the cleanup tasks. Next, i will make the global text-length setting optional, as i don't think soft-wrapping everything to 80 cols by default will please a lot of people.

One thing to note is that once a global text-length value is set, it is not possible to go back to no value in a languages setting.

pascalkuthe · 2023-02-09T19:37:04Z

One thing to note is that once a global text-length value is set, it is not possible to go back to no value in a languages setting.

I think thats fine in case it really bothers people they can just set it to something huge fo a sepcific language

Now that #5813 is included, this fixes #5810

When it was only used for `:reflow` it made sense to have a default value set to `80`, but now that soft-wrapping uses this setting, keeping a default set to `80` would make soft-wrapping behave more aggressively.

divarvel · 2023-02-10T12:13:08Z

Alright, i

made text-width optional and put the fallback of 79 back in the reflow impl;
added a new wrap-at-text-width setting in Softwrap;
renamed the PR to account for the superseding of use max_line_width + 1 during softwrap to account for newline char #5813.

I'm sure the wording of comments and documentation could be improved, don't hesitate to add suggestions

Softwrapping wraps by default to the viewport width or a configured `text-width` (whichever's smaller). In some cases we only want to set `text-width` to use for hard-wrapping and let longer lines flow if they have enough space. This setting allows that.

divarvel · 2023-02-10T12:46:42Z

helix-term/src/commands/typed.rs

-                .and_then(|config| config.max_line_length)
-        })
+        .or_else(|| doc.language_config().and_then(|config| config.text_width))
+        .or(cfg_text_width)


wouldn't it make more sense to no-op when no text_width value is set?

I think that would be a pretty bad user experience since the default for the global text-width is None so by default :reflow would do nothing. Why are we defaulting to 79 instead of 80 here btw?

Good point.

It was added in f9baced

I would change the default to 80 here as we are breaking configs anyway. Seems like that was just missed in the original PR.
@archseer also braught that up in #2423 (comment)

thinking about more, i think it makes more sense to make text-width always defined with a default value of 80 instead of having this hardcoded number, and maybe make softwrap ignore text-width by default (i was quite surprised by the current behaviour, at first i thought it was a bug)

book/src/configuration.md

divarvel · 2023-02-16T19:13:24Z

The issue is tagged waiting-on-author, but i'm not sure what's expected of me. I think there is room for improvement wrt defaults, but i don't think that should be my decision. What's done currently works, anyway.

pascalkuthe · 2023-02-16T19:24:17Z

A PR is usally tagged as waiting-on-author until all concerns are addressed.

In this case that would be #5893 (comment). Usually I don't lay out the entire solution and just give comments and let the authors figure out a good solution from there.

But since you asked here is my idea how this should be addressed:

Add a soft-wrap-at-text-width to the language config (you can use a better name if you come up with one) config option that can control whether to use text-width for soft-wrapping for this language.
That option should default to editor.soft-wrap.wrap-at-text-width
editor.soft-wrap.wrap-at-text-width should default to false
text-width (both globally and per language) should default to 80.

I would love to see this PR merge before the next release so if you are unsure how to proceed please just ask :)

divarvel · 2023-02-16T19:29:44Z

I would love to see this PR merge before the next release so if you are unsure how to proceed please just ask :)

Yeah that's why I asked, i was not sure if #5893 (comment) was just a suggestion for further improvement or a blocking issue. Since this PR introduces a breaking change and it is added to the release milestone, i guess getting things right the first time is important.

I'll give your idea a try and lay out pros/cons for other solutions so that we can move ahead.

pascalkuthe · 2023-02-16T19:34:28Z

I would love to see this PR merge before the next release so if you are unsure how to proceed please just ask :)

Yeah that's why I asked, i was not sure if #5893 (comment) was just a suggestion for further improvement or a blocking issue. Since this PR introduces a breaking change and it is added to the release milestone, i guess getting things right the first time is important.

I'll give your idea a try and lay out pros/cons for other solutions so that we can move ahead.

In general you can consider all my review comments intended as blocking issues unless during a review I explicitly say otherwise. I can see how that comment didn't sound as autharative and could be taken the wrong way if you didn't know that so it's good you asked to avoid misunderstandings :)

yeah for a breaking changes to the config (especially one in a realease milestone) we want to make sure it's the right change to minimize the number of breaking changes.

divarvel · 2023-02-16T19:41:34Z

i'm not super comfortable with having the same option live in two different places in the editor-wide config and in language-specific config. The alternative is to be able to override the whole softwrap config in language settings, or maybe to add a soft-wrap table to the langage settings, containing only wrap-at-text-width for now.

Agreed on editor.soft-wrap.wrap-at-text-width defaulting to false. I did not suggest it because it would change default behaviour, but i think it's a better default that what's done currently, so 👍

pascalkuthe · 2023-02-16T19:48:39Z

i'm not super comfortable with having the same option live in two different places in the editor-wide config and in language-specific config. The alternative is to be able to override the whole softwrap config in language settings, or maybe to add a soft-wrap table to the langage settings, containing only wrap-at-text-width for now.

Yeah that might be better and more futurue proof as it allows overwriting all softwrap options per language 👍
It's why I wasn't as authorative above, I wasn't sure if it was exactly the right solution.

That would require moving the SoftWrap config struct to helix-core tough (as that is where the language config currently lives) and changing all fields to options so that we can do fallback like let max_wrap = language_config.soft_wrap.max_wrap.or(editor_config.soft_wrap.max_wrap).unwrap_or(20); (where 20 is the default value for editor_config.soft_wrap.max_wrap). Or in other words: We need to use options instead of just Default so we can detect whether a value was manually overwritten in the langauge config.

divarvel · 2023-02-16T19:51:00Z

another solution would be to define a dedicated SoftWrap type for the language config, this would allow both:

add support for each field separately (iiuc that would not be breaking changes so it could be done in separate PRs, in order to not block this one)
not require modifying the existing one by making all fields optional and thus reduce ripple effects (also in order to not block this PR)

if there is something my haskell days have taught me, is to not be shy with creating new types instead of trying to co-opt existing ones, even when they seem close (there are tricks to have a single definition where fields can be optional or not depending on context, but that might not be idiomatic rust)

pascalkuthe · 2023-02-16T19:55:20Z

another solution would be to define a dedicated SoftWrap type for the language config, this would allow both:

* add support for each field separately (iiuc that would not be breaking changes so it could be done in separate PRs, in order to not block this one)

* not require modifying the existing one by making all fields optional and thus reduce ripple effects (also in order to not block this PR)

I would prefer avoidng duplicating this struct. It's really just a convenience feature to use the Default implementation. Option<T> is actually more accurate for the config options (they are optional afterall).

I also would love if this PR could already add these additional config options in one go. It would be really great to have and something people have been asking for. Usually I am all for split things like these out to a across separate PR but adding support for all fields is really quite trivial and essentially just requires a line like I have shown above.

divarvel · 2023-02-16T20:03:26Z

I would prefer avoidng duplicating this struct. It's really just a hack to use the Default implementation. Option is actually more accurate for the config options (they are optional afterall).

the downside is that it puts default values at use site instead of having them in one place (it can be solved by constants, but it would make things a bit gnarly).

Another possible issue i have noticed when working on :toggle is that toggle relies on values being defined through Default. If the values are made optional, then it's not possible to inspect the serialized TOML config to know whether a value is a boolean or not. So making the fields optional instead of relying on Default would essentially break :toggle, and possibly :set.

To be clear: i think having the config type faithfully model optional config options would be a good thing in general, but in practice the codebase relies on the TOML serialization of this type being a fully defined values, with all possible values defined. Fixing this coupling would be a good thing, but maybe out of scope.

This reverts commit b247d52.

pascalkuthe · 2023-02-16T20:11:37Z

I would prefer avoidng duplicating this struct. It's really just a hack to use the Default implementation. Option is actually more accurate for the config options (they are optional afterall).

the downside is that it puts default values at use site instead of having them in one place (it can be solved by constants, but it would make things a bit gnarly).

Another possible issue i have noticed when working on :toggle is that toggle relies on values being defined through Default. If the values are made optional, then it's not possible to inspect the serialized TOML config to know whether a value is a boolean or not. So making the fields optional instead of relying on Default would essentially break :toggle, and possibly :set

Don't worry neither :set nor any :toggle implementation should be relying on the defaults of Default. There are already other Option config options in the editor config.

There is only a single place that the Softwrap config is used in Document::text_annotations and that's not likely to change so I don't think it's a huge concern to have this in one place. There are already other complex fallback meachanisims in that function so I don't think expanding them will cause issues.,

divarvel · 2023-02-16T21:16:47Z

alright, i have added support for per-language softwrap settings. I'm still not super satisfied with the default values moving away from the SoftWrap definition, but it seems to work okay.

:get now returns null for all softwrap fields if not overriden.

pascalkuthe

Just some minor nits otherwise this looks great now :)

book/src/configuration.md

book/src/languages.md

book/src/configuration.md

Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>

pascalkuthe

This LGTM now, thanks for tackling that. I definitely wanted to see consistent configuration for softwrap before the next release.

I am not sure what to do about :get. It already returns null for other configuration like the whitespace.render configuration options which also have a more complex fallback behavior. My opinion is that this is a problem with the :get command as fixing this would preclude us from doing any kind of non-trivial fallback logic in the config.

divarvel · 2023-02-17T14:23:44Z

I am not sure what to do about :get. It already returns null for other configuration like the whitespace.render configuration options which also have a more complex fallback behavior. My opinion is that this is a problem with the :get command as fixing this would preclude us from doing any kind of non-trivial fallback logic in the config.

My take would be to have a type faithfully representing config file contents: everything optional, no default values, and a type representing the actual (computed) configuration. With this setup, overrides and default values are set in a single place (when computing actual configuration from the config files). So defaults are all grouped, and all use sites read from the computed config, so there is no risk of forgetting about overrides when using config values. This approach would make :get work correctly in all cases, for instance.

If the goal is eventually to let languages.toml override everything that makes sense in config.toml, in addition to supporting per-directory config.toml files, properly separating "what the user has explicitly configured" from "what is the actual config" seems important.

pascalkuthe · 2023-02-17T14:26:45Z

I am not sure what to do about :get. It already returns null for other configuration like the whitespace.render configuration options which also have a more complex fallback behavior. My opinion is that this is a problem with the :get command as fixing this would preclude us from doing any kind of non-trivial fallback logic in the config.

My take would be to have a type faithfully representing config file contents: everything optional, no default values, and a type representing the actual (computed) configuration. With this setup, overrides and default values are set in a single place (when computing actual configuration from the config files). So defaults are all grouped, and all use sites read from the computed config, so there is no risk of forgetting about overrides when using config values. This approach would make :get work correctly in all cases, for instance.

If the goal is eventually to let languages.toml override everything that makes sense in config.toml, in addition to supporting per-directory config.toml files, properly separating "what the user has explicitly configured" from "what is the actual config" seems important.

The problem with this approach is that something like whitespace.render has complex fallbacks. That means if config option X changes multiple different config options may change too. Without using options calling :set X value would only update X and not all those configurations that depend on it.

The right way to do this is probably by keeping the raw toml Value around and modifying that with :set and then generate the actual config from that. This ensures that :set always merges on top of local configuration too.

I think this can be to future work tough as it would require larger changes and is a more fundemental problem that doesn't need to be tackeled here.

* use max_line_width + 1 during softwrap to account for newline char Helix softwrap implementation always wraps lines so that the newline character doesn't get cut off so he line wraps one chars earlier then in other editors. This is necessary, because newline chars are always selecatble in helix and must never be hidden. However That means that `max_line_width` currently wraps one char earlier than expected. The typical definition of line width does not include the newline character and other helix commands like `:reflow` also don't count the newline character here. This commit makes softwrap use `max_line_width + 1` instead of `max_line_width` to correct the impedance missmatch. * fix typos Co-authored-by: Jonathan Lebon <jonathan@jlebon.com> * Add text-width to config.toml * text-width: update setting documentation * rename leftover config item * remove leftover max-line-length occurrences * Make `text-width` optional in editor config When it was only used for `:reflow` it made sense to have a default value set to `80`, but now that soft-wrapping uses this setting, keeping a default set to `80` would make soft-wrapping behave more aggressively. * Allow softwrapping to ignore `text-width` Softwrapping wraps by default to the viewport width or a configured `text-width` (whichever's smaller). In some cases we only want to set `text-width` to use for hard-wrapping and let longer lines flow if they have enough space. This setting allows that. * Revert "Make `text-width` optional in editor config" This reverts commit b247d52. * soft-wrap: allow per-language overrides * Update book/src/configuration.md Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de> * Update book/src/languages.md Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de> * Update book/src/configuration.md Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de> --------- Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de> Co-authored-by: Jonathan Lebon <jonathan@jlebon.com> Co-authored-by: Alex Boehm <alexb@ozrunways.com> Co-authored-by: Blaž Hrastnik <blaz@mxxn.io>

pascalkuthe and others added 5 commits February 3, 2023 22:27

fix typos

d370598

Co-authored-by: Jonathan Lebon <jonathan@jlebon.com>

Add text-width to config.toml

404406a

text-width: update setting documentation

d102351

rename leftover config item

f3d0027

pascalkuthe added this to the next milestone Feb 9, 2023

divarvel mentioned this pull request Feb 9, 2023

Add max_line_len to config.toml #2423

Closed

pascalkuthe added E-easy Call for participation: Experience needed to fix: Easy / not much A-helix-term Area: Helix term improvements R-breaking-change This PR is a breaking change for some behavior labels Feb 9, 2023

divarvel added 2 commits February 9, 2023 19:19

remove leftover max-line-length occurrences

19bb502

Merge remote-tracking branch 'pascalkuthe/master' into max-line-len

6aad3ff

pascalkuthe mentioned this pull request Feb 9, 2023

use max_line_width + 1 during softwrap to account for newline char #5813

Closed

Make text-width optional in editor config

b247d52

When it was only used for `:reflow` it made sense to have a default value set to `80`, but now that soft-wrapping uses this setting, keeping a default set to `80` would make soft-wrapping behave more aggressively.

divarvel force-pushed the max-line-len branch from e69ade3 to 0a8cdeb Compare February 10, 2023 12:14

divarvel changed the title ~~Support for configuring wrapping width in global config~~ Softwrapping improvements Feb 10, 2023

divarvel force-pushed the max-line-len branch from 0a8cdeb to 7bff2a1 Compare February 10, 2023 12:18

divarvel force-pushed the max-line-len branch from 7bff2a1 to ee5f48d Compare February 10, 2023 12:32

divarvel marked this pull request as ready for review February 10, 2023 12:32

divarvel commented Feb 10, 2023

View reviewed changes

pascalkuthe added the S-waiting-on-review Status: Awaiting review from a maintainer. label Feb 10, 2023

gabydd reviewed Feb 10, 2023

View reviewed changes

book/src/configuration.md Outdated Show resolved Hide resolved

pascalkuthe added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 10, 2023

Revert "Make text-width optional in editor config"

51d9f30

This reverts commit b247d52.

divarvel force-pushed the max-line-len branch from a5eceaf to 51d9f30 Compare February 16, 2023 20:05

soft-wrap: allow per-language overrides

0c5bbe8

divarvel force-pushed the max-line-len branch from e2ce415 to 0c5bbe8 Compare February 16, 2023 21:29

pascalkuthe added S-waiting-on-review Status: Awaiting review from a maintainer. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 16, 2023

pascalkuthe requested changes Feb 16, 2023

View reviewed changes

book/src/configuration.md Outdated Show resolved Hide resolved

book/src/languages.md Outdated Show resolved Hide resolved

book/src/configuration.md Outdated Show resolved Hide resolved

divarvel and others added 3 commits February 17, 2023 08:54

Update book/src/configuration.md

505c731

Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>

Update book/src/languages.md

ad58166

Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>

Update book/src/configuration.md

9eaeb8d

Co-authored-by: Pascal Kuthe <pascal.kuthe@semimod.de>

pascalkuthe approved these changes Feb 17, 2023

View reviewed changes

gibbz00 mentioned this pull request Feb 22, 2023

Unify and clarify config loading #5636

Closed

5 tasks

Merge branch 'master' into max-line-len

3e0abd9

archseer merged commit 8dd1ab4 into helix-editor:master Mar 8, 2023

pascalkuthe mentioned this pull request Mar 12, 2023

rework positioning/rendering and enable softwrap/virtual text #5420

Merged

gabydd mentioned this pull request Mar 20, 2023

Line is soft-wrapped even if within max-line-length limit #5810

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Softwrapping improvements #5893

Softwrapping improvements #5893

divarvel commented Feb 9, 2023 •

edited

Loading

pascalkuthe commented Feb 9, 2023 •

edited

Loading

divarvel commented Feb 9, 2023

pascalkuthe commented Feb 9, 2023

divarvel commented Feb 10, 2023 •

edited

Loading

divarvel Feb 10, 2023

pascalkuthe Feb 10, 2023

divarvel Feb 10, 2023

pascalkuthe Feb 10, 2023

divarvel Feb 10, 2023

divarvel commented Feb 16, 2023

pascalkuthe commented Feb 16, 2023

divarvel commented Feb 16, 2023

pascalkuthe commented Feb 16, 2023

divarvel commented Feb 16, 2023 •

edited

Loading

pascalkuthe commented Feb 16, 2023 •

edited

Loading

divarvel commented Feb 16, 2023 •

edited

Loading

pascalkuthe commented Feb 16, 2023 •

edited

Loading

divarvel commented Feb 16, 2023 •

edited

Loading

pascalkuthe commented Feb 16, 2023

divarvel commented Feb 16, 2023 •

edited

Loading

pascalkuthe left a comment

pascalkuthe left a comment

divarvel commented Feb 17, 2023

pascalkuthe commented Feb 17, 2023 •

edited

Loading

Softwrapping improvements #5893

Softwrapping improvements #5893

Conversation

divarvel commented Feb 9, 2023 • edited Loading

pascalkuthe commented Feb 9, 2023 • edited Loading

divarvel commented Feb 9, 2023

pascalkuthe commented Feb 9, 2023

divarvel commented Feb 10, 2023 • edited Loading

divarvel Feb 10, 2023

Choose a reason for hiding this comment

pascalkuthe Feb 10, 2023

Choose a reason for hiding this comment

divarvel Feb 10, 2023

Choose a reason for hiding this comment

pascalkuthe Feb 10, 2023

Choose a reason for hiding this comment

divarvel Feb 10, 2023

Choose a reason for hiding this comment

divarvel commented Feb 16, 2023

pascalkuthe commented Feb 16, 2023

divarvel commented Feb 16, 2023

pascalkuthe commented Feb 16, 2023

divarvel commented Feb 16, 2023 • edited Loading

pascalkuthe commented Feb 16, 2023 • edited Loading

divarvel commented Feb 16, 2023 • edited Loading

pascalkuthe commented Feb 16, 2023 • edited Loading

divarvel commented Feb 16, 2023 • edited Loading

pascalkuthe commented Feb 16, 2023

divarvel commented Feb 16, 2023 • edited Loading

pascalkuthe left a comment

Choose a reason for hiding this comment

pascalkuthe left a comment

Choose a reason for hiding this comment

divarvel commented Feb 17, 2023

pascalkuthe commented Feb 17, 2023 • edited Loading

divarvel commented Feb 9, 2023 •

edited

Loading

pascalkuthe commented Feb 9, 2023 •

edited

Loading

divarvel commented Feb 10, 2023 •

edited

Loading

divarvel commented Feb 16, 2023 •

edited

Loading

pascalkuthe commented Feb 16, 2023 •

edited

Loading

divarvel commented Feb 16, 2023 •

edited

Loading

pascalkuthe commented Feb 16, 2023 •

edited

Loading

divarvel commented Feb 16, 2023 •

edited

Loading

divarvel commented Feb 16, 2023 •

edited

Loading

pascalkuthe commented Feb 17, 2023 •

edited

Loading