-
Notifications
You must be signed in to change notification settings - Fork 543
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
Fix compiler panic when using %
size in a flickable
#4223
base: master
Are you sure you want to change the base?
Conversation
The viewport of a flickable is of ElementType::Native, and `lookup_property` don't query the builtin reserved properties in that case. This commit fix the assert by allowing Type::Invalid as well. Fixes #4163
The question is if we should not change the meading of 100% to be the "semantic" parent: So to be the size of the flickable and not the viewport (see test) |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
width: 100%; | ||
height: 100%; | ||
|
||
ShowResult {} |
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 suppose this part is not needed for the automated test?
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 is the original testcase. Not needed but good to have in addition as it is a bit more complex.
} | ||
|
||
out property <bool> test: | ||
// The % applies to the viewport |
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.
Note that our terminology is wrong: the viewport is the "portion" that's visible. So the % applies to the content width, not the viewport width - while parent refers to the content.
The inconsistency is not good.
Since the % asserts right now, I guess aligning it with parent would be the "compatible" fix.
I'm inclined to say that parent referring to the content size would make more sense to me, as the Flickable can always be referred to by name. But I think that would be a breaking change.
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.
Note that our terminology is wrong: the viewport is the "portion" that's visible. So the % applies to the content width, not the viewport width - while parent refers to the content.
The inconsistency is not good.
parent doesn't refer to the content, it refer to the flickable itself.
"content" and "viewport" are two name for the same things. We seem to use viewport consistently.
But it's true that you already filled an issue to replace that. #1443
But that's out of scope of this PR
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.
Since the % asserts right now, I guess aligning it with parent would be the "compatible" fix.
It asserts in debug mode in Slint 1.3.x But it used to work in 1.2
But yes, i agree we can still do this change if we want.
I'm inclined to say that parent referring to the content size would make more sense to me, as the Flickable can always be referred to by name. But I think that would be a breaking change.
I'm not sure i understand this.
What is the "content size"? parent
doesn't refer to a size, it refer to an element.
And parent refer to the parent element in the AST as it is resolved. So you can say parent.interactive
or parent.foobar
if there is a foobar property declared in the flickable.
But currently % refer to the viewport size which is the same as the content size isn't it?
So you think that's how it should be and that the current behavior is correct?
width: 100%; | ||
height: 100%; | ||
|
||
ShowResult {} |
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 is the original testcase. Not needed but good to have in addition as it is a bit more complex.
} | ||
|
||
out property <bool> test: | ||
// The % applies to the viewport |
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.
Note that our terminology is wrong: the viewport is the "portion" that's visible. So the % applies to the content width, not the viewport width - while parent refers to the content.
The inconsistency is not good.
parent doesn't refer to the content, it refer to the flickable itself.
"content" and "viewport" are two name for the same things. We seem to use viewport consistently.
But it's true that you already filled an issue to replace that. #1443
But that's out of scope of this PR
Oops, I made indeed a mistake in terminology and expression. I thought about it again. I think it's good that parent refers to the Flickable (instead of that invisible Rect "inside"). That's consistent. I can't imagine what a mess it would be to document and explain that In terms of use cases for parent here I'm a bit at a loss. I'd imagine a lot more folks wanting to express sizes relative to the content width/height (what's currently called |
The viewport of a flickable is of ElementType::Native, and
lookup_property
don't query the builtin reserved properties in that case.This commit fix the assert by allowing Type::Invalid as well.
Fixes #4163