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

Support listeners with identical names #248

Closed
tomxor opened this issue Mar 21, 2015 · 4 comments
Closed

Support listeners with identical names #248

tomxor opened this issue Mar 21, 2015 · 4 comments

Comments

@tomxor
Copy link
Contributor

tomxor commented Mar 21, 2015

Following #148 and #245, there is a problem with declaring listener annotations having the same names for different targets.

There are different types of targets having listeners with the same names, yet those don't expect the same amount/types/order of arguments, or even if they do, it is possible that there are/will be definitions for multiple identical listener interfaces for different types of targets. So eventually you end up with something like RadioGroup.OnCheckedChangeListener and CompoundButton.OnCheckedChangeListener. I was able to come up with the following solutions given how Butter Knife works:

A: Define listener annotations in packages corresponding to the target type. Examples:

  • butterknife.compoundbutton.OnCheckedChanged
  • butterknife.radiogroup.OnCheckedChanged

Probably the biggest downside of this approach is that if you wish to use two (or more) annotations having the same name in the same file, you will end up specifying the fully qualified name of at least one of them every time you use it.

B: Prefix the annotation name with the corresponding target type. Examples:

  • butterknife.CompoundButtonOnCheckedChange
  • butterknife.RadioGroupOnCheckedChange
  • butterknife.AdapterViewOnItemLongClick
  • butterknife.ViewOnClick
  • butterknife.ViewOnFocusChanged

Both A and B are about declaring multiple annotations, following the rule of one annotation per target. The only benefit with these approaches as I see it is the fact that this doesn't require any changes to the underlying processor.

Obviously both approaches require modifying all existing listener annotations to follow one of these formats in order to keep the API consistent.

C: Support multiple target types per annotation.

Consider the current code for OnCheckedChanged.java:

@ListenerClass(
    targetType = "android.widget.CompoundButton",
    setter = "setOnCheckedChangeListener",
    type = "android.widget.CompoundButton.OnCheckedChangeListener",
    method = @ListenerMethod(
        name = "onCheckedChanged",
        parameters = {
            "android.widget.CompoundButton",
            "boolean"
        }
    )
)
public @interface OnCheckedChanged {
  /** View IDs to which the method will be bound. */
  int[] value() default { View.NO_ID };
}

If we add support to the processor for multiple targets, we could use something like the following:

@ListenerClassList({
    @ListenerClass(
        targetIdentifier = "CompoundButton",
        targetType = "android.widget.CompoundButton",
        setter = "setOnCheckedChangeListener",
        type = "android.widget.CompoundButton.OnCheckedChangeListener",
        method = @ListenerMethod(
            name = "onCheckedChanged",
            parameters = {
                "android.widget.CompoundButton",
                "boolean"
            }
        )
    ),
    @ListenerClass(
        targetIdentifier = "RadioGroup",
        targetType = "android.widget.RadioGroup",
        setter = "setOnCheckedChangeListener",
        type = "android.widget.RadioGroup.OnCheckedChangeListener",
        method = @ListenerMethod(
            name = "onCheckedChanged",
            parameters = {
                "android.widget.RadioGroup",
                "int"
            }
        )
    )
})
public @interface OnCheckedChanged {
  /** View IDs to which the method will be bound. */
  int[] value() default { View.NO_ID };

  String target();
}

This way, users of the library have to explicitly specify the type of listener they're interested in by providing a listener target identifier, for example:

@OnCheckedChanged(value = R.id.whatever, target = "RadioGroup")
void whatever() {
  // ...
}

Sure, this means slightly more code (which kind of works against boilerplate reduction), but I think this is a much more generic solution that could also be beneficial in the future for other use cases.

Note that the listener identifier should probably be an enum or a different type of constant provided by the library.

One final note is that for listeners with only one available target type it will be used by default.

If C sounds like a 👍 I would be happy to submit a PR introducing this functionality.

Thoughts?

@tomxor tomxor changed the title Support listeners with the same name Support listeners with identical names Mar 21, 2015
@tomxor
Copy link
Contributor Author

tomxor commented Apr 23, 2015

Could you please provide some input on this @JakeWharton ?

@neersighted
Copy link

👍, I want to use @onPageChange with a sliding tab strip widget.

@saadfarooq
Copy link

👍

1 similar comment
@Bkrickl
Copy link

Bkrickl commented Sep 15, 2016

+1

@tomxor tomxor closed this as completed Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants