Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Definition of NSWorkspace, NSNotificationCenter, and NSRunningApplication #101

Open
wants to merge 9 commits into
base: trunk
Choose a base branch
from

Conversation

agg23
Copy link
Contributor

@agg23 agg23 commented Jul 21, 2023

Got a little carried away going down the rabbit hole of getting some notifications and NSWorkspace methods working. I believe this PR should cover the majority of usecases, though there are some uncommon methods that I did not implement (I left comments marking what is missing).

  • Introduces Retainable trait - Allows for a common way to build retain definitions and allows for the retain_nullable connivence method, which will be needed more and more as more nullable instance properties are implemented
  • Expands NSMutableDictionary methods to be more complete. Adds Iterator support - Note that the untyped nature of Objective-C collections makes dictionaries particularly difficult. Several methods assume NSString keys
  • Adds strum macros for generating NSString constants from enums, like what is used for NotificationName - I don't imagine there will be many other locations outside of NSNotification where this macro is used, but I think it's hugely helpful in this case.
  • Definitions for NSWorkspace, NSNotificationCenter, NSRunningApplication, and NSNotification - These are all interconnected and rely on each other, hence the expanding scope of this PR. Addresses Bindings for NSWorkspace #24

I know there's a lot of stuff here, so take your time. I'm sure there's many things to complain about.

@@ -1,3 +1,5 @@
// TODO: This should be moved elsewhere
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really understand why this trait is under notification_center. It also isn't very obvious how this should be used, except for the examples.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is... ancient and really just dates back to when an early version of this was used in subatomic. It can def be moved.

Comment on lines 64 to 94
pub fn keys(&self) -> Vec<String> {
let keys = NSArray::retain(unsafe { msg_send![self.0, allKeys] });
keys.iter().map(|s| NSString::retain(s).to_string()).collect()
}

/// Converts the dictionary into a hashmap, passing each item through a transform function.
///
/// **NOTE:** This only works with string keys
pub fn into_hashmap<T, F>(&self, item_transform: F) -> HashMap<String, T>
where
F: Fn(&String, id) -> T,
{
let mut map = HashMap::new();

let keys = self.keys();

for key in keys {
let item_id = self.get(&key);

if let Some(item_id) = item_id {
let item = item_transform(&key, item_id);

map.insert(key, item);
} else {
// TODO: Should there be an assertion here for runtime failure?
continue;
}
}

map
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hate these being string specific, but I'm not sure how else to keep this API usable

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine to use String keys for now, to be honest - better to be out and working with potential improvements/revamps later, especially since while I'm fine with these pieces being more open, they're still mostly an internal aspect.

I think you're probably on the right train of thought re: having an assertion here.

@ryanmcgrath
Copy link
Owner

So I'm musing about juggling pull requests here - I definitely do want these in cacao, however I think the objc2 PR might muck this PR up. Given that we're starting to look into fixing more internal core stuff, I'd like to do the objc2 stuff first just so we're not backtracking later on - so apologies in advance if I put this into an un-merge-able state.

@madsmtm
Copy link
Contributor

madsmtm commented Aug 1, 2023

(Also: icrate has NS[Mutable]Dictionary iterators that doesn't require all the keys to be strings, so if you use that, you get iterators for free).

@agg23
Copy link
Contributor Author

agg23 commented Aug 1, 2023

That's perfectly fine. I think most of this becomes unnecessary with objc2 anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants