Skip to content

Add preferences abstraction #12

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

Merged
merged 5 commits into from
Mar 14, 2018
Merged

Add preferences abstraction #12

merged 5 commits into from
Mar 14, 2018

Conversation

faern
Copy link
Member

@faern faern commented Mar 12, 2018

PR #8 was growing quite a lot. So I felt like splitting out some of the code for separate review in order to shrink that PR a bit.

This PR introduces the safe abstraction for SCPreferences. The first commit is the code from @LuoZijun that I cherry picked from the other PR branch. The second commit is where I add some simplifying constructors making it easier to construct the type for the most common use case.

@LuoZijun please add your feedback to this as well.

Please note that all constructors take &CFString instead of &str. This is in line with how we have done in dynamic_store.rs and also with how other crates implementing Apple APIs usually do. Example: https://docs.rs/core-graphics/0.13.0/core_graphics/font/struct.CGFont.html#method.from_name


This change is Reviewable

@LuoZijun
Copy link

@faern I think your implementation is pretty good.

When this PR got merged, i will rebase PR-8.

@@ -13,8 +13,7 @@
//! [`SCPreferences`]: https://developer.apple.com/documentation/systemconfiguration/scpreferences

Choose a reason for hiding this comment

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

@faern
Copy link
Member Author

faern commented Mar 13, 2018

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


system-configuration/src/preferences.rs, line 60 at r3 (raw file):

        unsafe {
            SCPreferences::wrap_under_create_rule(SCPreferencesCreate(

I fixed a bug here now where the reference count was wrong. Since wrap_under_get_rule was used the retain count was incremented when it should not have been. Added test as well.


Comments from Reviewable

@faern
Copy link
Member Author

faern commented Mar 13, 2018

Review status: 0 of 2 files reviewed at latest revision, 2 unresolved discussions.


system-configuration/src/preferences.rs, line 13 at r2 (raw file):

Previously, LuoZijun (寧靜) wrote…

Link: https://developer.apple.com/documentation/systemconfiguration/scpreferences-ft8

Thanks. Done


Comments from Reviewable

@pronebird
Copy link
Contributor

Reviewed 1 of 2 files at r1, 1 of 1 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


system-configuration/src/preferences.rs, line 32 at r4 (raw file):

impl SCPreferences {

Am I right that we do not implement any other methods except instantiation because we don't directly access them?


Comments from Reviewable

@faern
Copy link
Member Author

faern commented Mar 14, 2018

Review status: all files reviewed at latest revision, 3 unresolved discussions.


system-configuration/src/preferences.rs, line 32 at r4 (raw file):

Previously, pronebird (Andrei Mihailov) wrote…

Am I right that we do not implement any other methods except instantiation because we don't directly access them?

Exactly. We don't need anything else for now. We just need access to the SCPreferencesRef in order to send into other functions. For now we will only need it for the SCNetworkService stuff etc.


Comments from Reviewable

@pronebird
Copy link
Contributor

:lgtm:


Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@faern faern force-pushed the add-preferences-abstraction branch from d037442 to 55477ed Compare March 14, 2018 11:04
@pronebird
Copy link
Contributor

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@faern faern merged commit 55477ed into master Mar 14, 2018
@faern faern deleted the add-preferences-abstraction branch March 14, 2018 19:22
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