Creating a MenuBar::visible property

[npwoods]

This allows a menu bar to be invisible, but also to have menu shortcuts to function properly.

This is incomplete; when we're doing menus with the muda crate, the visible property is not honored.

For muda to be supported, it looks like some additional plumbing would have to happen in internal/core/window.rs as setup_menubar() is passed a vtable::VRc<MenuVTable>, and after lowering the menubar-visible property is on MenuBarImpl. While I'm not an expert here, it looks like something more substantial would need to be passed to setup_menubar() as it is not just necessary to pass the property, but the facilities required to respond to changes.

[npwoods]

@LeonMatthes i'm curious what can be done to pull this over the finish line. This works for the non-native menubar, but doesn't work correctly for Winit.

It isn't clear to me how to properly plumb changes to the menubar-visible property to the parameters of MudaAdapter::setup().

Thoughts?

[npwoods]

At first glance it appears that I have all of the correct plumbing in place, with the exception that we fail to call MudaAdapter::rebuild_menus() in response to visibility changes. I'm finding it challenging to figure out how to make the muda code be properly notified in these scenarios.

[LeonMatthes]

Hi @npwoods .
Thanks for pinging me about this again. I took another look at it and what still needs to happen is actually updating the visible property in the MenuFromItemTree::update_shadow_tree function.

Because currently the visible property will always be set to true for the native menu. It queries that data out of the menu from item tree and the menu from item tree as the name suggests must get the menu data points out of an instantiated item tree. It's sort of an adapter between the Slint item tree and the native menu bar items.

So take a look at the update_shadow_tree function and see if you can get the visible property out of the item tree there. You may have to pass the corresponding property in when constructing it like we do with the condition property.

[npwoods]

Findings:

1. I tried running this demonstration with my changes in slint-viewer, and neither MenuFromItemTree::update_shadow_tree() nor the evaluate_if_dirty() callback are executed when I try to toggle the menu bar
2. I found myself in the Builtin::SetupMenuBar clause in internal/compiler/generator/rust.rs, because it seems that I need to take the property reference passed to SetupMenuBar. The problem is that the condition seems to be first, and the existing code expects to access that under rest. Is this a pattern we want to continue?
3. Currently we have MenuFromItemTree::new() and MenuFromItemTree::new_with_condition(). I suspect that we don't want to permute this with MenuFromItemTree::new_with_menubar_visible() and MenuFromItemTree::new_with_menubar_visible_and_condition()

Of course, this is assuming that I'm not completely off base.

[LeonMatthes]

You will probably have to call update_from_shadow_tree in the visible getter before returning the value.

But at the moment, that won't currently evalute the closure passed to tracker.evaluate_if_dirty.
That is just because we're not yet reading the visible property of the menubar.
As soon as you read that property within the property tracker, it will be tracked by the property tracker and it will correctly re-evaluate.

But you're right that likely you have to pass the visible property to the SetupMenubar function.

So that we don't get an explosion of new functions, can you try just using a new function with options instead (e.g. fn new(item_tree, condition: Option<...>, visible: Option<...>))

[npwoods]

I think I got something that works!

Some misgivings/concerns:

1. I still don't quite understand how properties seem to be magically tracked when evaluate_if_dirty() is invoked; I could be Cargo Culting
2. I'm not particularly crazy about parameterizing Option::None with <fn() -> bool>
3. Is pushing Expression::BoolLiteral(true) the correct thing to do if condition or visible is not specified?
4. I have not touched the C++ generator

I trust the audience of this PR will be able to evaluate this work.

[codecov]

Codecov Report

❌ Patch coverage is 92.85714% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (master@f6ad896). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
internal/compiler/passes/lower_menus.rs	92.85%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #11272   +/-   ##
=========================================
  Coverage          ?   73.07%           
=========================================
  Files             ?       98           
  Lines             ?    29086           
  Branches          ?        0           
=========================================
  Hits              ?    21255           
  Misses            ?     7831           
  Partials          ?        0           

Flag	Coverage Δ
slint-sc	73.07% <92.85%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[LeonMatthes]

Hey @npwoods , this looks good, you just need to adjust the interpreter and C++ generator to use the same basic approach.

With your current approach of pushing BoolLiteral(true), we could also consider using fn()->bool instead of Option<fn() -> bool> to reduce the special casing. I'll leave that decision up to you.

1. I still don't quite understand how properties seem to be magically tracked when `evaluate_if_dirty()` is invoked; I could be Cargo Culting

When Slint is evaluating a binding, it will set a thread local variable to indicate, we're now evaluating the binding for this property. And then all other properties that are accessed when evaluating the binding are automatically marked as dependencies of the current binding.
This is also how bindings are implemented in the slint language itself. They just boil down to Rust properties. They are implemented in properties.rs if you're interested in the code.

2. I'm not particularly crazy about parameterizing `Option::None` with `<fn() -> bool>`

I don't really see a problem with this, as long as we can make it work with C++.
As we're now always pushing the string literals onto the argument list, we could alternatively just make it a fn()->bool and just have that evaluate the bool literal.

3. Is pushing `Expression::BoolLiteral(true)` the correct thing to do if `condition` or `visible` is not specified?> 

yes, visible defaults to true, that's fine.

4. I have not touched the C++ generator

This will of course need to be implemented before we can merge this :)
Are you comfortable implementing the C++ side as well?

[npwoods]

Pushed some updates!

I don't really see a problem with this, as long as we can make it work with C++.
As we're now always pushing the string literals onto the argument list, we could alternatively just make it a fn()->bool and just have that evaluate the bool literal.

If you guys are comfortable with this, I am too ;-)

This will of course need to be implemented before we can merge this :) Are you comfortable implementing the C++ side as well?

I'm comfortable attempting to do so; hence my latest push. But full disclosure - I'm cargo-culting it.

As we're now always pushing the string literals onto the argument list, we could alternatively just remove the Option and just return a fn()->bool and evaluates the bool literal.

I suppose that I could update the various handlers for BuiltinFunction::SetupMenuBar to always expect Expression::BoolLiteral for both condition and visible, and create a method MenuFromItemTree::new_with_condition_and_visible() that does not take Option. I can try this, but wouldn't condition and visible still need to be lambas, because I don't know what the callbacks would look like? Your call

[LeonMatthes]

I suppose that I could update the various handlers for BuiltinFunction::SetupMenuBar to always expect Expression::BoolLiteral for both condition and visible, and create a method MenuFromItemTree::new_with_condition_and_visible() that does not take Option. I can try this, but wouldn't condition and visible still need to be lambas, because I don't know what the callbacks would look like? Your call

Yes, they would still need to be lambdas, but no longer optional.

Let's give this a try without the optionals, that feels much cleaner than having to special case a specific literal for saving a small property allocation.

[npwoods]

Sounds good - committed something 🤞

I don't really like the dummy lambdas that need to be present to serve other scenarios like the system tray; LMK how you want this handled

[npwoods]

Good catch - I believe I fixed it

[LeonMatthes]

Hey @npwoods ,
this looks good to me now implementation-wise 🙌

Can you please add a test for this in both Rust and C++?
Maybe a test that checks that a menubar accepts a shortcut if it is visible, then make it invisible, test that the window now has a larger inner size (as the menubar has disappeared), and that the shortcut still works.

Either add it in tests/cases/elements/menubar_shortcut.slint directly, or in a new test file for the visible property.

[LeonMatthes]

Thank you for the contribution @npwoods :)