Skip to content
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

Avoid ignoring newlines in multi line messages when rendering end-of-line diagnostics #11815

Conversation

irh
Copy link
Contributor

@irh irh commented Oct 2, 2024

set_string_truncated renders the entire string while ignoring newlines, so if the diagnostic's message contains multiple lines then draw_eol_diagnostic produces output like 'first linesecond line'.

An alternative would be to reformat the message to replace newlines with spaces (or something), but I think the first line should be sufficient for end of line diagnostics.

Before:
Screenshot 2024-10-02 at 10 13 25

After:
Screenshot 2024-10-02 at 10 14 49

the-mikedavis
the-mikedavis previously approved these changes Oct 3, 2024
@pascalkuthe
Copy link
Member

pascalkuthe commented Oct 3, 2024

I do find the second lines useful quite often so the current situation is preferable to this IMO

@irh
Copy link
Contributor Author

irh commented Oct 4, 2024

@pascalkuthe Ok, I checked Neovim's behavior and it uses double spaces to separate the lines, which seems like a reasonable idea to me. Here's the example from above using this approach.

Screenshot 2024-10-04 at 09 39 52

@pascalkuthe
Copy link
Member

Seems good. I don't really care what the seperator looks like, I just want all information to be there

the-mikedavis
the-mikedavis previously approved these changes Oct 6, 2024
@@ -102,10 +102,11 @@ impl Renderer<'_, '_> {
return 0;
}
let col = (col - self.renderer.offset.col) as u16;
let message = diag.message.replace('\n', " ");
Copy link
Member

@pascalkuthe pascalkuthe Oct 6, 2024

Choose a reason for hiding this comment

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

this allocates a lot (once for each diagnostic during each frame). I intentionally didn't do something like this in the original PR for this reason. We will need to refactor set_string_truncated but since there are other consumers that is a larger change and I am worried about breaking other places that use that API (but maybe after this change we can update unicode-width and then it's fine?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wasn't sure how much I needed to worry about allocations here, but I'm happy to avoid the overhead.

Rather than modifying set_string_truncated I've adapted draw_eol_diagnostic to render the lines separately in a loop. This behaves well in my testing but please let me know if I've overlooked something.

@irh irh force-pushed the fix-eol-diagnostic-with-multiline-message branch from d10bc48 to 696b79e Compare October 7, 2024 07:15
@irh
Copy link
Contributor Author

irh commented Oct 7, 2024

Apologies for the force push, I rebased but then went back. Other than the new `"Draw each message line separately" commit the history hasn't been changed.

This was copied from the function above (set_style). I don't know enough
about the function to suggest an alternative.
@irh irh changed the title Only render the first line of the message in end of line diagnostics Avoid ignoring newlines in multi line messages when rendering end-of-line diagnostics Oct 22, 2024
`set_string_truncated` renders the entire string while ignoring
newlines, so if the diagnostic's message contains multiple lines it
produces messages like 'first linesecond line'.

To avoid these run-ons, this commit renders each line separately,
inserting double spaces for disambiguation.
@irh irh force-pushed the fix-eol-diagnostic-with-multiline-message branch from 696b79e to 6da03c0 Compare October 22, 2024 08:30
@irh
Copy link
Contributor Author

irh commented Oct 22, 2024

I rebased the PR on to the latest master to keep it fresh, and squashed the solution-finding commits to make the intention of the branch clearer.

@the-mikedavis the-mikedavis merged commit 4c8175c into helix-editor:master Dec 5, 2024
6 checks passed
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.

4 participants