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

add default menu bar in dioxus-desktop to resolve #1691 #1696

Merged
merged 6 commits into from
Jan 4, 2024

Conversation

pascalbehmenburg
Copy link
Contributor

Feature: add default menu bar to dioxus-desktop

It looks like this on macOS (please ignore firefox underneath it):
image image

This also resolves #1691 since, as mentioned there, macOS requires menu bar items with keybindings assigned to them to enable system default text editing and window shortcut behaviors.

Solution

The feature was implemented by adding a function fn create_default_menu_bar() -> MenuBar in webview.rs which constructs a platform specific menu bar that may be passed into window_builder if disable_default_menu_bar is false which is defined in config.rs to enable the option to opt out of having a menu bar at all. diasable_default_menu_bar is false by default, and therefore the window_builder will use the default menu bar if the user does not opt out, as implemented in webview.

I think this should be exactly what @ealmloff described in his comment.

clippy and all tests including playwright passed on master.

Note: due to tao not supporting a lot of native menu bar items on Linux, there is no edit menu on Linux yet.

Example usage

Cargo.toml:

dioxus = { path = "../dioxus/packages/dioxus/" }
dioxus-desktop = { path = "../dioxus/packages/desktop/" }

main.rs:

use dioxus::prelude::*;

fn main() {
    dioxus_desktop::launch_cfg(
        App,
        dioxus_desktop::Config::new()
            .with_disable_default_menu_bar(false)
    );
}

fn App(cx: Scope) -> Element {
    render! {
        input {
            r#type: "text",
            placeholder: "Enter smth. and use Ctrl + A / Cmd + A...",
        }
    }
}

fixup naming and expose disable_default_menu_bar with builder function
@ealmloff ealmloff added enhancement New feature or request desktop Suggestions related to the desktop renderer labels Dec 6, 2023
Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

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

I have one small question about a function name, but overall looks great! This will help make desktop apps work more like I would expect out of box. (I also really appreciate the detailed PR description 🙂)

packages/desktop/src/cfg.rs Outdated Show resolved Hide resolved
@jkelleyrtp
Copy link
Member

Agreed, looks great, definitely something we want to support. It might be worth to make the function that builds the menubar public and then change the naming scheme to what @ealmloff mentioned. Other than that, LGTM!

@jkelleyrtp jkelleyrtp self-requested a review December 7, 2023 05:05
@esimkowitz
Copy link
Contributor

Do we want this to be the default behavior on all platforms or only on Mac? I haven't been targeting Windows or Linux recently but if I remember correctly they weren't impacted by the keybindings issue

@pascalbehmenburg
Copy link
Contributor Author

Do we want this to be the default behavior on all platforms or only on Mac? I haven't been targeting Windows or Linux recently but if I remember correctly they weren't impacted by the keybindings issue

Actually, I don't think it is an issue on Linux or Windows, though I haven't been able to test it either. Otherwise, I do think it won't be bad to have those very basic menus included on these platforms too. It is more consistent that way, otherwise one might wonder where the menus went when cross-compiling or similar.

Agreed, looks great, definitely something we want to support. It might be worth to make the function that builds the menubar public and then change the naming scheme to what @ealmloff mentioned. Other than that, LGTM!

So you basically want to expose the standard menu bar that was built so that one could adjust it further instead of having to re-create a new Menubar when intending to customize the existing one? I think that would be nice, but I am not too sure where to put it. I thought about exposing it and accepting a MenuBar instead of a bool, but MenuBar does not implement copy / clone. Also, I think no one would expect it to be found in webview.rs when using it from the outside. If you have an idea on that I would be really happy about adjusting it, otherwise I'd suggest keeping it the way it is with the incoming naming changes and opening an issue about exposing it where we can discuss this further.

…xplanatory

Note:
This is the actual correct commit. The previous one contained files touched by cargo fmt which are unrelated. Sorry for that.
@pascalbehmenburg
Copy link
Contributor Author

@ealmloff I think this should be ready to be merged now, please check my commit history though since I had a huge fuckup in there where I committed 90 files on accident which were touched by cargo fmt --all. I reverted the commit but since I am not that used to more advanced git commands it would be nice of you to give a look. Also, if possible, squashing the commits into one wouldn't be that bad, I think.

Copy link
Member

@ealmloff ealmloff left a comment

Choose a reason for hiding this comment

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

Thank you!

@ealmloff
Copy link
Member

ealmloff commented Dec 7, 2023

It might be better to wait to merge this until the last 0.4 version goes out. If you already have the system menu added manually, it wouldn't be great to add a second menu even if the API isn't technically breaking

@jkelleyrtp jkelleyrtp merged commit 3498396 into DioxusLabs:master Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
desktop Suggestions related to the desktop renderer enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some macOS keybindings do not work (Copy, Paste, Select all)
4 participants