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

Support default-font-* properties in Live-Preview #7011

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

tronical
Copy link
Member

@tronical tronical commented Dec 5, 2024

... by changing the resolution for the WindowItem to traverse the item tree from the current item, instead of going to the window.

This doesn't quite fix #4298 because rem resolution is still missing. That requires the built-in default font size function to be fixed as well, which is non-trivial.

cc #4298

@tronical tronical requested a review from ogoffart December 5, 2024 21:03
... by changing the resolution for the `WindowItem` to traverse the
item tree from the current item, instead of going to the window.

This doesn't quite fix #4298 because `rem` resolution is still missing.
That requires the built-in default font size function to be fixed as
well, which is non-trivial.

cc #4298
Comment on lines +129 to +130
self_component: &vtable::VRc<ItemTreeVTable>,
self_index: u32,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not self_rc: &ItemRc, like the other functions in this vtable have?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, this is quite annoying. The issue is that &ItemRc translates to *const ItemRc in the C signature. This is the only function that's called from the C++ code straight into the vtable where a &ItemRc is needed. But because this takes a pointer, I can't write ...->layout_info(..., &ItemRc{{ ... }) as that's taking the address of a temporary. In all the other places in the FFI we also pass this "pair".

I could alternatively change this to &ItemRc but then on the C++ code call a helper function in our private header files that stores the ItemRc on the stack and passes the address. Would you prefer that?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could alternatively change this to &ItemRc but then on the C++ code call a helper function in our private header files that stores the ItemRc on the stack and passes the address. Would you prefer that?

Yes, I think it is cleaner, don't you thik?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's indeed cleaner.

@ogoffart
Copy link
Member

ogoffart commented Dec 6, 2024

No tests?

Could you make a test that involve a PopupWindow. I have a suspicion that it may not work because the PopupWindow inject a WindowItem that doesn't define these default-font-* properties.

eg:

export component TestCase inherits Window {
   default-font-size: 140px;
   PopupWindow {
      // the font size of this text must be large
      Text { text: "hello"; } 
   }
}
``

@tronical
Copy link
Member Author

tronical commented Dec 6, 2024

No tests?

Could you make a test that involve a PopupWindow. I have a suspicion that it may not work because the PopupWindow inject a WindowItem that doesn't define these default-font-* properties.

eg:

export component TestCase inherits Window {
   default-font-size: 140px;
   PopupWindow {
      // the font size of this text must be large
      Text { text: "hello"; } 
   }
}
``

That's an excellent point - this is indeed also not handled (besides the rem issue).

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.

default-font-size and default-font-family don't work in the Live-Preview
2 participants