AppKitNavigation - Navigation

[Mx-Iris]

This PR provides AppKit navigation with present, sheet, modal, modal session

[stephencelis]

@Mx-Iris The other branch was merged so we should be able to review this one as soon as the conflicts are resolved. Can you ping us when it's ready?

Hopefully it's just a matter of cherry-picking your last commit back over main.

[Mx-Iris]

@stephencelis The conflict has been resolved and is ready for review

[stephencelis]

From what I can tell this is a helper that sets up an associated object. Is it doing anything else? If not, I'm inclined to be consistent with the pattern used in UIKitNavigation, where we define dedicated local keys where needed instead of leveraging a dynamic helper. How's that sound?

[Mx-Iris]

Again because there is more than one type of object that can navigate, this is mainly used for Observer generic types like Sheet, the classes that can execute it are NSWindow, NSSavePanel, NSAlert, NSPrintPanel.... They are not necessarily of the same type, and each class has a different way of executing a sheet.

[stephencelis]

Because associated keys are local to each object, does the type need to be encoded into the operation? Wouldn't it work the same to use the same static key instead?

[Mx-Iris]

private func sheetObserver<Content: SheetContent>() -> SheetObserver<Self, Content> {
        if let observer = objc_getAssociatedObject(self, sheetObserverKeys.key(of: Content.self)) as? SheetObserver<Self, Content> {
            return observer
        } else {
            let observer = SheetObserver<Self, Content>(owner: self)
            objc_setAssociatedObject(self, sheetObserverKeys.key(of: Content.self), observer, .OBJC_ASSOCIATION_RETAIN_NONATOMIC)
            return observer
        }
    }

Because of the one-to-many relationship, a class that executes a sheet may have more than one observer (this class is mainly used to store the object that has been executed sheet), the object that is executed by the sheet is indeterminate, it may be NSAlert or NSWindow. Currently, storing the object that is executed by the sheet is mainly to perform cleanup work, if you have a If you have a better idea, let me know! 😄

[stephencelis]

Can everything in this file be internal? They all seem to be helpers for driving navigation, but probably shouldn't be public APIs that folks encounter when they import AppKitNavigation.

[stephencelis]

Do you think it's possible to avoid these protocols? In UIKitNavigation we got by using the classes themselves, but maybe I'm missing something?

[Mx-Iris]

The controls that can perform ModalSession are not only NSWindow, but also NSAlert, NSSavePanel, etc. The same applies to other Sheet protocols.

[stephencelis]

I'll try to set aside some time later this week to explore the APIs and get a better understanding of how they all tie together then. Thanks for your patience!

[maximkrouk]

There are a few comments about public protocols that might be a little excessive for the public API, if they greatly simplify things internally I think there might be an option to hide them using @_spi 🤔

[Mx-Iris]

@maximkrouk I tried it and found that it doesn't work. If you use this protocol for spi, all methods that use this protocol need to be spi-compatible.

[stephencelis]

Similarly are these protocols needed, and if so do they need to be public? It'd be good to avoid adding public APIs that aren't intended for use outside the library when folks import AppKitNavigation.

[Mx-Iris]

This protocol abstracts a lot of navigation methods. UIKit's navigation is relatively homogeneous, but AppKit's navigation is messy and complex, and if you don't pull it out there will be a lot of duplicate code.

[stephencelis]

@Mx-Iris Thanks for continuing to contribute 😄 I did a quick initial review and commented on things that stood out to me. I think the main thing we'd like to aim for is consistency with other parts of the library, e.g. UIKitNavigation. In particular:

• Let's stick with the existing pattern for associated objects
• Let's try to avoid introducing any public APIs (protocols, methods, etc.) that are only meant to be used internally.
• If possible, let's try to avoid introducing protocols to abstract concepts. And if there are good reasons to introduce protocols, let's discuss why.
• Let's also try to avoid helpers unless they're saving a lot of extra work.

Let us know what you think!

[Mx-Iris]

@stephencelis Thanks for the review, I replied to some of the issues you mentioned because AppKit is very different from UIKit, it's very cluttered and can't be viewed in the same way as UIKit.

[stephencelis]

@Mx-Iris Thanks again for your patience! @mbrandonw and I took another look today and we think it's a fantastic start but we're not yet comfortable enough with the code to bring it in and maintain it as is. We think there are opportunities to remove some of the public and internal abstractions and expose helpers that mirror the platform's built-in helpers in a similar way to the UIKitNavigation module.

We think there are a couple paths forward depending on your interest:

1. We could consider this branch a really great reference for @mbrandonw and I to look at when we try to reframe the solution in a style that makes sense to us. We're not 100% sure when we'll have the spare cycles for such a task, but hopefully in the coming months.

2. We could maybe narrow this PR's focus even further so that we have a minimal amount of code to review and push in a direction that makes sense to us as project maintainers. Right now the PR introduces a lot of different navigation styles that are each implemented in their own chunk of code: presentation, navigation, modals, and sheets. If you're interested in continuing to work on this project and refine things, we think it'd be good to pick the simplest of all these forms of navigation and open a dedicated PR that tackles just that form. It looks like the "presentation" APIs might be the simplest or at least closest in analog to a lot of the UIKitNavigation APIs. Does that sound right?

Do you have a preference here? We don't want to keep pushing back on your work if it feels annoying or nitpicky, but are happy to continue working with you if you're up for it. Otherwise, we hope you're OK with us taking on the next steps of this PR when we have a chance. Thoughts?

[Mx-Iris]

@stephencelis Hey, Discussing the navigation separately indeed makes things clearer. Since I've been quite busy with work recently, I may not have much time to deal with these matters. Should we leave it here for now? I'll come back to finish it when I have time.

[stephencelis]

@Mx-Iris Yup no rush at all! Thanks for the update. If you want to break things down to a smaller PR when you have time that would be helpful and instructive, and if Brandon and I have time we may push some commits ourselves 😄

[Mx-Iris]

@stephencelis I've set all protocols to internal to only expose explicit types externally, this would be a lot of boilerplate code, but does reduce complexity a lot, if you can, follow up by introducing macros to solve the boilerplate code issue, see the extension in the Modal navigation section to see if it addresses your concerns!