Swiftgram / TDLibKit

Native Swift wrapper for Telegram's TDLib. Available on iOS, macOS, watchOS, tvOS and visionOS.
MIT License
106 stars 23 forks source link

`Hashable` and `Identifiable` #32

Closed levochkaa closed 1 year ago

levochkaa commented 1 year ago

I don't see any problems with adding Hashable protocol conformance, even to the TdInt64, that should be ok.

Identifiable is a bit more complicated: I've checked all the structs that now gets Identifiable conformance and most of them mention, that id is "Unique", but some of them don't, even if it is an identifier of the User struct, that must be unique AFAIK.

I see two possible variants to deal with Identifiable:

  1. Add Identifiable conformance, only if the id is mentioned in the comment as "Unique".
  2. Leave the uniqueness to the developers.

I would prefer the second variant.

levochkaa commented 1 year ago

This pull request belongs to the #26 issue

levochkaa commented 1 year ago

Also, I don't get the propose of using TdInt64 and not using just Int64 @Kylmakalle may you explain this a bit?

Kylmakalle commented 1 year ago

Follow-up on Identifiable - https://t.me/tdlibchat/72102

Kylmakalle commented 1 year ago

Identifiable is just a trait, which means that the class has field id. It implies nothing about id uniqueness, so it is safe to use it for all classes with id field

Kylmakalle commented 1 year ago

@levochkaa got a chance to test the Hashable protocol with TDLibKit?

Kylmakalle commented 1 year ago

Also, I don't get the propose of using TdInt64 and not using just Int64

Good question, here's the answer

https://core.telegram.org/tdlib/docs/td__api_8h.html#a552928ab323811c9694f5b7c9f53d0fb

This type is used to store 64-bit signed integers, which can't be represented as Number in JSON and are represented as String instead.

Kylmakalle commented 1 year ago

@levochkaa Please, Bump minor version in the root directory

levochkaa commented 1 year ago

@Kylmakalle

Identifiable is just a trait, which means that the class has field id. It implies nothing about id uniqueness, so it is safe to use it for all classes with id field

that's of course true, but if the id won't be unique, there can be some bugs in SwiftUI.

I was thinking about it for a while and came to this: Identifiable stays for struct's with id property, because for 99.99% that won't cause any bugs and it would be just easier to write some simple code. And when making a bigger app (like mine BetterTG or Ggora's Moc) you won't really use simple TDLib structs, because they would be pretty useless. (Also, when needed, I was making the struct Identifiable or fake Identifiable sometimes, never got bugs with that) And, if someone comes up with a bug with uniqueness of Identifiable structs, I hope -- he will open an Issue and that specific case would be discussed or fixed in the script. And, you can use Hashable for uniqueness, so, no problems with that

levochkaa commented 1 year ago

Follow-up on Identifiable - t.me/tdlibchat/72102

Btw, thx for the chat. I've checked the discussion, voting for struct's to be Identifiable still😅

levochkaa commented 1 year ago

@levochkaa got a chance to test the Hashable protocol with TDLibKit?

Nope, but what can happen, also, I've used it few times (as I said in previous messages), and the coder, that opened an Issue did that Adopting to Hashable is just annoying, when doing it from the app, not from the TDLibKit

levochkaa commented 1 year ago

@levochkaa Please, Bump minor version in the root directory

Bumped

levochkaa commented 1 year ago

Also, I don't get the propose of using TdInt64 and not using just Int64

Good question, here's the answer

core.telegram.org/tdlib/docs/td__api_8h.html#a552928ab323811c9694f5b7c9f53d0fb

This type is used to store 64-bit signed integers, which can't be represented as Number in JSON and are represented as String instead.

Wow, thanks, now I know

Kylmakalle commented 1 year ago

Nope, but what can happen, also, I've used it few times (as I said in previous messages), and the coder, that opened an Issue did that Adopting to Hashable is just annoying, when doing it from the app, not from the TDLibKit

@levochkaa I meant, have you tried this branch with some application that can benefit from the Hashable

levochkaa commented 1 year ago

Nope, but what can happen, also, I've used it few times (as I said in previous messages), and the coder, that opened an Issue did that

Adopting to Hashable is just annoying, when doing it from the app, not from the TDLibKit

@levochkaa I meant, have you tried this branch with some application that can benefit from the Hashable

Now i get, Ok, will give an example app in a few days

levochkaa commented 1 year ago

Made an example usage of Identifiable protocol in a List and using Hashable protocol in new iOS 16 SwiftUI NavigationStack I've made the app really simple, but showing how useful it can be

(Without Hashable protocol in models the new navigation usage is almost impossible)

https://github.com/levochkaa/TDLibKitHashable/

Kylmakalle commented 1 year ago

@levochkaa thank you for the contribution and comprehensive example! Feel free to provide more additions that will help TDLibKit to be easily integrated into SwiftUI