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

Possible issue with multiple short listeners #24

Open
guersam opened this issue May 4, 2013 · 6 comments
Open

Possible issue with multiple short listeners #24

guersam opened this issue May 4, 2013 · 6 comments
Labels

Comments

@guersam
Copy link
Contributor

guersam commented May 4, 2013

There are many onSomethingHappend with more than 1 callback like below (from here):

@inline def onItemSelected(f:  => Unit): V = {
  basis.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener {
    def onItemSelected(p1: AdapterView[_], p2: View, p3: Int, p4: Long): Unit = { f }
    def onNothingSelected(p1: AdapterView[_]): Unit = {  }
  })
  basis
}

As it invokes basis.setSomeListener which replaces previous listener, calling onNothingSelected with onItemSelected will cancel each other.

We might solve this with internal addSomeListener implementation. Do you have any idea to improve this?

@pocorall
Copy link
Owner

I wrote the listener-helper as you pointed out because:

  • It is simple that not supporting multiple listener-helper 😄
  • It saves less line-of-code when using multiple listeners

For example, instead of this:

  basis.setOnItemSelectedListener(new AdapterView.OnItemSelectedListener {
    def onItemSelected(p1: AdapterView[_], p2: View, p3: Int, p4: Long): Unit = { doSomething() }
    def onNothingSelected(p1: AdapterView[_]): Unit = {  }
  })

This saves LOC dramatically:

  basis.onItemSelected(doSomething())

Meanwhile, this can be an improvement as well, but less impressive:

  basis.onItemSelected(doSomething())
  basis.onNothingSelected(doAnother())

As you mentioned, addSomeListener approach can be a solution enabling this. But it costs runtime overhead on memory, CPU, and the size of compiled jar. I did not decided that it is worth doing in spite of these overheads.

I admit that it is not documented (non-) feature. One of the most pressing issue is documentation.

@guersam
Copy link
Contributor Author

guersam commented May 28, 2013

I agree. Then how about a macro shows a warning about using 'unsafe' listener in compile time?

@pocorall
Copy link
Owner

Developers might get too much warning. Warning usually indicates bad practice, but this is not the case. I think that proper documentation is better.

@pocorall
Copy link
Owner

Assigning the same listener multiple times is obviously a bad practice though. Is the Scala macro can handle this?

@guersam
Copy link
Contributor Author

guersam commented May 28, 2013

It will be hard unless the scope is narrow enough. How about to show compile-time warning by default, and allow disabling it by importing specific value? Might need some time though....

@pocorall
Copy link
Owner

If it takes some time, leave it and moving to other urgent issues would be wise. I will stay opened this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants