Skip to content

feat: Event::Reopen support for macOS#3499

Closed
I-Info wants to merge 13 commits intorust-windowing:masterfrom
I-Info:master
Closed

feat: Event::Reopen support for macOS#3499
I-Info wants to merge 13 commits intorust-windowing:masterfrom
I-Info:master

Conversation

@I-Info
Copy link
Copy Markdown

@I-Info I-Info commented Feb 18, 2024

  • Tested on all platforms changed
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Description

This event is typically used to handle dock icon clicking event on macOS and uses applicationShouldHandleReopen:hasVisibleWindows: callback from AppKit Framework.

Use case

It will make it possible to keep application running in backgound when all windows are closed, and create new window after dock icon clicked (application reopen callback).

@I-Info I-Info requested a review from madsmtm as a code owner February 18, 2024 04:41
@I-Info I-Info changed the title feat: Reopen event support for macOS feat: Event::Reopen support for macOS Feb 18, 2024
@daxpedda daxpedda added the DS - appkit Affects the AppKit/macOS backend label Feb 19, 2024
Copy link
Copy Markdown
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

I can see this being useful, thanks!

I-Info and others added 2 commits February 21, 2024 16:54
Co-authored-by: Mads Marquart <mads@marquart.dk>
@I-Info I-Info requested a review from madsmtm February 21, 2024 12:29
Copy link
Copy Markdown
Member

@kchibisov kchibisov left a comment

Choose a reason for hiding this comment

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

nack for 0.30, should be good for 0.31 with traits.

@kchibisov kchibisov added this to the Version 0.31.0 milestone Feb 23, 2024
@daxpedda daxpedda added the C - nominated Nominated for discussion in the next meeting label Feb 28, 2024
Copy link
Copy Markdown
Member

@madsmtm madsmtm left a comment

Choose a reason for hiding this comment

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

nack for 0.30, should be good for 0.31 with traits.

I'm not going to block this on 0.31, it's a simple change that we can do now, and generally enables better behaviour on macOS.

@kchibisov
Copy link
Copy Markdown
Member

kchibisov commented Mar 1, 2024

And what should we do with all of that on other platforms? The event is macOS specific, but it bleeds into other backend and people will demand its handling, while in reality such event doesn't exist for them.

You should never trade quality for short term wins, this is just straight reduces overall quality because the event is not tied to window in particular and leaves the impression that such thing should be handled.

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Mar 1, 2024

And what should we do with all of that on other platforms? The event is macOS specific, but it bleeds into other backend and people will demand its handling, while in reality such event doesn't exist for them.

I think the event is well documented as "Unsupported", which we already have precedent for, I don't see the issue with adding another one.

In the end it won't really matter, since we're going to be changing things for 0.31 anyways. But like I said, in the months until that happens, this is a useful feature.

@kchibisov
Copy link
Copy Markdown
Member

In the end it won't really matter, since we're going to be changing things for 0.31 anyways. But like I said, in the months until that happens, this is a useful feature.

No it's not, because it'll assume porting it to other backends as a top-level event. If you want this feature, require the new application trait for that and wire the vtable like we've discussed, so it won't bleed into any other backend.

@kchibisov kchibisov removed the C - nominated Nominated for discussion in the next meeting label Mar 1, 2024
Copy link
Copy Markdown
Member

@daxpedda daxpedda left a comment

Choose a reason for hiding this comment

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

I nominated this specifically because I wanted to discuss this "blocking it because its platform-specific" issue in general, but we didn't get to it unfortunately.

Personally I strongly disagree that this somehow reduces the quality of the Winit API, we already have a big amount of events that are not cross-platform, this doesn't change anything.

That said, I'm glad we are trying to fix/improve this issue in v0.31.

@kchibisov
Copy link
Copy Markdown
Member

All Event stuff is cross platform and has special semantics. WindowEvent stuff has specifics, but not the Event.

As I said, the solution is already here even for 0.30, if you want I can write it myself for macOS.

@madsmtm
Copy link
Copy Markdown
Member

madsmtm commented Jun 24, 2024

Instead of exposing this through Winit, #3758 is going to allow listening to NSApplicationDelegate events outside of Winit using objc2::declare_class!, Objective-C, Swift, or similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DS - appkit Affects the AppKit/macOS backend

Development

Successfully merging this pull request may close these issues.

4 participants