fix: Update to objc2 0.3.0-beta.4; use new icrate bindings #200
Conversation
| @@ -13,6 +13,13 @@ edition = "2021" | |||
| [dependencies] | |||
| accesskit = { version = "0.8.1", path = "../../common" } | |||
| accesskit_consumer = { version = "0.11.0", path = "../../consumer" } | |||
| objc2 = "0.3.0-beta.3" | |||
| objc2 = "0.3.0-beta.4" | |||
There was a problem hiding this comment.
I'm on holiday at the moment, will gladly review it later.
To fix it in the interim, you should use objc2 = "=0.3.0-beta.3" instead, at least until I've released a stable version.
There was a problem hiding this comment.
Though even with this, you should use "=" so that it doesn't break again in the future (this is just a quirk in how pre-release versions work)
| objc2 = "0.3.0-beta.4" | |
| objc2 = "=0.3.0-beta.4" |
There was a problem hiding this comment.
Everything looks good, except for the version thing.
Note that this will make accesskit quite slow to compile, though I'm working on resolving that in madsmtm/objc2#311
| @@ -13,6 +13,13 @@ edition = "2021" | |||
| [dependencies] | |||
| accesskit = { version = "0.8.1", path = "../../common" } | |||
| accesskit_consumer = { version = "0.11.0", path = "../../consumer" } | |||
| objc2 = "0.3.0-beta.3" | |||
| objc2 = "0.3.0-beta.4" | |||
There was a problem hiding this comment.
Though even with this, you should use "=" so that it doesn't break again in the future (this is just a quirk in how pre-release versions work)
| objc2 = "0.3.0-beta.4" | |
| objc2 = "=0.3.0-beta.4" |
| foundation::MainThreadMarker, | ||
| rc::{Id, Shared, WeakId}, | ||
| }; | ||
| use icrate::{AppKit::*, Foundation::MainThreadMarker}; |
There was a problem hiding this comment.
Nit: AppKit::* is a lot of types, I'd probably recommend importing the specifics you need instead (when feasible)
| use std::rc::Rc; | ||
|
|
||
| use crate::{ | ||
| appkit::*, | ||
| context::Context, | ||
| node::{filter, filter_detached, NodeWrapper}, | ||
| }; |
There was a problem hiding this comment.
Nit:
I'd recommend using NSAccessibilityNotificationName in QueuedEvent::Generic::notification below, since that is actually the correct type.
Likewise for QueuedEvent::Announcement::priority being NSAccessibilityPriorityLevel
| let window = unsafe { view.window() }.unwrap(); | ||
| let point = unsafe { window.convertPointFromScreen(point) }; | ||
| let point = unsafe { view.convertPoint_fromView(point, None) }; | ||
| // AccessKit coordinates are in physical (DPI-dependent) pixels, but | ||
| // macOS provides logical (DPI-independent) coordinates here. | ||
| let factor = view.backing_scale_factor(); | ||
| let factor = unsafe { window.backingScaleFactor() }; | ||
| let point = Point::new( | ||
| point.x * factor, | ||
| if view.is_flipped() { | ||
| if unsafe { view.isFlipped() } { | ||
| point.y * factor | ||
| } else { | ||
| let view_bounds = view.bounds(); | ||
| let view_bounds = unsafe { view.bounds() }; | ||
| (view_bounds.size.height - point.y) * factor | ||
| }, |
There was a problem hiding this comment.
FYI: These methods are always safe, and will be marked as such in icrate in the future ;)
|
I will probably reverse the description to pull the AppKit definitions from icrate, due to the substantial increase in compile time. I don't want developers that care about compile time to reject AccessKit. In any case, there's no longer any urgency to this PR, since the dependency on objc2 is now correctly pinned in main. |
|
It looks like this should now update to Is that correct? |
|
Yes, I've known about objc2 0.4.0 for several days but have been putting off working on this. |
|
I'm thinking about doing it this month or next, if you like. |
|
OK, I'll abandon this PR and look forward to your contribution. Thanks! |
fixes #199
Switching from our own AppKit bindings to the new ones in icrate isn't strictly necessary to fix the bug, but I figured it was a good idea anyway.
@madsmtm Please review.