search

escamoteur / watch_it

MIT License
103 stars 8 forks source link

added registerChangeNotifierHandler and fixed some typo. closes #19 #22

Closed smjxpro closed 6 months ago

escamoteur commented 6 months ago

new version is up on pub

smjxpro commented 6 months ago

@escamoteur I just read the changelog on pub.dev which says "you can now register handlers for pure Listenables/ChangeNotifiers", isn't it inconrrect because I constrained the object to be a ChangeNotifier because I didn't have listenables in my mind. However, after reading the log, the idea came to my head that I could just constrain it to be a Listenable and change the handler name to be registerListenableHandler for all the listenables including ChangeNotifier. Can you do that? Or should I do it and create another PR?

escamoteur commented 6 months ago

If you have time it would be great if you could make another PR Am 27. Dez. 2023, 14:21 +0100 schrieb S. M. JAHANGIR @.***>:

@escamoteur I just read the changelog on pub.dev which says "you can now register handlers for pure Listenables/ChangeNotifiers", isn't it inconrrect because I constrained the object to be a ChangeNotifier because I didn't have listenables in my mind. However, after reading the log, the idea came to my head that I could just constrain it to be a Listenable and change the handler name to be registerListenableHandler for all the listenables including ChangeNotifier. Can you do that? Or should I do it and create another PR? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

smjxpro commented 6 months ago

If you have time it would be great if you could make another PR Am 27. Dez. 2023, 14:21 +0100 schrieb S. M. JAHANGIR @.***>:

@escamoteur I just read the changelog on pub.dev which says "you can now register handlers for pure Listenables/ChangeNotifiers", isn't it inconrrect because I constrained the object to be a ChangeNotifier because I didn't have listenables in my mind. However, after reading the log, the idea came to my head that I could just constrain it to be a Listenable and change the handler name to be registerListenableHandler for all the listenables including ChangeNotifier. Can you do that? Or should I do it and create another PR? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

Ok. I am doing it. 😀

smjxpro commented 6 months ago

On another note, let's say I am throwing an exception inside one of the methods in my ChangeNotifier, is there any way or is it a good idea to catch that exception and pass the error on another callback in watchers/handlers?

escamoteur commented 6 months ago

IMHO if you expect an exception you should place the try catch inside your handler function Am 27. Dez. 2023, 14:26 +0100 schrieb S. M. JAHANGIR @.***>:

On another note, let's say I am throwing an exception inside one of the methods in my ChangeNotifier, is there any way or is it a good idea to catch that exception and pass the error on another callback in watchers/handlers? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

smjxpro commented 6 months ago

@escamoteur After searching I found out that only ValueNotifier and ChangeNotifier implements the Listenable interface which are used for state management purposes. And there's already a handler for ValueNotifier therefore I found it redundant to implement/generalize another on by changing registerChangeNotifierHnadler to registerListenableHandler.

It just needs to change the you now can register handlers for pure Listenable/ChangeNotifiers too to you now can register handlers for ChangeNotifiers too in the changelog.

Also, it would be useful to add the pub.dev link in the about section on the repo.

Thanks for the great work.

escamoteur commented 6 months ago

It depends, anybody can implement their own listenable class, but you are probably right, let's keep it like that for the moment Am 27. Dez. 2023, 15:38 +0100 schrieb S. M. JAHANGIR @.***>:

@escamoteur After searching I found out that only ValueNotifier and ChangeNotifier implements the Listenable interface which are used for state management purposes. And there's already a handler for ValueNotifier therefore I found it redundant to implement/generalize another on by changing registerChangeNotifierHnadler to registerListenableHandler. It just needs to change the you now can register handlers for pure Listenable/ChangeNotifiers too to you now can register handlers for ChangeNotifiers too in the changelog. Also, it would be useful to add the pub.dev link in the about section on the repo. Thanks for the great work. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>