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

fix hard tab display #33

Merged
merged 4 commits into from
Oct 20, 2023
Merged

fix hard tab display #33

merged 4 commits into from
Oct 20, 2023

Conversation

pm100
Copy link
Contributor

@pm100 pm100 commented Oct 18, 2023

hard tabs always insert 4 spaces

heres is minimal example switched to hard tab mode by adding
textarea.set_hard_tab_indent(true);

image

this is the result of typing

  • a,tab,b,return
  • aa,tab,b,return
  • aaa,tab,b,return

the bs should be aligned, they are not

src/highlight.rs Outdated Show resolved Hide resolved
src/highlight.rs Outdated Show resolved Hide resolved
src/highlight.rs Outdated Show resolved Hide resolved
@pm100
Copy link
Contributor Author

pm100 commented Oct 19, 2023

Tbc,I will fix the tab_len ==0 in prperae_line. I was asking 'should I allow tabs in the case of hard tabs and tab len==0' I think the answer is yes. If the user wants to insert a hard tab, they should be allowed to

@rhysd
Copy link
Owner

rhysd commented Oct 19, 2023

@pm100 Do you mean removing this if guard? Then I'm not sure it should insert tab character. When a tab character is inserted to text buffer, the text displayed to the user has not changed. It would be confusing for the user.

if self.tab_len == 0 {
return false;
}

Anyway, I feel this is an off-topic for this PR.

@pm100
Copy link
Contributor Author

pm100 commented Oct 19, 2023

@joshka these tests are in fact incorrect. They are verifying the incorrect behavior that this PR is fixing. But thank you for them. I will fix them

I was thinking 'system test' rather than 'unit test' when I said it was very hard to do.

Question: how did you create these? copilot, I have heard of it but not used it.

@rhysd
Copy link
Owner

rhysd commented Oct 20, 2023

Some kind of system test is easy to implement for this crate because we can use the test backend (or create our own dummy backend). The problem is that I have no time to implement the test cases.

@rhysd rhysd merged commit 64ca882 into rhysd:main Oct 20, 2023
@rhysd
Copy link
Owner

rhysd commented Oct 20, 2023

Thank you for the changes. I merged.

@pm100
Copy link
Contributor Author

pm100 commented Oct 20, 2023

Some kind of system test is easy to implement for this crate because we can use the test backend (or create our own dummy backend). The problem is that I have no time to implement the test cases.

Having done this on another project I think 'easy' is a little optimistic, certainly doable tho :)

@joshka
Copy link
Contributor

joshka commented Oct 20, 2023

In general, for testing ratatui widget rendering, I'd recommend avoiding using the TestBackend and prefer to save a few steps for every test and hit the buffer directly. The assert_buffer_eq! macro leads to a unit testing approach as the output is intentionally pretty similar to what you'd have to paste into your code to make the test run.

You might want just one TestBackend test though to ensure that everything works as expected, but the majority of the tests could easily use the following approach. Many of the newer tests written in Ratatui use this approach:

  1. setup the widget params
  2. Create an empty buffer of the right size to render into
  3. call widget.render(buffer.area, &mut buffer);
  4. call:
    assert_buffer_eq!(buffer, Buffer::with_lines(vec![
        "expected contents of buffer",
        "line 2",
    ]);

Combined with an LLM it's pretty quick to crank these out fast.

rhysd pushed a commit that referenced this pull request Oct 20, 2023
* fix hard tab display

* fix clippy complaint

* fix tab_len == 0 case. Add tests

* oops commited wrong file.
@rhysd
Copy link
Owner

rhysd commented Oct 20, 2023

Having done this on another project I think 'easy' is a little optimistic, certainly doable tho :)

Yeah, maybe. It would be hard to maintain the tests as Rust code. I would prepare 3 files per each test case:

  • Text buffer before test
  • JSON file to specify TextArea configuration and text manipulations
  • Expected text buffer after test

@pm100 pm100 deleted the hardtab branch November 2, 2023 21:33
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.

3 participants