slint-ui / slint

Slint is a declarative GUI toolkit to build native user interfaces for Rust, C++, or JavaScript apps.
https://slint.dev
Other
16.93k stars 568 forks source link

Refactoring: remove geometry properties from the runtime `Item`s #1932

Closed ogoffart closed 1 year ago

ogoffart commented 1 year ago

Almost all items have the geometry property in them, and they use that to re-implement the Item::geometry But having properties for each item in the item itself means that it cannot be optimized (inlined or removed when it is just a constant) (and it makes fixing https://github.com/slint-ui/slint/issues/1072 a bit harder.)

The idea is to

Another benefit will be that there will be less query of the geometry properties since the runtime will query it once and pass it to the render function.

ogoffart commented 1 year ago

Steps from #2340:

  1. Add a new fn item_geometry(item_index: usize) -> Rect function in the Component vtable
  2. Add a geometry parameter to all functions in the ItemVTable that needs it.
  3. Remove the geometry function from the ItemVTable
  4. Remove the x, y, width, height from all the items.
Guiguiprim commented 1 year ago

I was looking at this (looked like a good issue to seriously discover the internals of slint).

One thing I see that might make the 4 steps you describe fall down is that PartialRenderer::filter_item() uses a PropertyTracker around the call to geometry() when accessing his cache. Giving the geometry directly to the function (as it would no longer be accessible from the item itself) would break this property tracking. I haven't seen if this issue that can/would be catch higher up the stack, but from where I stand for now it looks like a nice obstacle on the path.

ogoffart commented 1 year ago

I do have some work in progress on that (which i now just pushed in the olivier/wip/item_geometry) That branch get rid of the geometry() already. Now all is there to do is to remove the properties from the items where it can be removed, and then compared to see if this change is actually good.

Guiguiprim commented 1 year ago

So ItemRc that I had not seen yet was the answer here ! :smile:

Guiguiprim commented 1 year ago

I'm trying to finish this based on your work, but I'm hitting this wall: WindowInner::set_window_item_geometry() when removing height and width from WindowItem.

I'm not really sure how it should be carried on:

ogoffart commented 1 year ago

It's ok if some item keep their with and height. I think the Window, and maybe the Text and TextInput might need to keep their width and height because the runtime need it for other things.

That said, an api in the Component to resize is not a bad idea at all.

But for now i think it'd be easier to keep the width and height on the Window.

Note that once this is done, we can then compare the branch with master and see how this compares in terms of binary size, compilation time, and memory usage. My intuition tells me that this will save memory. And perhaps binary size, depending on how good the property can be optimized.

Guiguiprim commented 1 year ago

I'm almost there I think https://github.com/Guiguiprim/slint/tree/item_geometry

But I have a bug with Flickable item that might be related to @ogoffart commit Flickable: don't make the viewport special.

The generated code for the flickable viewport width (and other) looks weird. But I'm having a hard time tracking the exact issue.

new code on the left, old on the right:

image

I also get this empty item that is use nowhere:

image

EDIT:

This is code generated for:

import { HorizontalBox, GroupBox, TextEdit } from "std-widgets.slint";

export component App inherits Window {
  TextEdit {
    text: @tr("This is our TextEdit widget, which allows for editing text that spans over multiple paragraphs.\nFor example this line starts in a new paragraph.\n\nWhen the amount of lines - due to wrapping and number of paragraphs - exceeds the available vertical height, a vertical scrollbar is shown that allows scrolling.\nYou may want to enter a bit of text here then in order to make them visible.");
    wrap: word-wrap;
  }
}
Guiguiprim commented 1 year ago

So after more investigation the issue does not come from this commit but from the following work. Investigation in progress...