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

Add :character-info command #4000

Merged
merged 3 commits into from
Feb 3, 2023

Conversation

wetheredge
Copy link
Contributor

@wetheredge wetheredge commented Sep 27, 2022

This is my take on Vim's get ascii (ga) / get utf8 (g8) command. This works on graphemes not characters, so it correctly handles things like emoji with skin tone modifiers or characters with combining diacritics.

I'm open to opinions on what the output should look like. Atm, it outputs the decimal value only for single byte characters in an ascii-compatible encoding. For UTF-8, it also prints the decoded codepoints. (The decoder is based on fasterthanlime's article since I could not find anything in std or already in helix, or on crates.io for that matter, and it's pretty concise).

Character Encoding Output
a UTF-8 "a" (U+0061) Dec 97 Hex 61
a windows-1252 / extended ascii "a" Dec 97 Hex 61
newline UTF-8 "\n" (U+000a) Dec 10 Hex 0a
CRLF UTF-8 "\r\n" (U+000d U+000a) Hex 0d + 0a
é UTF-8 "é" (U+00e9) Hex c3 a9
é windows-1252 "é" Hex e9
UTF-8 "ë" (U+0065 U+0308) Hex 65 + cc 88
👍🏽 UTF-8 "👍🏽" (U+1f44d U+1f3fd) Hex f0 9f 91 8d + f0 9f 8f bd

Remaining tasks:

  • Improve the help text
  • Replace Debug formatting of character with something more appropriate. Debug handles escaping non-printable & whitespace characters, but it also escapes the combining diaeresis on the ë…
    • Now the only escapes are \0, \t, \n, and \r
  • Tests

I'm not familiar with the codebase, so let me know if there's anything that could be more idiomatic. It does not have any tests yet, but it looks like most commands don't?

closes #3885

@the-mikedavis
Copy link
Member

Instead of a regular command which must be bound to a key, I think this would be appropriate as a typable command (helix-term/src/commands/typed.rs). We could follow Vim and call it :ascii (aliased to :as) or come up with a new name

@kirawi kirawi added A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer. labels Sep 27, 2022
@wetheredge
Copy link
Contributor Author

Got it switched over to a typable command. I've named it :character (aliased to :char) since it should support any encoding, even if it's not ASCII compatible.

@sudormrfbin
Copy link
Member

Maybe something a bit more descriptive like :character-info?

@wetheredge wetheredge changed the title Add get_character command Add :character-info command Oct 1, 2022
@wetheredge wetheredge marked this pull request as ready for review October 1, 2022 17:39
@wetheredge
Copy link
Contributor Author

Rebased to fix conflicts. Let me know if I should write tests, otherwise it should be ready

@the-mikedavis
Copy link
Member

the-mikedavis commented Oct 29, 2022

I think this is looking great 😀

You can add an integration test like so:

// helix-term/tests/test/commands.rs
#[tokio::test(flavor = "multi_thread")]
async fn test_character_info() -> anyhow::Result<()> {
    test_key_sequence(
        &mut helpers::AppBuilder::new().build()?,
        Some("ih<esc>h:char<ret>"),
        Some(&|app| {
            assert_eq!(r#""h" (U+0068) Dec 104 Hex 68"#, app.editor.get_status().unwrap().0);
        }),
        false,
    )
    .await?;

    Ok(())
}

(you will want to rebase on latest master first since there are recent changes to the integration testing harness)

@wetheredge
Copy link
Contributor Author

Thanks for the pointer on the tests. I added that and a few more cases to fully cover the possible output formats.

@wetheredge
Copy link
Contributor Author

wetheredge commented Dec 7, 2022

@archseer, sorry for the ping, but is there any chance of this making it into 22.12? I can rebase if that would be helpful.

Oops too late there on my part 😅

@kirawi kirawi self-requested a review January 18, 2023 15:46
@wetheredge
Copy link
Contributor Author

Should I resolve the conflict? Looks like it's just accepting both the test I added & a couple new ones from master.

@archseer
Copy link
Member

archseer commented Feb 2, 2023

Sorry for the delay! Looks good to me, just needs a rebase :)

@the-mikedavis the-mikedavis merged commit f7bd7b5 into helix-editor:master Feb 3, 2023
@wetheredge wetheredge deleted the get-character branch February 3, 2023 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-review Status: Awaiting review from a maintainer.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Vim’s “get ascii” command
6 participants