Fixed bug in testbed where secondary windows could open as tabs on macOS

[HalfWhitt]

The contructor for the Cocoa WindowProbe now sets NSWindow.allowsAutomaticWindowTabbing to False, and I've added a note to the testbed section of the How To guide.

As discussed in Testbed failures on macOS, if a macOS user has "Prefer tabs when opening documents" set to "Always", it causes failures in the testbed suite because secondary windows are opened instead as tabs. This modification to the WindowProbe avoids this... mostly.

For some reason, the very first test in any run will disregard the change and still open a new "window" as a tab; this isn't an issue when the full test suite is run, or even all the window tests. The note explains this, and how it could be an issue if one needs to run a single test and that test needs to open a second window.

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]

For some reason, the very first test in any run will disregard the change and still open a new "window" as a tab; this isn't an issue when the full test suite is run, or even all the window tests.

I'm going to guess this is because the probe for a window is created after the window is created, and the setting is an NSWindow-level setting. If we modify the flag in the app probe, it might ensure the value is set before any window is created.

[HalfWhitt]

Hm. I thought that had done the same thing, but now that I double check, putting it in AppProbe gets some tabs and some windows, but seemingly tabs from the first several? Trying it globally where NSWindow is declared in appkit gets all tabs.

(Except for test_non_resizable, which appears to always manage to make a window.)

[freakboy3742]

Hm. I thought that had done the same thing, but now that I double check, putting it in AppProbe or even globally where NSWindow is declared in appkit is different... I just get all tabs in either case.

Hrm... That's probably because an app probe is only being created if you explicitly ask for one.

Here's something to try - modify the window_probe fixture to have an explicit usage of the app_probe fixture. That will force the invocation of the app_probe constructor, even if it isn't used.

[HalfWhitt]

Hmm. I changed:

def window_probe(app, window):
    module = import_module("tests_backend.window")
    return getattr(module, "WindowProbe")(app, window)

to

def window_probe(app_probe, window):
    module = import_module("tests_backend.window")
    return getattr(module, "WindowProbe")(app_probe.app, window)

With the tab-deactivation magic in AppProbe, both versions behave identically. Which is to say, they each open some tabs and some windows, and I can't quite discern where the distinction lies.

The initial fix is the most effective version so far, but something's definitely not quite adding up here. I think I'm gonna sleep on it and come back with fresh eyes.

[HalfWhitt]

(Incidentally, why getattr(module, "WindowProbe") rather than module.WindowProbe?)

[freakboy3742]

(Incidentally, why getattr(module, "WindowProbe") rather than module.WindowProbe?)

That's... a good question. I think it's an accident of history - the first probes were widget specific, and for widgets, using getattr() means the probe name can be dynamically specified with f"{widget_name}Probe". It's not dynamic for Window or App... but copy and paste is a powerful engineering technique :-)

[HalfWhitt]

One correction: I must have been blearily looking at the wrong thing, because disabling allowsAutomaticWindowTabbing right at the source, in appkit.py, does apply universally, across all tests, regardless of order. Obviously that's not exactly ideal, unless we really do want to make it the default.

Of course it also works across all tests if put in the app fixture in testbed/tests/conftest.py, which of course is even less ideal since that shouldn't be doing anything Cocoa-specific.

