Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a way to remove listeners #48

Open
Grohden opened this issue Nov 29, 2018 · 4 comments · May be fixed by #54
Open

Provide a way to remove listeners #48

Grohden opened this issue Nov 29, 2018 · 4 comments · May be fixed by #54

Comments

@Grohden
Copy link
Contributor

Grohden commented Nov 29, 2018

I could not find any method in the beta that provides a way to remove a listener.. i have something like this:

RequestAllTasksListener { hadErrors ->
      if (hadErrors) {
        cancel(true)
      } else {
        download()
      }
    }.also {
      WendyConfig.addTaskRunnerListener(it)
      Wendy.shared.runTasks(null)
    }

And every time i need to call the function with this piece of code a listener is added.. one could say that i should only put a listener one single time, but for our needs this is not true, we need to listen to events, see if there's anything wrong and 'unsubscribe' after that..

@levibostian
Copy link
Owner

What are you using the task runner listener for? The use case that I created it was for UI purposes. If you wanted to show in the UI a status of the task runner such as a progress bar saying that your data is being saved.

With my use case described, you would only need 1 task listener attached to the UI and that's it.

What are you using the listener for if you don't mind me asking to get a better idea on how to solve this problem?


P.S. I didn't even think to add a remove listener function. Because Wendy uses weak references that solved the problem for my use case. After I get a better idea of what you are trying to do, I'll see what I can do to improve the library.

@Grohden
Copy link
Contributor Author

Grohden commented Nov 29, 2018

Our app here is kind of old, I'm implementing new stuff here while maintaining some of the old
code (this complicate things a bit XD) ... and the data synchronization part is one of this old codes, on the synchronization part we send all our new data to the server (one request at time), if no error occur we download a package (basically a snapshot of the remote DB at a given time) and update/delete the entities based on this package.

So basically i need to run all tasks and check:

  • on error: show error and not download the package
  • on success: download the package

That's basically why i need to listen and unsubscribe after that..
I was reading the wendy's source and i know that it uses weak references, but if i understand correctly, the code i sent on my first comment should be 'cleaned' after being used.. i don't even make a reference for the newly created object (ok, there's the it, but that's the only time).. Anyway, again, if understand correctly how WeakReferences should work, the code below should solve the problem:

    var listener: RequestAllTasksListener?

    listener = RequestAllTasksListener { hadErrors ->
      if (hadErrors) {
        cancel(true)
      } else {
        download()
      }
      listener = null
    }

    WendyConfig.addTaskRunnerListener(listener!!)
    Wendy.shared.runTasks(null)

I will test it soon, but a removeListener i would trust more than explicit setting the variable to null
Here's the task RequestAllTasksListener code if that help:

class RequestAllTasksListener(
    private val onFinish: (hadErrors: Boolean) -> Unit
) : TaskRunnerListener {
  private var hasRecordedErrors = false

  override fun allTasksComplete() {
    onFinish(hasRecordedErrors)
  }

  override fun errorRecorded(task: PendingTask, errorMessage: String?, errorId: String?) {
    hasRecordedErrors = true
  }

  override fun errorResolved(task: PendingTask) {
  }

  override fun newTaskAdded(task: PendingTask) {
  }

  override fun runningTask(task: PendingTask) {
  }

  override fun taskComplete(success: Boolean, task: PendingTask) {
  }

  override fun taskSkipped(reason: ReasonPendingTaskSkipped, task: PendingTask) {
  }
}

@Grohden
Copy link
Contributor Author

Grohden commented Nov 29, 2018

ok, that doesn't work, but at least i tried. I think that the weak reference will be freed only when the system decides that is needed.. so basically i will need to control on the listener itself if it has been ran once

@levibostian
Copy link
Owner

From what you stated above, "the synchronization part we send all our new data to the server (one request at time), if no error occur we download a package (basically a snapshot of the remote DB at a given time) and update/delete the entities based on this package." I have a possible idea.

Instead of having a listener that checks when the task runner is complete with all tasks, you can instead use Wendy.shared.getAllTasks().isEmpty after each API call and when it is empty, you can download the whole package.

That's a thought that might make things a little less complex and will fix your issue with the current Wendy code base.


However, I do agree there needs to be a way to remove a listener. That is just something that should be added anyways. Thank you for answering my question about what you use the listener for so I could understand your use case behind using it.

In the end, use whatever method works best for you. We should have a remove listener function anyways.

If the idea I stated in this comment works for your purpose, go ahead and use that to get by. If it will not work for you, you can make a PR or I'll be sure to make a fix for you quick so you can continue on with your app 😄.

Thanks for the PR. We do need a remove listener function.

@levibostian levibostian added this to the 0.3.0-alpha milestone Dec 8, 2018
@Grohden Grohden linked a pull request Dec 10, 2018 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants