Add implementations for NSAccessibility... classes #362
Conversation
There was a problem hiding this comment.
Pull Request Overview
Implements core NSAccessibility classes and C helpers to make accessibility elements, custom rotors, and custom actions functional under manual retain/release.
- Add concrete initializers, accessors, and simple role description to NSAccessibilityElement
- Implement NSAccessibilityCustomRotor and ItemResult, plus NSAccessibilityCustomAction (with block support, perform logic, and factories)
- Provide basic NSAccessibility* C helpers for notifications and role/action descriptions
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Source/NSAccessibilityElement.m | Implements element initializers, accessors, dealloc, and a simple role description. |
| Source/NSAccessibilityCustomRotor.m | Fills in rotor and item result initializers, accessors, and dealloc. |
| Source/NSAccessibilityCustomAction.m | Adds safe block retention/release, perform logic, convenience factories, and description. |
| Source/NSAccessibility.m | Adds basic implementations for posting notifications and role/action description utilities. |
| Headers/AppKit/NSAccessibilityElement.h | Public API and ivars for NSAccessibilityElement, including initializers and accessors. |
| Headers/AppKit/NSAccessibilityCustomRotor.h | Public ivars for rotor and item result classes. |
| Headers/AppKit/NSAccessibilityCustomAction.h | Declares new factory methods and perform method for custom actions. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| - (void) setAccessibilityLabel: (NSString *)label | ||
| { | ||
| ASSIGN(_accessibilityLabel, label); | ||
| } |
There was a problem hiding this comment.
[nitpick] Setters for string attributes should copy (not just retain) to avoid surprises with mutable strings. Replace ASSIGN with ASSIGNCOPY here, and likewise in setAccessibilityIdentifier, setAccessibilityRole, and setAccessibilitySubrole.
Copilot uses AI. Check for mistakes.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Updated accessibility property setters to use ASSIGNCOPY for better memory management.
|
I think this PR is ready as well, @fredkiefer / @rfm |
There was a problem hiding this comment.
I'm not familiar with what this stuff is supposed to be doing, but as far as I can see this replaces unimplemented code so can't really be breaking anything even if it doesn't operate correctly.
One thing that confuses me is the use of
#pragma clang diagnostic ignored "-Warc-performSelector-leaks"
From what I read, that diagnostic should only be produced if you are building in ARC mode, but we don't make the core libraries depend on ARC and it doesn't look like you are even preparing to make this code depend on ARC (for instance, that would require the calls to [super dealloc]; to be replaced with the DEALLOC macro), so I wonder why the pragma to suppress a non-existent warning? Is it because a compiler bug generates an incorrect warning?
| target: (id)target | ||
| selector: (SEL)selector; | ||
|
|
||
| /* Convenience factory returning an autoreleased custom action that invokes a block. */ |
There was a problem hiding this comment.
Shouldn´t we separate the GNUstep extensions from the standard methods? That way people might get frustrated when writing for GNUstep and then porting to MacOS. It would be easy to have a specific category here.
| ASSIGN(_name, name); | ||
| ASSIGN(_handler, handler); | ||
| ASSIGNCOPY(_name, name); | ||
| if (_handler != handler) |
There was a problem hiding this comment.
Why copy this complicated code? You could just use the setter here.
| // Results... | ||
| @implementation NSAccessibilityCustomRotorItemResult : NSObject | ||
|
|
||
| - (instancetype)initWithTargetElement:(id<NSAccessibilityElement>)targetElement |
There was a problem hiding this comment.
Spaces are missing in this line.
| if (self != nil) | ||
| { | ||
| _targetElement = targetElement; | ||
| _targetRange = NSMakeRange(0, 0); |
There was a problem hiding this comment.
Did you verify this specific value? The old value NSMakeRange(0,NSNotFound) seems more plausible to me.
No description provided.