atomikpanda / libcolorpicker

Color Picker Library For iOS
MIT License
38 stars 18 forks source link

This is just wrong and causing headaches #13

Closed uroboro closed 4 years ago

uroboro commented 4 years ago

This snippet of code has caused many issues since this project's inception:

https://github.com/atomikpanda/libcolorpicker/blob/c5b51a29665442434b505a64eece036fd2b523b7/PFSimpleLiteColorCell.mm#L28-L32

However, I cannot blame the developers to know about the core cause of the issue because it is an odd behaviour.

The problem is that [self.specifier properties][@"libcolorpicker"] returns an NSDictionary, but the code assumes it will be an NSMutableDictionary. Here's DHowett explaining this issue on June 6 on #theos on saurik's IRC server:

06:01:30 The code in libcolorpicker is doing this:
06:01:47 1. Preferences.framework loads up your "specifier" plist, and it sees that "libcolorpicker" is required for it
06:01:59 2. Preferences gives the loaded NSDictionary from your data to libcolorpicker
06:02:13 3. libcolorpicker tried to edit the dictionary by using setObject:forKey:
06:02:21 this is _critical_. okay.
06:02:28 tries*
06:02:39 It's not allowed to do that.. like, ever. It should never change somebody else's dictionary.
06:02:58 That should always fail. They should be using "[options mutableCopy]" to make an editable copy that's disconnected from the original version
06:03:12 But here's a little quick about how property lists work in Cocoa!
06:03:24 There are three property list formats. The text one, the XML one, and the binary one.
06:03:38 FINALPACKAGE converts all your property lists into binary format. They're WAY faster to load, and a lot smaller too!
06:03:45 so it's great for a release version of the package.
06:03:54 I said "quick;" i meant "quirk"
06:04:11 Here's the quirk: XML property lists are loaded into mutable dictionaries .. and then Cocoa just lies about it and says they're NOT mutable
06:04:18 binary property lists are loaded into IMMUTABLE dictionaries
06:04:32 so when libcolorpicker tries to edit it ........ it either fails or succeeds, depending on what format the plist was in
06:05:12 so because the FINALPACKAGE=1 build rules optimized your property lists, libcolorpicker failed to edit the dictionary..
06:05:21 all because it should not have ever tried. that was their fault. they wrote bad code
06:05:26 bad code that "just worked"
06:05:31 because of a quirk. :)

The solution is to simply change:

https://github.com/atomikpanda/libcolorpicker/blob/c5b51a29665442434b505a64eece036fd2b523b7/PFSimpleLiteColorCell.mm#L29

to this:

self.options = [[self.specifier properties][@"libcolorpicker"] mutableCopy];

Nosskirneh commented 4 years ago

Thanks for the detailed insight! I started maintaining the project in June, so I don't know why the code looked like that in the first place. Looking at the commit history, it seems to originate five years ago, which is why it's surprising to see no reports until very recently.

Not directed at you, but towards Howett: I see no reason to get snarky and call things "bad code". All code have issues. I know this is none of his responsibilities, but reading into the issue and not forwarding it to the responsible developer(s) seems like bad practice to me. There's a reason why things are kept open source.

With that being said, I'm happy that you took the time to bring this to our attention!