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

password mode #29

Closed
wants to merge 1 commit into from
Closed

password mode #29

wants to merge 1 commit into from

Conversation

pm100
Copy link
Contributor

@pm100 pm100 commented Oct 11, 2023

A new feature, password mode. This masks the input with *****. Sample added

@joshka
Copy link
Contributor

joshka commented Oct 12, 2023

Could this possibly use text::Masked introduced in ratatui/ratatui#168?

@pm100
Copy link
Contributor Author

pm100 commented Oct 12, 2023

I had not seen that, let me check it out

@pm100
Copy link
Contributor Author

pm100 commented Oct 12, 2023

the most obvious thing is that its only ratatui. My vote would be to drop support of tui since this will increasingly be an issue

@joshka
Copy link
Contributor

joshka commented Oct 12, 2023

Ah yeah. Makes sense.

@pm100
Copy link
Contributor Author

pm100 commented Oct 12, 2023

@joshka - well its a simple change to make it use Masked (less code), but then, as I said, it now gets complicated. I either have 2 implementations, or I make it a 'feature' that depends on ratatui. Neither seems a good choice. I will post an issue about dropping tui support.

@pm100 pm100 mentioned this pull request Oct 12, 2023
Copy link
Contributor

@joshka joshka left a comment

Choose a reason for hiding this comment

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

I'd suggest adding unit tests, doc comments, and considering how this interacts with the other features. Does this break line numbering, placeholders, ...?

Comment on lines +92 to +93
// big enough for the largest line I can imagine
const STARS: &str = "******************************\
Copy link
Contributor

Choose a reason for hiding this comment

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

This is probably not a good idea as things can be longer than you'd imagine ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its per line. And thats 180 chars. I could not imagine a line longer than that, the UI still works in that case.

Copy link
Contributor Author

@pm100 pm100 Oct 12, 2023

Choose a reason for hiding this comment

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

line numbers and placeholder works fine (I admit i did not test it till just now , which is dumb since i wrote the placeholder feature)

Comment on lines -93 to +106
lines.push(self.0.line_spans(line.as_str(), top_row + i, lnum_len));
let mut thisline = line.as_str();
if self.0.password_mode {
let count = thisline.chars().count();
thisline = &STARS[0..cmp::min(count, STARSLEN)];
}
lines.push(self.0.line_spans(thisline, top_row + i, lnum_len));
Copy link
Contributor

Choose a reason for hiding this comment

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

consider perhaps:

let line = if self.0.password_mode {
    "*".repeat(line.len())
} else {
    line
};

Note: this probably should do unicode width() rather than len(), but I don't see that in the library already.

Copy link
Contributor Author

@pm100 pm100 Oct 12, 2023

Choose a reason for hiding this comment

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

Oh I tried that. But the returned Text object is marshalling &strs. So the strings must persist after the function returns. FYI - see here. https://stackoverflow.com/questions/77269247/trying-to-subsitiute-text-of-variable-size

@pm100 pm100 closed this Oct 13, 2023
@pm100
Copy link
Contributor Author

pm100 commented Oct 13, 2023

WIll redo as a conditional feature depending on ratatui

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