Fix lineWrap issues #67
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The test that we're adapting here shows what's wrong: we end up with a wrapped line that's one character longer than our allowed width.
The space is kept in the wrapped string, which we don't want. Works correctly for hyphen already.
I don't understand what the `&& lastWhitespaceIndex+1 != i` condition was added for; I can't see a reason for it. Removing it fixes the problem. Granted, it's not a serious problem, not even a cosmetic one as the space is not visible. Still, it's cleaner and less confusing this way.
These work correctly here, but it's good to have the test coverage (there was a bug related to this case in the lazygit code that copies this algorithm).
Doesn't work correctly yet.
As opposed to "logical" lines, which means that when a long line is wrapped, we only highlight the actual view line that the cursor is in, rather than all wrapped view lines that belong to the original, unwrapped line. This effectively reverts what was done to fix jroimartin#50, but since we don't need (or want) this feature in lazygit, we don't bother making this configurable.
I don't see a reason why we should prevent any code from instantiating a view if they want to. We want to use it in a test in lazygit.
It's surprising to clients if this doesn't happen, and it's a very cheap call, so we can afford it.
So far, search was only enabled in views with Wrap=false, so it didn't make a difference. If Wrap is on, we need to work with viewLines rather than lines, because that's what the code that highlights the search results expects. Note that a viewLine index is also passed back to the onSelectItem callback, so clients need to be able to deal with that. We don't touch the modelSearchResults code path, as it's a bit unclear how to support Wrap=true in that case, and we don't need it right now.
stefanhaller
added a commit
to jesseduffield/lazygit
that referenced
this pull request
Dec 2, 2024
Corresponds to the changes in jesseduffield/gocui#67.
7 tasks
stefanhaller
added a commit
to jesseduffield/lazygit
that referenced
this pull request
Dec 23, 2024
- **PR Description** Add a user config (on by default) to turn on line wrapping in the staging view (and custom patch building view). Fixes #3558. This is a bit of a longer PR because I had to fix a bunch of things to make this possible, and because it takes much more than just turning on `Wrap` on the staging view. Usually when I make changes to gocui I include them in the lazygit PR for easier review; this time, however, the changes required touching gocui's tests, which are not included in our vendored copy, so I made a [PR in gocui](jesseduffield/gocui#67) to be reviewed there, and only included the squashed changes here. Hot-reloading of the new user config is not perfect: if the staging view is focused, you need to escape out of it and enter it again for the change to take affect. I wasn't sure it's worth adding code to fix this. As for tests: I didn't find a way to add an integration test for this, as the view geometry is not fixed when you run them locally. But I didn't look into this very deeply. - **Please check if the PR fulfills these requirements** * [x] Cheatsheets are up-to-date (run `go generate ./...`) * [x] Code has been formatted (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#code-formatting)) * [x] Tests have been added/updated (see [here](https://github.com/jesseduffield/lazygit/blob/master/pkg/integration/README.md) for the integration test guide) * [x] Text is internationalised (see [here](https://github.com/jesseduffield/lazygit/blob/master/CONTRIBUTING.md#internationalisation)) * [x] If a new UserConfig entry was added, make sure it can be hot-reloaded (see [here](https://github.com/jesseduffield/lazygit/blob/master/docs/dev/Codebase_Guide.md#using-userconfig)) * [ ] Docs have been updated if necessary * [x] You've read through your own file changes for silly mistakes etc
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes several issues with the lineWrap implementation (see individual commit messages for details).
In addition, it changes view highlighting so that in a view with Wrap=true, only the current "physical" line is highlighted, rather than all view lines that belong to the current "logical" line. This is not a breaking change since in lazygit highlighting is only used in views that don't wrap; we want to change that, however.
Finally, it fixes searching for views with Wrap=true.