fix: force tabs to fixed size

firestack commented 4 months ago

This is a simple fix for #2 Rendering Tab Characters. I think this could be made smarter in the future (i.e., making the tab replacement characters configurable. Potentially getting the tab size from the terminal) but this fixes the immediate issue.

I added a stub of tests, but I'm not confident that I've done this correctly, so I figure we can chat about it.

I also noticed I could try doing this logic in Viewport::draw_text instead, but I wasn't sure if that'd be desired.

Partially Addresses: #2

firestack commented 4 months ago

I ran CI in my repo and I believe I fixed the previous failure

arxanas commented 4 months ago

Thanks!

It looks like this doesn't render the tab character in gray, like we do with the newline character. We definitely want to portray a semantic difference between literal characters and control characters that we added custom renderings for. (Color is suboptimal as the sole indicator, but the terminal doesn't give us a lot of accessibility options here.)
Can we actually combine the rendering of newlines and tabs into the same function, and render them both in gray as their specified replacement string?
- We would change the behavior for newlines to render them anywhere on the line, not just at the very end. This would handle callers passing multiple newline characters at the end of the line for whatever reason, or cases where newlines are not the selection boundary (I experimented with word diff once, which might have run into this), or maybe cases with string literals with embedded control characters. Currently I expect that it would affect no consumers, though.
- Rather than print each Span in a loop yourself, it's probably best to collect them into a container (Line?) and use the normal printing functions to print that container.

firestack commented 3 months ago

We would change the behavior for newlines to render them anywhere on the line, not just at the very end. This would handle callers passing multiple newline characters at the end of the line for whatever reason, or cases where newlines are not the selection boundary

One question, when would multiple newlines appear in a single line?.. A line, by definition, shouldn't contain multiple newlines, correct?

firestack commented 3 months ago

We would change the behavior for newlines to render them anywhere on the line, not just at the very end. This would handle callers passing multiple newline characters at the end of the line for whatever reason, or cases where newlines are not the selection boundary

One question, when would multiple newlines appear in a single line?.. A line, by definition, shouldn't contain multiple newlines, correct?

The way I've tried implementing this doesn't actually care, so this will work out anyway if it does happen 👍🏻

firestack commented 3 months ago

I think I've got something that addresses feedback! Cleaning things up and then will push 👍🏻

arxanas commented 2 months ago

I queued https://github.com/arxanas/scm-record/pull/46 for merge; you might have to rebase/update tests.

firestack commented 2 months ago

can you check cargo clippy and cargo test and make sure they pass on each of the commits in the stack individually?

@arxanas Just ran both(at every commit in the stack, I wish JJ run worked lol), and didn't get any failures.

IIRC two of them should be squashed

I think it was handled in this? https://github.com/arxanas/scm-record/pull/37#discussion_r1641581903

arxanas commented 2 months ago

This was the lint error I was talking about on the bottom of the stack:

    Checking scm-record v0.3.0 (/Users/waleed/Workspace/scm-record)
error: method `draw_line` is never used
   --> src/render.rs:527:12
    |
368 | impl<'a, ComponentId: Clone + Debug + Eq + Hash> Viewport<'a, ComponentId> {
    | -------------------------------------------------------------------------- method in this implementation
...
527 |     pub fn draw_line(&mut self, x: isize, y: isize, line: &Line) -> Rect {
    |            ^^^^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(dead_code)]`

error: could not compile `scm-record` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `scm-record` (lib test) due to 1 previous error

I squashed together the bottom two commits to fix this. Thanks for implementing this!

firestack commented 2 months ago

This was the lint error I was talking about on the bottom of the stack:

    Checking scm-record v0.3.0 (/Users/waleed/Workspace/scm-record)
error: method `draw_line` is never used
   --> src/render.rs:527:12
    |
368 | impl<'a, ComponentId: Clone + Debug + Eq + Hash> Viewport<'a, ComponentId> {
    | -------------------------------------------------------------------------- method in this implementation
...
527 |     pub fn draw_line(&mut self, x: isize, y: isize, line: &Line) -> Rect {
    |            ^^^^^^^^^
    |
    = note: `-D dead-code` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(dead_code)]`

error: could not compile `scm-record` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...
error: could not compile `scm-record` (lib test) due to 1 previous error

I squashed together the bottom two commits to fix this. Thanks for implementing this!

oh sorry! I swear I tried to find them😅! I have a bunch of my own clippy lints in my global config which makes it hard to see these sometimes but I had sworn that it had no failures.

arxanas commented 2 months ago

@firestack It's no trouble 😊. Can I interest you in git test? 😉

This is my config for this project:

$ git test run -c help  # HACK
The test command alias "help" was not defined.

To create it, run: git config branchless.test.alias.help <command>
Or use the -x/--exec flag instead to run a test command without first creating an alias.

These are the currently-configured command aliases:
• branchless.test.alias.lint = "cargo clippy --workspace --all-targets -- -D warnings"
• branchless.test.alias.fmt-fix = "cargo +nightly fmt --all"
• branchless.test.alias.test = "cargo nextest run --workspace --no-fail-fast"

$ git test run -c lint 'stack()'
...

which is how I checked for lint+test failures.

(Ideally, GitHub would just be better at doing stacked PRs, or make it easier to run actions on each commit in a PR, and we'd have CI per commit and it would be surfaced in a useful manner.)

arxanas / scm-record

fix: force tabs to fixed size #37