arxanas / scm-record

`scm-record` is a UI component to interactively select changes to include in a commit. It's meant to be embedded in source control tooling.
32 stars 9 forks source link

bug: tab characters not rendered correctly #2

Closed crackcomm closed 2 weeks ago

crackcomm commented 11 months ago

Issue mentioned on discord. Not sure exactly what happened - not rendered tab or replaced with a different character.

image

misrendered output (notice last character is missing): image

with spaces instead of tabs: image

crackcomm commented 11 months ago

I noticed missing last characters even in output w/o indentation. It was some go.sum file IIRC.

firestack commented 7 months ago

Also running into this this. I couldn't quite deduce in the code where this is rendered/handled (I did look through src/render.rs).

I'm looking at fixing this so if anyone has any pointers as to where to start reading and/or editing that'd be appreciated!

arxanas commented 6 months ago

Also running into this this. I couldn't quite deduce in the code where this is rendered/handled (I did look through src/render.rs).

@firestack I believe that there's nowhere specific in the code that handles tab characters. My guess is that we simply print tab characters to the screen literally, and the terminal emulator does the normal tab behavior, i.e. moves the cursor to the next tabbable column, and continues printing there. This kind of makes sense as we see in the screenshots how random parts of the output seem to be truncated — it would make sense if we just started drawing/overwriting at random later portions of the line.

The easiest fix would be to replace each tab character with several spaces (note that we have to choose a fixed-width representation as we won't be able to calculate where the rest of the line would render, figure out where to truncate text, be able to render text side-by-side, etc.).

firestack commented 6 months ago

Awesome, thank you @arxanas!

I was able to fix the buggy rendering by replacing tabs with the symbol. But as a user (who loves tabs), I wouldn't be happy with this as a final solution. I'm personally a fan of , but I wouldn't expect that to work for everyone.

Intermediate solution diff ```diff diff --git a/src/ui.rs b/src/ui.rs index 1692a54a6b...0078ec9045 100644 --- a/src/ui.rs +++ b/src/ui.rs @@ -3029,6 +3029,12 @@ fn draw(&self, viewport: &mut Viewport, x: isize, y: isize) { const NEWLINE_ICON: &str = "⏎"; + const TAB_ICON: &str = "␉"; + + fn replace_control_characters(line: &str) -> String { + line.replace("\t", TAB_ICON) + } + let Self { line_key: _, inner } = self; viewport.draw_blank(Rect { x: viewport.mask_rect().x, @@ -3046,7 +3052,7 @@ viewport.draw_span(x, y, &Span::styled(format!("{line_num:5} "), style)); let (line, line_end) = match line.strip_suffix('\n') { Some(line) => ( - Span::styled(line, style), + Span::styled(replace_control_characters(line), style), Some(Span::styled( NEWLINE_ICON, Style::default().fg(Color::DarkGray), @@ -3080,7 +3086,7 @@ let x = x + change_type_text.width().unwrap_isize(); let (line, line_end) = match line.strip_suffix('\n') { Some(line) => ( - Span::styled(line, style), + Span::styled(replace_control_characters(line), style), Some(Span::styled( NEWLINE_ICON, Style::default().fg(Color::DarkGray), ```

I briefly looked for some way to make this character configurable but didn't find anything, so any pointers would be appreciated.

I'm thinking on it some more before opening a PR to chat about it, but if there's no immediate objections here and I don't come up with something better, I'll open this diff as a PR "Soon™️"

arxanas commented 6 months ago

The arrow seems fine to me. I would prefer to have intuitive iconography to descriptive iconography. We are not trying to have 100% fidelity for the rendered diff anyways. But I would like to see the arrow embedded in some number of spaces for alignment, rather than having the tab itself span only one column.