Nipticky! (no need to change anything). This feels more swifty!

NSArray *sortDescriptors = @[sortServiceDescriptor, sortExternalNameDescriptor];

Force of habit, I like it better that way, I will change it.

Thank you! =D

Out of curiosity, does [NSNumber: [String: String]] breaks with Core Data?

Nope it doesn't and it looks better, I appreciate the style suggestions since I'm not so proficient in Swift as I am in Objective-C

None of us is proficient in Swift... since it's mutating all of the time! Thanks for updating this one!

Unless i'm missing something, you can replace this whole method with one line:

return disabledPublicizeConnections?[keyringID]?["value"] == "1"

Can we (maybe) move "value" and "1" / else, to an actual constant?

(Common practise looks something like this):

class X {
   struct Constants {
       static let publicizeValyeKey = "value"
       // ...
   }
}

Will do, thanks for the suggestion and example.

Can't Swift dictionaries be compared? (i mean, can we avoid the NSDictionary conversion?)

They can as long as their keys and values are equatable, but Dictionary is not equatable so in this case we can't avoid the NSDictionary conversion.

Apparently, not nested dictionaries :(
You can compare [String: String], but not [NSNumber: [String: String]].

When you look at the operator it makes sense:

extension Dictionary where Key : Equatable, Value : Equatable {

    public static func ==(lhs: [Key : Value], rhs: [Key : Value]) -> Bool

    public static func !=(lhs: [Key : Value], rhs: [Key : Value]) -> Bool
}

So you can == on [String: String] because both Key and Value are Equatable, but that doesn't make [String: String] Equatable itself. So [NSNumber: [String: String]] can't be compared because it's Value is not Equatable (even though it has a == operator).

What this means is we'd need conditional conformances, which has been accepted, but not landed into Swift yet (Swift 4, maybe?).

That said, we could skip the default values :)

if let current = (disabledPublicizeConnections as NSDictionary?),
    let original = originalPost.disabledPublicizeConnections,
    !current.isEqual(to: original) {
    return true
}

Wouldn't that prevent detecting changes when disabledPublicizeConnections = nil ?

Yep, you're right, I got confused with all the optionals.

(Maybe) split this line in two, to ease out readability?

Please, replace 0.01f with CGFLOAT_MIN

Same as above: CGFLOAT_MIN

No need for else statement, since the return in line 597 will prevent this branch from running

Perhaps capitalize the constant?

I'm wondering if we can make this non-optional. That would simplify all the optional handling a bit.

Bonus: if we could have a simple Connection type instead of [String: String] and get rid of Constants.publicize*, although dealing with Core Data I'm not sure if that'd complicate things even more 😕

@koke yes we can make it non-optinal and the optional handling code does improve a lot.
I'm happy to make the change if it's ok with everyone @kurzee @jleandroperez
BTW do you guys have a policy for when an attribute should be non-optional in CoreData? I know by default attributes are crated as optional so maybe that's why most of them are currently this way.

I keep forgetting about all the Core Data subtleties 🤦‍♂️
We could make this non-optional in the data model, and non-optional in the code, but @NSManaged makes it kind of a lie. Since you don't init managed objects manually, core data's "optional" is only enforced on validate/save. So while you can be certain that an object fetched from storage will have a non-optional attribute set, you can't have the same guarantee about a new unsaved object, making it less safe. So I think it's probably best to leave it as optional? 🤷‍♂️

I absolutely agree, leaving it as optional will be the safest option here.

My take on this would be (considering disabledPublicizeConnections non-optional):

func enablePublicizeConnectionWithKeyringID(_ keyringID: NSNumber) {
    guard var connection = disabledPublicizeConnections[keyringID] else {
        return
    }

    guard connection[Constants.publicizeIdKey] != nil else {
        disabledPublicizeConnections.removeValue(forKey: keyringID)
        return
    }

    connection[Constants.publicizeValueKey] = Constants.publicizeEnabledValue
    disabledPublicizeConnections[keyringID] = connection
}

It's less compact, but easier to read IMO :)

I know the other sorted* methods do the same, but we should be using localizedCaseInsensitiveCompare: for any visible strings. I didn't know better when I wrote that :D

We should use localizedCaseInsensitiveCompare: only for the service name, right? We don't want to modify the external account name which is what the user is used to expect as a visible name. Even though the chances of an external name being in the localized strings file there's no point in using localizedCaseInsensitiveCompare: for non localizable strings.

Good point. I don't think we're translating service names, so it makes sense to only localize account names 👍

This doesn't cover the case when you edit a draft, tap publish, but the post fails. When you edit the post again and try to publish, this will be false as it hasRemote and the status is publish. I'm not sure we can work around it, since there's no way to know the status on the server, but I wanted to document that case.

I think you can drop the ?? "" from both sides.

We want the default values there to avoid detecting changes for nil vs the empty string.

Apparently, not nested dictionaries :(
You can compare [String: String], but not [NSNumber: [String: String]].

When you look at the operator it makes sense:

extension Dictionary where Key : Equatable, Value : Equatable {

    public static func ==(lhs: [Key : Value], rhs: [Key : Value]) -> Bool

    public static func !=(lhs: [Key : Value], rhs: [Key : Value]) -> Bool
}

So you can == on [String: String] because both Key and Value are Equatable, but that doesn't make [String: String] Equatable itself. So [NSNumber: [String: String]] can't be compared because it's Value is not Equatable (even though it has a == operator).

What this means is we'd need conditional conformances, which has been accepted, but not landed into Swift yet (Swift 4, maybe?).

That said, we could skip the default values :)

if let current = (disabledPublicizeConnections as NSDictionary?),
    let original = originalPost.disabledPublicizeConnections,
    !current.isEqual(to: original) {
    return true
}