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(macos): add tabbing_identifier option, closes #2804, #3912 #5399

Merged
merged 7 commits into from
Oct 19, 2022

Conversation

caesar
Copy link
Contributor

@caesar caesar commented Oct 13, 2022

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Docs
  • New Binding issue #___
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes, and the changes were approved in issue #___
  • No

Checklist

  • When resolving issues, they are referenced in the PR's title (e.g fix: remove a typo, closes #___, #___)
  • A change file is added if any packages will require a version bump due to this PR per the instructions in the readme.
  • I have added a convincing reason for adding this feature, if necessary

Other information

Closes #2804, closes #3912

Uses the new APIs introduced in Tao in tauri-apps/tao#586 and tauri-apps/tao#592.

@caesar caesar requested a review from a team as a code owner October 13, 2022 10:31
@FabianLars
Copy link
Member

Is it just the Windows/Linux user in me or is this something that should be off by default (or always)?

@caesar
Copy link
Contributor Author

caesar commented Oct 13, 2022

I'd love it to be off by default but that's not default behaviour on macOS…
Should we override it by default? I don't know. 🤔

Edit: for reference, default behaviour on macOS (ie, when this is on) is to follow the preference set by the user.
The default system preference is for "automatic tabbing" to be enabled in fullscreen mode only.

@caesar
Copy link
Contributor Author

caesar commented Oct 13, 2022

Just a thought – if it's off by default that will technically be a breaking change, so even if we do want that, maybe best to add it as-is for v1.2 and then change the default in v2.0?

lucasfernog
lucasfernog previously approved these changes Oct 17, 2022
Copy link
Member

@lucasfernog lucasfernog left a comment

Choose a reason for hiding this comment

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

Nice addition, thanks! This also closes #2804

@lucasfernog
Copy link
Member

electron actually uses tabbingIdentifier and disables tabbing if the window is transparent or if it doesn't have decorations.

if (tabbingIdentifier.empty() || transparent() || !has_frame()) {
    [window_ setTabbingMode:NSWindowTabbingModeDisallowed];
  } else {
    [window_ setTabbingIdentifier:base::SysUTF8ToNSString(tabbingIdentifier)];
  }

@caesar caesar changed the title feat(macos): add automatic_tabbing option, closes #3912 feat(macos): add automatic_tabbing option, closes #2804, #3912 Oct 17, 2022
@caesar
Copy link
Contributor Author

caesar commented Oct 17, 2022

This also closes #2804

Ah, I was sure there was another issue, but I failed to find it, thanks.

electron actually uses tabbingIdentifier

I did try that it it didn't seem to have any effect, but I might have done something wrong. Might be worth playing with it again.

and disables tabbing if the window is transparent or if it doesn't have decorations.

That seems sensible. Though I must say I'm quite drawn to the idea of disabling it by default, and providing an option to enable. What do you think?

@lucasfernog
Copy link
Member

I can also play with the tabbingIdentifier later - I expect it to define groups of windows that can be tabbed together. I like the idea of disabling it by default, and it's what electron does.

@lucasfernog
Copy link
Member

I've opened a PR to add tabbing identifier support on tao. I couldn't understand how it works when changing the identifier at runtime, but the builder with_tabbing_identifier method works fine. I think we should include it here and work similar to electron, they have that nice logic for it.

@lucasfernog lucasfernog changed the title feat(macos): add automatic_tabbing option, closes #2804, #3912 feat(macos): add tabbing_identifier option, closes #2804, #3912 Oct 18, 2022
@caesar
Copy link
Contributor Author

caesar commented Oct 18, 2022

Wouldn't keeping both options make most sense, since both exist on macOS? I don't think they quite do the same thing.
At the very least, if a tabbing identifier isn't set, we should set NSWindowTabbingModeDisallowed like in the Electron code you shared above. Otherwise, unless a unique tabbing identifier is set for each window, windows will still be grouped into tabs.

Edit: Apologies, now I look properly, I see that's effectively what you've done.
Seems good, but I'll go over it properly now 😄

@caesar
Copy link
Contributor Author

caesar commented Oct 18, 2022

Looks good! Thanks @lucasfernog.

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.

How to create multiple separate windows, instead of "tabs"? Items automatically added to View menu on macOS
3 participants