Added tabbingIdentifier to Window in Cocoa

[HalfWhitt]

Based on discussion on #2227, I've tried out the tabbingIdentifer property of NSWindow and found that it works as one would expect; setting it to a string of the window's class does indeed make it so that windows only tab together with others of the same class, which solves the "main window is special but gets lost as a normal tab" issue.

I'm a little unsure how best to design the API used to test this, so wanted to check in before implementing. Ultimately it would make sense for a Window object to have access to its tabs, but since it's only implemented at all yet on macOS, would it make sense to leave core Window unaltered and only put ways to check on tabbing status in WindowProbe (with xfails on other platforms)?

I'm also not positive whether it would make more sense to have something like tabs which is a list including the calling window, or tabbed_with which only includes the other windows; each would be empty when there aren't any tabs, so there wouldn't necessarily need to be a separate has_tabs boolean unless we want it for legibility.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

The tabbingIdentifier approach you've taken here definitely seems like the right idea to me. Using the class of the window is as good an indicator as any for forming tab clusters; and, if a user wants different tab clusters, subclassing is a relatively obvious API for that.

I'd be inclined to the tests for this into the tests for the "merge all windows" command. Every platform except macOS will have an "xfail" implementation of the probe backend (because the functionality won't exist); but on macOS, you can create a couple of Window subclasses, a couple of windows of each class, and then show that merging causes the tabs to be created as you'd expect (using the platform-specific tab introspection APIs).

