Use NSAccessibilitySharedFocusElementsAttribute in OakChooser

[dusek]

This new 10.10 API allows one to mark, for some UI element, a set of UI
elements to track selection. VoiceOver (and other accessibility clients)
will then track and announce selection (cursor) changes not only in the
currently focused element, but also for all elements contained in its
"shared focus elements" array. This is a perfect fit for search field
with a table of results. In fact it is used for new 10.10 Spotlight -
its search field's shared focus element is the table containing the results.

As NSAccessibilitySharedFocusElementsAttribute is available only in the 10.10 SDK,
so make a way to use its value with previous SDKs (with which TextMate
is currently compiled).

We also retain the kind-of-hacky solution for pre-10.10 OSes introduced in
and instead use NSAccessibilitySharedFocusElementsAttribute.

This code was tested in all 6 fields of a 3×2 matrix:

• compiled with 10.8, 10.9 and 10.10 SDK
• run on 10.9 and 10.10 (DP2)

For user, this means more standard behavior (says the same "completion selected"
thing as with Spotlight) and a bit more correctness (no extra space
before beginning / after end of search field on braille display).

For developer, this means once we stop supporting 10.9, we will be ready to
drop a lot of code which, while serving us well to make the choosers more
user friendly on pre-10.10, will no longer be needed.

I release this patch (and any possible subsequent follow-up patches in this pull request)
to public domain.

[dusek]

Forgot to add that I tried to add NSAccessibilitySharedFocusElementsAttribute also to Bundle Item Chooser's search field, but did not find yet a place (code-wise) where I could reach both the search field and table view at the same time, from the first look it looks like those are decoupled by design.

[sorbits]

Merged as 900ee1f. I was confused by the defines and wanted to change to using weak linking of the symbol, but after having changed the code I realised that this requires linking against the 10.10 SDK (which is an unwelcome dependency right now).

I kept my changes and guarded them so that weak linking is only used when building with the 10.10 SDK (b3aa14b). It does add complexity because we now rely on a custom run-time check (when building with the 10.9 SDK or earlier) and a linker-check (used when building with the 10.10 SDK), but once we can require the 10.10 SDK we can remove most of the code and rely solely on weak linking.

That said, I have not actually tested building or running it on 10.10, so please let me know if it fails.

As for the bundle item chooser: I think I have mentioned it before, but it should be migrated to OakChooser, my main problem is that I want to re-do the UI at the same time, and I got stuck the last time I gave it a try, because I want to place too much functionality into this window…

[dusek]

Hi,

first, thanks for reviewing and merging!

I think I agree that you really got confused by the defines because now I tested compiling with 10.10 SDK and, as I expected from the changes in b3aa14b, it does not run on 10.9 :-) Let me therefore explain a little bit the purpose of changes in OakAppKit.h and OakAppKit.mm that I made.

First, let me note that the code I submitted there is indeed complicated and confusing at first sight, but I arrived at it incrementally as the only possible solution to allow building with all SDKs and running with all OS versions.

If we were compiling with 10.10 SDK, we would be fine and would need no changes in OakAppKit.h and .mm, as NSAccessibilitySharedFocusElementsAttribute (as all other AppKit symbols) is available for us readily in AppKit headers and there it is properly marked for weak linking.

Here comes the first mistake in b3aa14b: checking the availability of a weakly linked symbol of type NSString * requires to check the address of the symbol. Therefore one has to check not for nil != NSAccessibilitySharedFocusElementsAttribute, but for nil != & NSAccessibilitySharedFocusElementsAttribute. I think what makes a developer's mind slip into thinking they can check directly for the value of the symbol (that is nil != NSAccessibilitySharedFocusElementsAttribute) is that the symbol is a pointer and hey, with pointers you have this convention that when there is "no value", you assign nil to the symbol value. But if you think about it, this does not extend to e.g. integer symbols. Since if you had a weakly linked NSUInteger i;, then you would not check whether it is nil (i.e. nil != i), as i is not a pointer. You would check the address of the symbol, i.e. nil != &i. The same way, you have to check the address of the symbol NSAccessibilitySharedFocusElementsAttribute. Let me add that I fell into this trap myself as well and in first iterations of this patch (before actually testing it), I wrote the code to check the value (instead of the address) of the symbol. Only testing then revealed that is not what is needed (more specifically, TextMate crashes when bringing up the File Chooser, as we are basically checking nil != *(& NSAccessibilitySharedFocusElementsAttribute) in a situation where nil == & NSAccessibilitySharedFocusElementsAttribute, i.e. we are dereferencing a NULL pointer).

So to conclude this first part of explanation, we need to check the availability of the weakly linked symbol in this way: nil != &NSAccessibilitySharedFocusElementsAttribute, i.e. using the symbol's address, because that's what AppKit (and weakly-linked symbols in general) require.

In code, this means: check if we are compiling with the 10.10 SDK or later (by checking defined(MAC_OS_X_VERSION_10_10)) that is set to behave as 10.10 SDK or later, and not 10.9 SDK (by checking MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_X_VERSION_10_10), and if yes, do nothing as AppKit does declare the NSAccessibilitySharedFocusElementsAttribute for us properly, and allows the above mentioned check to be done in runtime to check whether the symbol is available (when running on 10.10 and later), or unavailable (10.9 and earlier). I.e. this branch of an #if needs to have no code.

This also uncovers a second mistake in b3aa14b: there is no need to redefine the symbol in this branch of the #if, as AppKit defines it for us in such a case.

If we are not compiling with 10.10 (or later) SDK set to behave as 10.10 (or later) SDK, then we have to add our way to simulate the weak linking. This is the only branch that needs our custom code.

Now that custom code cannot define a weakly-linked symbol. Weakly-linked symbols require the symbol to either simply be present in the library or simply not. And we need to determine this based on which version of OS we are running. But we cannot define a symbol at runtime (which is the only time we can check for the OS version), only during compile time, so we are a bit stuck. Apple can solve this as it has a unique monopoly in that they actually can ship different versions of their libraries with each OS version, and in a library shipped with previous OS version, simply have the symbol not available at all, and in a library shipped with later OS version, have the symbol simply available.

But we are not in a position to convince Apple to ship different versions of OakFilterList.framework with different versions of OS :-) And even if we were we don't want that.

It's the dynamic linker (dyld) that determines at runtime what value it will put into the address of a weakly-linked symbol, but we cannot use the linker for the above reasons. So we need to do a runtime check ourselves.

I hope now it is clear why I had to choose the double-pointer solution and why it was the only way. I also add that I tested my original code 6 (3*2) times: compiled with 10.8, 10.9 and 10.10 SDKs (3), and then run the resulting binary on 10.9 and 10.10 (2). It worked in all 6 cases. So my original change does not introduce any compile-time or runtime dependency.

So having said all that :-) it seems the right solution is to revert b3aa14b, as, based on all of the above, I think it's not needed at all and the original code provides everything that's needed. And as you properly stated, the moment we require 10.10 SDK, we can drop all the changes in OakAppKit.h and OakAppKit.mm.

(This "comment" unexpectedly grew to basically a blog post on weak linking, so I might be publishing it elsewhere in a modified version :-) )