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

Added non printable characters rendering #9

Conversation

RaresCon
Copy link

@RaresCon RaresCon commented Jan 16, 2023

I added non printable characters rendering using the new key bind CTRL+P, that also lets you hide them. Now whitespaces (·), tabs (├──┤), linefeeds () and other control characters will be rendered (I encountered a problem here, some terminals are not in fact able to render UTF-8 characters, so I tested the code of the character that appears on render and all codes are correct). The problem now is that nightly formatting is messing around with all if statements and other declarations that I tried to write on a single line, so the number of lines is above limit by almost 50 lines. I tried modifying the fmt config file, adding other rules that would help reduce the number of lines, but I couldn't force it to format everything more compressed. Even though I can compress them by hand, the code won't pass the tests. Any tips for refactoring the code would be awesome.

Issue: #2

Non printable characters demo

Copy link

@alexandruradovici alexandruradovici left a comment

Choose a reason for hiding this comment

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

Some tips to delete some lines.

src/row.rs Outdated
Comment on lines 214 to 223
'\x0D' => rendered_char = '␍',
// null
'\x00' => rendered_char = '␀',
// bell
'\x07' => rendered_char = '␇',
// backspace
'\x08' => rendered_char = '␈',
// escape
'\x1B' => rendered_char = '␛',
_ => rendered_char = '?',

Choose a reason for hiding this comment

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

What about using a constant HashMap?

Copy link
Author

Choose a reason for hiding this comment

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

I tried to use it, but it doesn't compile as all methods for creating a HashMap are not const and other options like a sorted Map of characters would add too many lines.

src/config.rs Outdated
@@ -20,12 +20,20 @@ pub struct Config {
pub message_dur: Duration,
/// Whether to display line numbers.
pub show_line_num: bool,
/// Whether to display nonprintable characters.
pub non_printable: bool,

Choose a reason for hiding this comment

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

Use a shorter name here so that the default function fits into one line.

Copy link
Author

Choose a reason for hiding this comment

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

I think the formatting doesn't care about the length of the default function when there are too many arguments. I tried to shorten all names and it still moves them on a new line.

src/editor.rs Outdated
@@ -21,6 +21,7 @@ const PASTE: u8 = ctrl_key(b'V');
const DUPLICATE: u8 = ctrl_key(b'D');
const EXECUTE: u8 = ctrl_key(b'E');
const REMOVE_LINE: u8 = ctrl_key(b'R');
const NON_PRINTABLE: u8 = ctrl_key(b'P');

Choose a reason for hiding this comment

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

Use tuple assignments here like:

const (EXECUTE, REMOVE_LINE, NON_PRINTABLE): (u8, u8, u8) = (ctrl_key(b'E'), ctrl_key(b'R'), ctrl_key(b'P'));

@RaresCon RaresCon closed this Jan 28, 2023
@RaresCon RaresCon deleted the non_printable_characters branch January 28, 2023 20:08
@RaresCon RaresCon restored the non_printable_characters branch January 28, 2023 20:13
@RaresCon
Copy link
Author

I've seen that the number of lines for kibi is now 1023 (too close). I decided that I will try to refactor as much as possible the code to reduce the number of lines before trying to implement this feature. Thank you again!

@alexandruradovici
Copy link

Waiting for the refactoring pull request 😄

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.

2 participants