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

feat: implement set_menu for system tray #106

Merged
merged 11 commits into from
Jul 14, 2021

Conversation

fnune
Copy link
Contributor

@fnune fnune commented Jun 25, 2021

What kind of change does this PR introduce? (check at least one)

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Documentation
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes. Issue #___
  • No

The PR fulfills these requirements:

  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix: #xxx[,#xxx], where "xxx" is the issue number)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

Other information:

@fnune fnune requested a review from a team June 25, 2021 23:07
@fnune fnune mentioned this pull request Jun 25, 2021
3 tasks
@fnune
Copy link
Contributor Author

fnune commented Jun 25, 2021

I'm not sure if this works. I'm not sure how _window_target is being passed to these functions.

src/system_tray.rs Outdated Show resolved Hide resolved
@amrbashir amrbashir changed the title feat(linux): implement set_menu for system tray feat: implement set_menu for system tray Jul 1, 2021
@fnune
Copy link
Contributor Author

fnune commented Jul 2, 2021

Oh sweet! You're extending it. I'll have time to work on this tonight, or maybe over the weekend

@fnune fnune requested a review from a team July 2, 2021 11:51
@lemarier
Copy link
Member

lemarier commented Jul 2, 2021

Ok, so It works fine on Linux and macOS.

The only issue we have is Windows, once we set the menu the tray menu stops working.

Gonna try to dig in. @amrbashir you have an idea?

@amrbashir
Copy link
Member

amrbashir commented Jul 2, 2021

yeah I saw that issue yesterday, It was because the Menu gets dropped which calls DestroyMenu() and it destroys the HMENU stored in the SystemTray and the stash

@lemarier lemarier marked this pull request as draft July 6, 2021 12:04
@fnune
Copy link
Contributor Author

fnune commented Jul 14, 2021

You guys rock! I wasn't expecting you to just take over the PR 😄

@fnune fnune marked this pull request as ready for review July 14, 2021 15:05
@fnune fnune requested a review from a team as a code owner July 14, 2021 16:06
Copy link
Member

@amrbashir amrbashir left a comment

Choose a reason for hiding this comment

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

@lemarier Why disabling this for windows ?
I think it is fine that the menu is dropped, if they want to set a new menu, they probably need to keep it alive as possible either build it within the same lifetime of event_loop or just static lifetime.

@lemarier
Copy link
Member

@lemarier Why disabling this for windows ?
I think it is fine that the menu is dropped, if they want to set a new menu, they probably need to keep it alive as possible either build it within the same lifetime of event_loop or just static lifetime.

I wonder if we shouldn't handle it internally? Because it's not how macOS and Linux work, but you are right... maybe we can ship it anyway and see how it goes

@amrbashir
Copy link
Member

I wonder if we shouldn't handle it internally? Because it's not how macOS and Linux work, but you are right... maybe we can ship it anyway and see how it goes

Well, we can either add drop impl for linux and macOS or remove all drop impls.

@lemarier
Copy link
Member

I wonder if we shouldn't handle it internally? Because it's not how macOS and Linux work, but you are right... maybe we can ship it anyway and see how it goes

Well, we can either add drop impl for linux and macOS or remove all drop impls.

on windows did the menus are linked to the app? so when closed they are all dropped anyway?

@amrbashir
Copy link
Member

on windows did the menus are linked to the app? so when closed they are all dropped anyway?

I think so.

@lemarier lemarier merged commit 578dd23 into tauri-apps:dev Jul 14, 2021
@github-actions github-actions bot mentioned this pull request Jul 14, 2021
@lemarier
Copy link
Member

Thanks for your contribution @fnune we do appreciate it a lot!

I'm sure the community will enjoy this new feature.

Have a great day!

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.

3 participants