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

feat: configure node open state symbols #19

Merged
merged 4 commits into from
May 19, 2023
Merged

Conversation

EdJoPaTo
Copy link
Owner

This is a base implementation for configuration of symbols which works but isn't finished yet.

closes #17

The current state raises some questions:

  • What happens on different width symbols? Should that be handled in some way?
  • How to do the methods setting these? The default is already Some(…) and the other methods default to None so overriding and setting it will always be something afterwards. Change the other methods to accept Option<&str> too?

@dzfrias
Copy link
Contributor

dzfrias commented Mar 21, 2023

To handle different width symbols, we could restrict node_*_symbol fields to chars and add a trailing space to them in StatefulWidget::render. This would obviously limit the user options a bit but it makes sense if we want to keep indentation consistent. I think the trailing space would be better to add in by default anyway.

I also think we could default node_no_children_symbol to None. Combined with the trailing space stuff, we could do unwrap_or(" ") instead. This would make the width of all symbols 1 (which could be further enforced with char).

Maybe we could even shift completely from using Options and instead just let them pass in chars. If they passed in ' ', it would be the same as resetting it. This seems a bit more idiomatic when looking at tui-rs's builtin widgets. However, tui-logger use the Option pattern here.

Personally, I think moving from Options and going to chars is the best way to go. It hides as little as possible from the user, and eliminates the possibility of setting the symbols to a super long string and breaking the indentation.

@EdJoPaTo
Copy link
Owner Author

I think allowing to have "" without a space afterwards might also be of interest. If someone wants to customize then this will allow even more freedom there.

Also its one less creation of a String. When redrawing often its definitely a benefit of being a bit more efficient when possible.

Guess if someone wants equal sized values then they can just use the same length themselves without some check in there.
So I think your suggestion of defaulting to String instead of Option<String> is a good idea here.

Side Note: The code is closely matched to the List Widget which also uses Option<String> for the highlight_symbol. (At least it did when I created this Widget)

@dzfrias
Copy link
Contributor

dzfrias commented Mar 21, 2023

Good point. I think both ideas, char and String, are good. Whatever you think is best!

@joshka
Copy link

joshka commented Apr 29, 2023

QQ: what's left to do on this (asking as it probably should be done before moving tui-rs-tree-widget into ratatui. Is it making some decisions or actual changes / implementation?

@EdJoPaTo
Copy link
Owner Author

Currently the variables are available in the code but there is no way to set them as a user of the lib.
tui traditionally has setter methods. The other way would be just marking these variables as pub which might be simpler code wise but would be breaking in case of future things needed on setting them.

Other than that it’s done I think.

@EdJoPaTo EdJoPaTo marked this pull request as ready for review May 19, 2023 23:29
@EdJoPaTo EdJoPaTo merged commit 3e92570 into main May 19, 2023
@EdJoPaTo EdJoPaTo deleted the configure-node-open-symbols branch May 19, 2023 23:30
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.

Add ability to change icons
3 participants