levibostian / Wendy-Android

Build offline first Android mobile apps. Remove loading screens, perform tasks instantly.
https://levibostian.github.io/Wendy-Android/wendy/
MIT License
64 stars 16 forks source link

Added listener remove functions #54

Open Grohden opened 5 years ago

Grohden commented 5 years ago

This should close #48 I added two listener removal functions.. but i think we need to discuss some situations

Also, we need some tests on Wendy and examples on the example app.. I think that testing needs to be done after #50 ... and the example app i don't have ideas for the remove functions uses

Grohden commented 5 years ago

I need to mention that dokka generates the .html files based on the system path separator.. which is bad, i got 99 files of diff after generating the docs on windows (my fedora is broken :/), is there any config for that?

levibostian commented 5 years ago

Replaying to your comment @Grohden

If the task is being 'listened' at the time of the removal.. what should happen?

What do you mean by "task is being listened"? If a task is currently added as an active listener?

We should really remove all instances when they're the same?

Yes

Should we use predicates instead of instances as param? (i personally prefer predicates, the dev has more control over it)

Predicates are great, but in this case, not required, right? Listeners will be object instances so we can run .equals() to see if the object instances are the same reference. That should do the job.

Is there any thread restriction to remove a listener?

WendyConfig is not thread-safe at this time, as you can see. We will need to make the list of listeners and the mutating functions all thread safe. It would be difficult to see if adding thread safety code works until we have unit testing setup.

With that in mind, I am leaning towards waiting to merge this PR (since it's a nice-to-have and not required since we are using weak references) until we have unit testing setup and this PR can be fully tested. Agree?

Also, we need some tests on Wendy and examples on the example app..

Agree. That should be the top priority at this time.

the example app i don't have ideas for the remove functions uses

The example app used to exist for me to test that Wendy works while not yet having unit testing setup. Because unit testing is a high priority now, the example app will exist to show off best practices and give some example code that people can use to play around with the library. With that in mind, I do not believe we need to add functionality to the example app to remove listeners.

levibostian commented 5 years ago

Thanks for the PRs, @Grohden! You are helping to make Wendy better and better.

Grohden commented 5 years ago

@levibostian

What do you mean by "task is being listened"? If a task is currently added as an active listener?

I mean, if you write a listener that removes itself on one of its callbacks. (i do not expect someone to do this.. but ¯\(ツ)/¯)

With that in mind, I am leaning towards waiting to merge this PR (since it's a nice-to-have and not required since we are using weak references) until we have unit testing setup and this PR can be fully tested. Agree?

Agreed, i'm not in a hurry for this