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

Setting TypeRegister::enable_experimental on the start of compile_syntax_node() #7226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

npwoods
Copy link
Contributor

@npwoods npwoods commented Dec 28, 2024

Slint has many nifty experimental features coming (like menu bars) that need to be enabled when compiling by setting. While I was able to use this technique to experiment with menu bars, context menus did not function. After some debugging, it appears that that the way that way experimental features is enabled is by modifying the TypeRegister object in a few ad hoc ways, along these lines:

dependency_registry.borrow_mut().expose_internal_types =
            is_builtin || state.borrow().tl.compiler_config.enable_experimental;

I do not quite understand why these enablements seem to be sufficient for MenuBar but not ContextMenu but in any case adding analogous logic to compile_syntax_node() does the trick.

All this said, the ideal solution seems (to me) to likely be adding a parameter to TypeRegister::new(), but that would be a much more intrusive change.

…tax_node()

Slint has many nifty experimental features coming (like menu bars) that need to be enabled when compiling by setting.  While I was able to use this technique to experiment with menu bars, context menus did not function.  After some debugging, it appears that that the way that way experimental features is enabled is by modfying the `TypeRegister` object in a few ad hoc ways, along these lines:

````
dependency_registry.borrow_mut().expose_internal_types =
            is_builtin || state.borrow().tl.compiler_config.enable_experimental;
````

I do not quite understand why these enablements seem to be sufficient for `MenuBar` but not `ContextMenu` but in any case adding analogous logic to `compile_syntax_node()` does the trick.

All this said, the ideal solution seems (to me) to likely be adding a parameter to `TypeRegister::new()`, but that would be a much more intrusive change.
@ogoffart
Copy link
Member

Thanks for the PR.
I actually thought of doing something like that before. But as you can see, this breaks the CI because the syntax_test enable experimental feature, but internal types shouldn't be available.
We'd need to dissociate the concept of internal types and experimental feature.
(Or change the test)

@npwoods
Copy link
Contributor Author

npwoods commented Dec 30, 2024

Yeah I very quickly noticed the CI got broken :-(

I'd be happy to (hopefully not quixotically) try my hand at changing the constructor to something along the lines of:

#[derive(Default)]
pub struct TypeRegisterArgs {
    pub enable_experimental: bool
}

...
impl TypeRegister {
    ...
    pub fn new(args: TypeRegisterArgs) -> Self {
        ...

if you wish

@ogoffart
Copy link
Member

ogoffart commented Jan 3, 2025

What I meant is that we should differentiate internal components and experimental components.
I'm aware that this is a much bigger change.

But your patch is very sensible too. As I said, I remember trying exactly the same before.
It might make sense to try to just remove that test or make sure the experimental flag is not enabled for all syntax tests.

But overall, might not be worth it as this is all experimental. The ContextMenu is not finished anyway. Hopefully it will be enabled in the 1.10 release.

It's also great that you are willing to try experimental features, and your feedback would be welcome.

@npwoods
Copy link
Contributor Author

npwoods commented Jan 4, 2025

Of course, I defer to you on the proper way to handle experimental features.

In my application, I'm using the muda crate currently, which works great for Windows and probably would for macOS, but on Linux using a GTK+ menubar when the rest of the application is Slint doesn't seem workable. So for the time being, I'm taking my muda menubar structure and on Linux, I convert what can be converted to a Slint menubar. This approach seems to work for what Slint supports, and I'm looking forward to more comprehensive menubar support in Slint.

One problem I do have is I set up a child window and run another application in that child window (hence my other PR). I do have a problem where the Slint menus seem to be below the child window, rendering them unusable. While I suspect my particular use case is unusual, I would imagine that somebody trying to use OpenGL or Vulkan might run into the same problem (disclaimer: I really don't know much about 3D graphics APIs).

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.

2 participants