Can't compare two Channel objects

[Nepherpitou]

Overrided equals function makes it impossible to compare old and updated Channel objects using operator == in Kotlin: https://github.com/GetStream/stream-chat-android/blob/master/library/src/main/java/com/getstream/sdk/chat/model/Channel.java#L400

[Nepherpitou]

For example, I replace existing channel info with new one only if new object is differs from currently displayed. In current implementation of equals method operator == will always return true.

[adrian09h]

@Nepherpitou , Thanks for your share.

I think the compare is correct.

...
return TextUtils.equals(this.getCid(), otherChannel.getCid());

Because the override equal method is called from only areItemsTheSame of ChannelListDiffCallback

https://github.com/GetStream/stream-chat-android/blob/master/library/src/main/java/com/getstream/sdk/chat/adapter/ChannelListDiffCallback.java#L29

cid is unique for channel object so it means in case two channel objs have same cid, those are same item. (we don't call it in areContentsTheSame ).

If you update the channel's content, it will be compared correctly from areContentsTheSame of ChannelListDiffCallback.

[Nepherpitou]

@adrian09h unfortunately, UI components of SDK can't be integrated into project with architecture differing from MVVM, especially while all fields in ViewModels are private and can't be overrided, so I'm using low level API and such flaws in design makes a lot of problems. It's not acceptable for SDK to override such methods as equals and hashCode (especially it's not acceptable to override equals without hashCode) just to skip null-checks and ID comparing in the only one place.

[tbarbugli]

@Nepherpitou I am the person to blame here (git pun intended) for the equals code :) Which architecture do you use in your application?

Perhaps I am missing the point but if you use plain equals (the one from Object) you get the opposite problem; two objects holding the same channel data will always be not equal which will trigger useless UI refreshes (this is pretty common since many API responses and WS include channels' most recent data).

You have point on hashCode not being implemented (using a Channel object as hash-map key seems a fairly exotic choice but I agree this is weird/incorrect).

@elevenetc any input on this conversation? After some review we could also decide to leave equals alone and move the diff-ing methods to Channel class that we have in the ChannelListDiffCallback (such method could be useful to someone not using our VM or the SDK Channel List view)

[Nepherpitou]

@tbarbugli I'm using MVI, but problem mostly isn't with architecture itself, but with integration into existing domain models. For example, our users has their own avatars, and I always can access that info. Now, I want to display these avatars as messages and channels avatars, but there are no way to implement loading of our data inside your viewmodels. About equals. Please, keep in mind developers actively using kotlin in development, and in kotlin operator == using equals method by default, so overridden equals methods causing very unpredictable behavior. It's fine to override equals, but such changes should be reasonable and obvious or documented. When some POJO has overridden equals with comparison of single property it's not obvious or reasonable change (from my point of view, of course) and produces issues that are very hard to debug. About useless UI refreshes. Here is a very simple point: useless refresh is better than inconsistency, and if you put all together (kotlin with == with overridden equals) you definitely should see where you will get inconsistency.

[tbarbugli]

@Nepherpitou thank you for the reply; in terms of custom data, you can track additional custom fields on users, channels and messages. These models have an extraData attribute.

IE. you can have a custom field called "avatar" on users to track the URL of the avatar image and store that in the extraData map.

[Nepherpitou]

@Nepherpitou thank you for the reply; in terms of custom data, you can track additional custom fields on users, channels and messages. These models have an extraData attribute.

IE. you can have a custom field called "avatar" on users to track the URL of the avatar image and store that in the extraData map.

Yeah, I know. But in that case I need to sync our models with GetStream user models and I should do sync before any interactions with SDK components because there are no documented way to gently refresh already populated views.

[frost13it]

using a Channel object as hash-map key seems a fairly exotic choice but I agree this is weird/incorrect

Putting a Channel to a HashSet does not seem to be exotic, is it? Breaking equals()/hashCode() contract is not just weird, it's totally wrong and may let application misbehave in an unpredictable way. So this should defenitely be fixed.

[tbarbugli]

@frost13it totally agree on the hashCode we have this fixed and will release a new version of the SDK soon!

[elevenetc]

It's not acceptable for SDK to override such methods as equals and hashCode

@Nepherpitou may I ask why exactly it's not acceptable? Do you propose that all developers who use SDK must define equals/hashCode for all model classes before using SDK?

[Nepherpitou]

It's not acceptable for SDK to override such methods as equals and hashCode

@Nepherpitou may I ask why exactly it's not acceptable? Do you propose that all developers who use SDK must define equals/hashCode for all model classes before using SDK?

God, no! I mean only existing implementation with comparing only model's IDs. It's not obvious. I mean, you have two Channel objects "new" and "old", and you know their properties are different, but id are same, what value will you expect for val a = new == old? Personally, I'm expecting a to be false, but equals are overriden and I got true.

[tschellenbach]

Fixed in the 4.x branch (using data classes)