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

[WIP] Add support for "password" TextEdit and TextBox widgets #964

Closed

Conversation

daboross
Copy link
Contributor

@daboross daboross commented Mar 22, 2017

The way I've done it here technically works, but seems very hacky, and probably could be designed and written better.

There are a few places in TextEdit where &text is replaced with &replacement_string.as_ref().unwrap_or(&text), which is a bit bulky, but I'm not sure if there's a better way to do this while still having the ability to mutate text in the way this PR currently implements it.

I've tested this with an example text box, and it seems to support dragging, selecting, deleting, and correctly treats the entire password as a single word for the sake of ctrl+left, ctrl+right and ctrl+backspace. I can look into adding some actual unit tests for this once the design is improved more.

@mitchmindtree
Copy link
Contributor

Hey @daboross, thanks for the PR!

I'm not sure how I feel about adding this into TextEdit directly. It does add quite a bit more work (especially for large texts) and adds a little more complexity. That said, adding a unique type that was capable of this would probably require quite a bit of code duplication from the TextEdit widget. Maybe some of the TextEdit logic could be abstracted into a conrod::text::edit module though which could be shared between TextEdit and this new char replacer?

I think I'd like to think about our options a little longer before going ahead with this. I can definitely see this functionality being useful though!

@daboross
Copy link
Contributor Author

Abstraction sounds like a good idea, I'll see what I can pull out.

Do you think, if we add this as a separate widget, we would want two ones to match TextEdit and TextBox, or just a TextBox equivalent? I'm not sure what the use case really is for a multi-line hidden text widget is, but it might want to be included for completeness?

@Drakulix
Copy link
Contributor

Any news on this? I would have a use case for this feature.

@daboross
Copy link
Contributor Author

Sorry! I've been a bit busy with other things, I definitely want to keep working on thin but my work with Conrad has been for a hobby project so far.

Everything I've done for this PR is currently published, if you want to take a stab at making this a separate widget and abstracting out shared functionality, feel free to! I'll get back to this eventually but it may be a few weeks.

@Drakulix
Copy link
Contributor

@daboross Thanks for that quick response. I am not in a hurry about using this feature. I also just need it for a hobby project. I am busy enough myself right now, but if I find some time to tackle this, I will definitely ping you before I do, so no work here is wasted/replicated. Still even with a few weeks, you might be quicker ;)

@daboross
Copy link
Contributor Author

I feel like this could be made a lot simpler if TextEdit and TextBox simply had two things separated out into an accessible thing: calculating what changes are made to text, and displaying text.

Kind of like the note in text_edit.rs:

    // TODO: We should create a more specific `Event` type that:
    // - Allows for mutating an existing `String` directly
    // - Enumerates possible mutations (i.e. InsertChar, RemoveCharRange, etc).
    // - Enumerates cursor movement and range selection.

Having something (maybe an iterator wrapper over the events iterator?) that just turned a "mouse/key state" + "events" into "text changes" would be super helpful as an abstraction to work under both TextEdit, TextBox and a PasswordBox.

The other thing needed would just be something generic to display some text + a selection from index1 to index2 in that text.

It should be possible to make these abstraction - but I haven't gotten a working thing at all yet. If this was done, I think it could also mean TextBox could just handle the events itself, rather than having an inner TextEdit.

@daboross daboross force-pushed the add-password-text-boxes branch from 0b9b71e to 7bcb6f0 Compare May 21, 2017 22:34
@daboross daboross force-pushed the add-password-text-boxes branch from 7bcb6f0 to ee43384 Compare June 17, 2017 18:06
@daboross daboross force-pushed the add-password-text-boxes branch from ee43384 to 7afb4ce Compare July 13, 2017 07:05
This works, but may definitely want to be designed and implemented in a different way, as adding this in does seem a bit hacky.
@daboross
Copy link
Contributor Author

Closing this, and opening a new PR for the event-based design. I feel like it's different enough from the ad-hoc password support implemented here to warrant a new PR.

See: #1137.

@daboross daboross closed this Feb 16, 2018
@daboross daboross deleted the add-password-text-boxes branch February 16, 2018 01:05
@daboross daboross restored the add-password-text-boxes branch February 16, 2018 01:05
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