(I made a quick-and-dirty little test app to play around with; if nothing else, it demonstrates that it doesn't seem to matter what the tabbing attribute was when a given window was first created; as long as the setting is currently enabled, a newly created window can still tab into it.)

The more I think about it, the more I wonder if this should be made a library-user-selectable option, perhaps as a constructor argument and property of App. I don't know if Windows or GTK have a native equivalent, but if not it could simply have no effect (unless and until Toga implements it somehow). If that were the case, then the testbed could create its app with tabbing disabled, without mucking about in the Cocoa implementation. Does that sound reasonable?

[HalfWhitt]

Actually now that I think about it, having the flag enabled lets new windows open as tabs "for free", but it doesn't automatically give any mechanism for turning them into tabs, such as by dragging them onto each other. Perhaps it should be disabled app-wide until Toga has a convenient API to interact with it more fully.

[freakboy3742]

Actually now that I think about it, having the flag enabled lets new windows open as tabs "for free", but it doesn't automatically give any mechanism for turning them into tabs, such as by dragging them onto each other. Perhaps it should be disabled app-wide until Toga has a convenient API to interact with it more fully.

My inclination is to go the other way.

One of the goals of Toga is to be as compliant as possible to native operating system conventions as possible. For better or worse, the tabbed-window option exists on macOS, and some people have it enabled - presumably because they like the behavior. As end-developers, we can have opinions as to whether this is a good thing or not, but some people do turn it on, and having Toga unilaterally declare that "we know better" (or providing the option to developers to make the same declaration on a per-window basis) is just impolite to user preferences.

Interestingly, there's precedent for this on other operating systems - in particular, Linux. GTK's APIs around window size and placement are explicitly documented (by GTK) as being, at best, hints, because the size and placement of windows is not the app's concern - it's the window manager's concern. An app can suggest that "a bigger window would be preferable", but it's up to the window manager to satisfy that request. This also extends to APIs like minimising windows - the API endpoint exists, but it might not actually be honoured. The best manifestation of this can be seen in mouseless and tiled window managers - they don't have positioning and size controls, because that would violate how they lay out windows.

When we run the Toga test suite on GTK, we have to do so in a very specific window manager, because not all window managers behave the same way - especially the "minimalist" ones, which would ordinarily be the best choice for running in CI because you want the simplest possible test environment.

So - my preference would be to document this as a platform oddity. It's already somewhat documented in the usage notes of window, but it would probably be better to pull the relevant paragraph out as a standalone note, and indicate some of the ways this will manifest in practice (tabs on macOS, window manager interactions on Linux etc)

[HalfWhitt]

I definitely hear that! The philosophy of integrating with what OSes do is one of the things I really like and admire about Toga.

I don't think I was very clear... what I meant was that as far as I've seen, all macOS apps that support spawning tabs also support turning existing windows into tabs in order to combine them. Finder and Safari, for instance, both have Window > Merge All Windows. So far in Toga, you can drag a tab from one into another, but if you have two individual windows, there's no way to turn them into one window with two tabs, which feels jarring to me. But! I see that NSWindow.mergeAllWindows exists, and does what it says on the tin. If it sounds like a good idea to you, I can see about using it to implement Window > Merge All Windows and have it show up by default in apps running on macOS 10.12+ (presumably as a separate PR).

But the better news is that I've fixed the tabbing issue for testing! Putting it in the app probe constructor was indeed the right move, I just wasn't properly ensuring it was called. The trick was putting it in second_window_probe rather than window_probe, since the former calls the latter.

[HalfWhitt]

Oh crap my bad, I did that wrong. Somehow didn't think about the fact I was putting Cocoa-specific code in the general Testbed...

[HalfWhitt]

...Huh. Any idea how coverage on scrollcontainer.py and the build of the docs could fail? Having undone the documentation note I added earlier, neither of those should have changed.

[freakboy3742]

...Huh. Any idea how coverage on scrollcontainer.py and the build of the docs could fail? Having undone the documentation note I added earlier, neither of those should have changed.

Neither of these failures are the result of something you've done.

The Cocoa coverage test is an edge case that occurs occasionally (rarely, but occasionally). It's reporting that the scrollcontainer tests haven't validated the case where the container being scrolled doesn't have content.

The thing is - we do test this branch (L80 of the testbed test_scrollcontainer)... but if the machine is feeling overworked, the exact sequence of refreshes in the GUI can result in the refresh event that would trigger that branch of code to be delayed, causing the coverage gap. Re-running the test almost always addresses the problem; re-running it twice definitely does. It's almost impossible to reproduce reliably, so it's also almost impossible to determine a fix. We've had similar failures on other platforms fixed by adding an explicit 0.1s delay on the probe.redraw() call that exercises the branch... the same might work here, but it's almost impossible to validate that a fix has actually worked.

The cause of the documentation failure is linting - it validates that all the URLs in the docs point to valid destinations. In this case, the failure is one we've seen before: https://developer.microsoft.com/en-us/microsoft-edge/webview2/#download-section . We've seen this error before, but it's usually transient. In this case, it looks like Microsoft has updated the page, and the section header is now tagged #download, not #download-section. I suspect you may need to update this to get the docs to pass. However, every other build will also be failing, so we'll need to get a fix for this into main fairly quickly.

[HalfWhitt]

Ah, gotcha. I guess if you merge 2209 first that should do it then, right?

[freakboy3742]

Ah - I didn't see that PR in the queue. Yes - that will fix it.

[HalfWhitt]

I only just saw it myself. : )

[HalfWhitt]

...Are the Windows colors tests another thing that just occasionally doesn't work due to timing?

[freakboy3742]

...Are the Windows colors tests another thing that just occasionally doesn't work due to timing?

Yes - that one is logged. We've got no idea what's happening there - the colors are returning as 1/255, but nothing is setting them to that value.

[freakboy3742]

The code implementation looks good; the only piece that is missing is the docs clarification around interaction with OS/window manager settings. If you're not comfortable with messing with docs, let me know and I can make that change.

[HalfWhitt]

What would you like me to clarify in the docs? The testbed should now run the same regardless of what the user's tab settings are. I removed the note once I got it working even on the first test run.

[HalfWhitt]

Oh, do you mean just more generally? About the philosophy of not always being able to dictate what a window manager will do?

[freakboy3742]

Oh, do you mean just more generally? About the philosophy of not always being able to dictate what a window manager will do?

Yes - I referenced the Window usage docs previously; the note about availability of window controls should be moved to the notes section, and a similar note added to clarify that window position and size are also at the discretion of the OS/window manager (and give the 2 obvious examples of this - Linux WMs and macOS's window tab control).

[HalfWhitt]

Oh, right! Will see about doing that.

[freakboy3742]

One small suggestion for the wording of the note; but otherwise, this looks great!

[freakboy3742]

Also - it looks like you're having a stellar run finding all the test suite edge cases... the Android failure is another known problem (#2185) - although this one I thought we'd fixed...

Don't worry about doing pushes to correct these - I can manually re-run individual test targets until they pass.

[freakboy3742]

Looks like the CI has finally decided to play ball, so this is good to land!

Thanks for the awesome work debugging this gnarly edge case - it's most appreciated!

[HalfWhitt]

Hooray! Happy to help, and thank you for your patience as I do all this for the first time.