As for more advanced API - my inclination is to call YAGNI (You Ain't Gonna Need It). This is a very niche macOS API. Unless you're proposing adding a more comprehensive "tabbed window content" API that exists on all platforms (so - a precursor to a user-space API for writing tabbed application), I don't think it's worth going to the trouble implementing an API that will literally only exist for the benefit of macOS apps, and even then, only to support users who are using tabbed interfaces. While that is a type of app that exists in the real world, unless you've got a particularly intense itch that needs to be scratched around this feature, I think there are much bigger fish to fry :-)

[HalfWhitt]

That makes sense. I did, in fact, have generalized tabbing on other platforms in mind, but that's probably getting way ahead of myself.

I've learned two new things:

• A given window's mergeAllWindows command doesn't actually merge all windows; it only merges windows in that tabbing identifier. That is, other windows floating around of a different identifier aren't grouped into a second tabbed window of their own, they're just left alone. This further muddies what exactly a "Merge All Windows" menu command should, by default, do.
• Even though the property is named "allows automatic window tabbing", turning it off disables even an explicit call to mergeAllWindows.

The test I've currently implemented makes new base and subclassed Windows, enables tabbing, then calls merge on them and tests in between that the right windows have gained tabs. But it occurred to me after getting it working that this might be overkill. If we can't figure out a clear way to meaningfully provide window-merging to users, is it even worth implementing on WindowProbe? Perhaps I should take that part back out and just create the windows with tabbing already enabled, to check that they spawn as tabs in the right places. That would be plenty to keep people with automatic tabbing enabled from losing their main window.

...Not sure what's going on with the Linux tests. Is this one of the intermittent issues?

[freakboy3742]

This further muddies what exactly a "Merge All Windows" menu command should, by default, do.

tl;dr - it should do whatever macOS does. "Merge all windows of the same window class" is a bit unwieldy as a menu item; given the context, and some documentation of the platform eccentricity, I don't think we need to overthink it. A platform note on Window describing how macOS will merge windows would seem plenty to me.

The test I've currently implemented makes new base and subclassed Windows, enables tabbing, then calls merge on them and tests in between that the right windows have gained tabs. But it occurred to me after getting it working that this might be overkill. If we can't figure out a clear way to meaningfully provide window-merging to users, is it even worth implementing on WindowProbe? Perhaps I should take that part back out and just create the windows with tabbing already enabled, to check that they spawn as tabs in the right places. That would be plenty to keep people with automatic tabbing enabled from losing their main window.

The test looks good to me; the only thing I really noted was the specific thing you've noted here - that the "feature" is triggered using a probe-specific function. If this can't be triggered by the user, then why are we testing for it?

My inclination is that this should be added as a Merge All Windows default command on macOS; the test case then triggers that code, rather than probe-specific feature.

...Not sure what's going on with the Linux tests. Is this one of the intermittent issues?

It's not a presentation I'm aware of. I've given the test a kick to see if it occurs again... and it did. Mainline is currently passing... so I don't think it's related to something like a version update on Github's side, either. That's gonna be a fun one to hunt down :-)

[freakboy3742]

A couple of fairly minor nitpicks inline; other than the trigger issue mentioned in my previous thread comment, this is looking good.

[HalfWhitt]

Not sure what was going on with artifact downloading, but I guess it's working now?

I've encountered an odd issue. I changed the Merge All Windows command to use a native handler, which works great... manually. In a toy example, it auto-enables and disables as appropriate, merges the correct windows, and merges them regardless of what my system-wide tab preference is set to. Awesome.

However, I can't seem to get it to work in the testbed. In the test as just committed, it pauses indefinitely when the app probe's _activate_menu_item method is called on it. Even then, I can go over, manually click the Merge All Windows menu item, and it works as expected. (I found that adding waits after switching the current window at least avoids possible segfaults.) I don't know anything about Objective C, so despite looking at the method's implementation, Rubicon's SEL, and AppKit's documentation, I'm a little lost.

Additionally, it would be nice to be able to check if the command is auto-disabled when there's nothing active to merge, but assert_menu_item only checks an item's explicit enabled state, and I don't know (yet) how to check the current status of items auto-enabled by their menu.

[freakboy3742]

Not sure what was going on with artifact downloading, but I guess it's working now?

Yeah - that's all fallout from the recent actions/download-artifact@v4 update. Github thinks it's amazing, and would just love you to upgrade... which we have. It... isn't working out well.

I kicked the task a couple of times to get it to complete. AFAIK, there's nothing else we can do other than waiting for actions/download-artifact@249 to be resolved (or roll back our update...)

I've encountered an odd issue. I changed the Merge All Windows command to use a native handler, which works great... manually. In a toy example, it auto-enables and disables as appropriate, merges the correct windows, and merges them regardless of what my system-wide tab preference is set to. Awesome.

However, I can't seem to get it to work in the testbed. In the test as just committed, it pauses indefinitely when the app probe's _activate_menu_item method is called on it. Even then, I can go over, manually click the Merge All Windows command, and it works as expected. (I found that adding waits after switching the current window at least avoids possible segfaults.) I don't know anything about Objective C, so despite looking at the method's implementation, Rubicon's SEL, and AppKit's documentation, I'm a little lost.

SEL is a "selector" - which is Objective C terminology for a dynamic method call. When you invoke [myobj somemethod], somemethod is the selector. When you construct a menu item, you have to provide a selector which defines how the method will be invoked; a selector is how that is done.

A Cocoa menu item specifies a selector to invoke; an attempt will be made to invoke the selector on a series of objects (known as the responder chain). A "normal" menu item uses the selector selectMenuItem:, which will be invoked on the the AppDelegate. Native handlers specify other selectors, which will usually find other targets.

Additionally, it would be nice to be able to check if the command is auto-disabled when there's nothing active to merge, but assert_menu_item only checks an item's explicit enabled state, and I don't know (yet) how to check the current status of items auto-enabled by their menu.

This is possible, but a little complicated. It involves a couple of methods, on the series of objects in the responder chain. The relevant documentation is here

If you've already seen that documentation, let me know where you're stuck and I'll see what I can do to help.

[rmartin16]

I kicked the task a couple of times to get it to complete. AFAIK, there's nothing else we can do other than waiting for actions/download-artifact@249 to be resolved (or roll back our update...)

Not to derail this PR...but just wanted to note I've seen Wandalen/wretry.action implemented with download-artifact@v4....not great...but technically another option rather than rolling back.

For instance:

      - name: Download artifacts with retry
        uses: Wandalen/wretry.action@master
        with:
          action: actions/download-artifact@v4
          with: |
            path: build
            name: build-output
          attempt_limit: 5
          attempt_delay: 1000

[HalfWhitt]

SEL is a "selector" - which is Objective C terminology for a dynamic method call. When you invoke [myobj somemethod], somemethod is the selector. When you construct a menu item, you have to provide a selector which defines how the method will be invoked; a selector is how that is done.

A Cocoa menu item specifies a selector to invoke; an attempt will be made to invoke the selector on a series of objects (known as the responder chain). A "normal" menu item uses the selector selectMenuItem:, which will be invoked on the the AppDelegate. Native handlers specify other selectors, which will usually find other targets.

Aha, thank you! I had already read basically that but I think it clicks now. AppProbe._activate_menu_item explicitly provides the app delegate as the receiver, so it doesn't let other selectors find theirs; mergeAllWindows is supposed to invoked on the currently active window. I guess I'll see if I can figure out how to generalize it.

Incidentally — and probably relatedly — I notice the method is "private". If I get it working on both normal and native handler items, and now that it's being called directly in a test, should I remove the underscore?

This is possible, but a little complicated. It involves a couple of methods, on the series of objects in the responder chain. The relevant documentation is here

I did look at that page last night. As far as I can tell, it describes how NSMenu decides whether to enable or disable a menu item, and how you can write your own validation methods for the menu item to tell NSMenu whether or not it should currently be enabled. I understand that, at least broadly speaking. It seems to me like there'd be some way to ask either the menu or the item "Is this item currently autoenabled?" Or does one have to perform the relevant checks oneself, essentially copying what NSMenu already does?

[freakboy3742]

Incidentally — and probably relatedly — I notice the method is "private". If I get it working on both normal and native handler items, and now that it's being called directly in a test, should I remove the underscore?

Sounds reasonable to me. "Private" is a bit of a loose definition when we're talking about internal test tooling, but "is used directly in a test" is as good a definition as any.

It seems to me like there'd be some way to ask either the menu or the item "Is this item currently autoenabled?" Or does one have to perform the relevant checks oneself, essentially copying what NSMenu already does?

If there's a public API for this, I don't know what it is. To the best of my knowledge, this is one of those "you shouldn't need to care about this, so we don't provide an API for it" features - after all, we only care because we're trying to validate that the menu option is actually working as we expect... which isn't something you'd ordinarily need to do as a macOS programmer.

The good news is that for the purpose of testing, you don't need to implement the entire chain - just the one method call that actually returns the validation for that specific feature. I'm guessing it will be either [[NSApplication sharedApplication] validateMenuItem:item] or [current_window validateMenuItem:item], but I could be wrong on that.

[mhsmith]

Not to derail this PR...but just wanted to note I've seen Wandalen/wretry.action implemented with download-artifact@v4....not great...but technically another option rather than rolling back.

This is still breaking the Toga CI every day. If it's easy to roll back, let's just do that and wait until the upstream issue is fixed, as we should have done in the first place.

[freakboy3742]

Not to derail this PR...but just wanted to note I've seen Wandalen/wretry.action implemented with download-artifact@v4....not great...but technically another option rather than rolling back.

This is still breaking the Toga CI every day. If it's easy to roll back, let's just do that and wait until the upstream issue is fixed, as we should have done in the first place.

The upstream issue was just fixed (or, at least, they've added a retry loop to the download action), and @rmartin16 has rolled out the update. I guess we can watch to see if that has actually addressed the issue (Toga PRs may need to merge with main to get all the the updated references)