-
Notifications
You must be signed in to change notification settings - Fork 22
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
Adaptive height with animation #143
Conversation
let mut last_y = point.y as u32 | ||
- self.params.font_size as u32 | ||
- self.params.margin.bottom as u32 | ||
- ADDITIONAL_CAP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand why this constant is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't remember why I choosed ADDITIONAL_CAP=10
. But it is very little extra space just to make sure the last line fits in the window because of font
draw that can move letters lower for correct layout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it should be something proportional to font_size? E.g font_size / 2
) { | ||
use std::iter::once; | ||
|
||
state.process_entries(); | ||
state.process_entries(list_config.show_default); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something wrong with encapsulation here. List config is intended to be using only inside list_view.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but I think that state
is global app with possible access to all configs. Additionaly it is very intuitive to see show_default
parameter in [list_items]
section, on my opinion. But I can move it to global section of Config
if you want it.
|
||
// Hack for input_changed: If input buffer capacity is 0 then we do not typing | ||
// anything, so disable animation for first calls of draw (for support | ||
// ListItem::show_default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This hack is inside
input_changed
, right? - Why it's capacity and not length?
- Actually animation is added here, why it stated it's disabled then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this hack inside input_changed
. Length of string will be zero if we are typing something and delete it. But capacity will not be zero anymore because we don't free string. It is for first draw of yofi
.
Render:
- Run yofi
- Draw window with
height
fromConfig
- Try to place
list_items
in here - Truncate
height
to new height with removing unused height space. If it is just a first render we do not want to see this "jumping" window, but onlyinput
, thats why i added this
Thank you for review. I committed my changes and updated pr description. |
Sorry, I can't understand what is the problem. Did you turn on the animation and nothing happened?
There is one more param for height change: |
Yeah, the screenshot was a really dumb way to explain the issue, sorry. I did enable animation and it mostly works. demo.webm |
Thank you. I reproduced it myself with |
Can you check again with new changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demo.webm
Sorry for the late reply. I'm still able to reproduce the issue. You could use seq 15 | yofi dialog
for that.
let current_time = Instant::now(); | ||
|
||
for name in std::mem::take(&mut self.remove_later).into_iter() { | ||
self.animations.remove(&name).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could get a panic here, reproduction:
seq 100 | cargo r dialog
(debug build is important)
then quickly type any two digits
@@ -100,14 +108,19 @@ where | |||
(self.selected_item - self.skip_offset, self.skip_offset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether it's related but this piece of code above gives a panic now if running in debug build. So this code should work:
let displayed_items = (((space.height - margin.top - margin.bottom + item_spacing)
/ (entry_height + item_spacing)) as usize)
.saturating_sub(has_subname as usize);
let max_offset = self.skip_offset + displayed_items;
let (selected_item, skip_offset) = if self.selected_item < self.skip_offset {
(0, self.selected_item)
} else if max_offset <= self.selected_item {
(
displayed_items.saturating_sub(1),
self.skip_offset + (self.selected_item - max_offset) + 1,
)
} else {
(self.selected_item - self.skip_offset, self.skip_offset)
};
Sorry no time for this pr and no interest. If someone want to continue work on animations welcome to take my code :). I'm using my branch for adaptive height but without animation (turns out it looks good) |
Add animation.rs with some math for animations and new features:
Params
Global:
List items section
New animation section (if it is present, then animations are enabled)