Avoid overwrite ListView viewport size#12264
Conversation
cc7f6ad to
63bfb7a
Compare
ogoffart
left a comment
ogoffart
left a comment
There was a problem hiding this comment.
Thanks for the PR.
I added some comments.
But please resolve the conflicts so we can see the CI results.
| let vp_w_arg = if component.prop_analysis | ||
| .get(&llr::MemberReference::from(listview.viewport_width.clone())) | ||
| .and_then(|a| a.property_init) | ||
| .is_some() |
There was a problem hiding this comment.
this kind of logic probably belong in the LLR instead of cpp/rust.rs. In the LLR, we can change listview.viewport_width/height to Option<...>
There was a problem hiding this comment.
Could you point me another property on LLR similar to that so I can understand what is necessary to have it there? Changing pub viewport_height: Option<MemberReference>, causes a breakage on the API, and will require some function changes, wondering if that is what you expect
There was a problem hiding this comment.
Yes, this is private API, so that's fine to change
There was a problem hiding this comment.
I choose a different path since I need to keep the original property value on both cases make it a Option will discard the value in one of the cases, so instead I created a new info field for each viewport property.
There was a problem hiding this comment.
I need to keep the original property value on both cases
I might have missed something, but why do you need to keep the original value?
If there is a reason, then disregard my comment.
There was a problem hiding this comment.
update based on discussion
d5235ad to
665a55f
Compare
ogoffart
left a comment
ogoffart
left a comment
There was a problem hiding this comment.
Sorry for the delayed review. Please ping me to get faster review, otherwise I miss it.
| @@ -185,7 +178,12 @@ fn update_visible_instances( | |||
| ops.splice(0, ops.len(), 0); | |||
| viewport_height.set(zero); | |||
There was a problem hiding this comment.
I guess this line can be removed?
| viewport_height: Pin<&Property<LogicalLength>>, | ||
| viewport_height_is_fixed: bool, |
There was a problem hiding this comment.
Make this
| viewport_height_is_fixed: bool, | |
| viewport_width: Option<Pin<&Property<LogicalLength>>>, | |
| viewport_height: Option<Pin<&Property<LogicalLength>>>, |
and explain in the function documentation what these property are for: "when set, the viewport_with is set to ..."
| let vp_w_arg = if component.prop_analysis | ||
| .get(&llr::MemberReference::from(listview.viewport_width.clone())) | ||
| .and_then(|a| a.property_init) | ||
| .is_some() |
There was a problem hiding this comment.
I need to keep the original property value on both cases
I might have missed something, but why do you need to keep the original value?
If there is a reason, then disregard my comment.
…explicity set The view port properties width and height should not be modified if they are explicity set so we can skip storing reference to it on info structure Fixes: slint-ui#5485
If viewport-height is set we should be able to scroll until the end of it even if the items does not fit it. Fixes: slint-ui#5485
2 participants
When the viewport size is explicitly set, the values must not be overwrite when resizing teh listview.
Fixes: #5485