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

Make info_dictionary return mutable dictionary #146

Closed
wants to merge 1 commit into from

Conversation

@faern
Copy link
Contributor

faern commented Jan 21, 2018

The problem is that glutin sets a value in an immutable dict here: https://github.com/servo/glutin/blob/servo/src/api/cocoa/mod.rs#L403. That does not work any longer after #135, since set_value is gone.

CFBundleGetInfoDictionary returns a CFDictionaryRef (not a CFMutableDictionaryRef) and the docs on that function does not mention the returned dictionary being mutable. So if going by that information, this PR changes into something wrong. But if the code in glutin is correct (which I know nothing about, only have to assume), then it can obviously set a value in the dict. So is it mutable?

One way to solve it is in the way done in this PR at the moment. Because then glutin can continue to set the value in the dict. But if we think the returned dict should be immutable in general (as the Apple docs seems to indicate?), then we could keep this crate as is, but instead make glutin step down one level and call CFDictionarySetValue unsafely by itself.


This change is Reviewable

@faern
Copy link
Contributor Author

faern commented Jan 22, 2018

Another way to solve it would be to continue let CFBundle::info_dictionary return an immutable CFDictionary, as it likely should(?). But then add an unsafe:

CFDictionary::to_mutable(&self) -> CFMutableDictionary

That could be used to convert any dictionary to a mutable if you know what you are doing.

But I guess someone with better understanding of core foundation would have to fill in here.

@jrmuizel
Copy link
Collaborator

jrmuizel commented Jan 22, 2018

In this case I think we can just drop this code from servo/glutin. It looks like it was added before servo/servo#11899 and was a hack to set the app name without having a .app

It's not unlikely that we'll run into this kind of thing elsewhere and I feel like CFDictionary::to_mutable(&self) will probably be the best solution but we can probably wait until we encounter it again.

@jdm
Copy link
Member

jdm commented Jan 23, 2018

I agree with @jrmuizel's conclusions.

@jdm jdm closed this Jan 23, 2018
@faern faern deleted the faern:make-bundle-dict-mutable branch Jan 23, 2018
@faern faern mentioned this pull request Jan 24, 2018
2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.