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 non-final IDs #605

Closed
ghost opened this issue Jun 2, 2016 · 10 comments
Closed

Support non-final IDs #605

ghost opened this issue Jun 2, 2016 · 10 comments

Comments

@ghost
Copy link

ghost commented Jun 2, 2016

I know this was covered in #2, but I have a real use case for it.

In our app, we retain presenters across configuration changes using a retained fragment. However, we don't want the presenters to have to care about views being destroyed and recreated, so we have a "view manager" object which implements the View interface and manages the View's lifecycle, like a mini-fragment.

Our view managers implement an interface like this:

interface ViewManager {

  View onCreateView(LayoutInflater inflater, ViewGroup container);

  void onDestroyView();
}

Then an implementation would look something like:

interface MyView {
  void setText(String text);
}

class ViewManagerImpl implements ViewManager, MyView {

  private String text = "my text";

  private TextView textView;

  @Override
  public View onCreateView(LayoutInflater inflater, ViewGroup container) {
    View view = inflater.inflate(...);
    textView = (TextView) view.findViewById(R.id.view2);
    textView.setText(text);
    return view;
  }

  @Override
  public void onDestroyView() {
    textView = null;
  }

  @Override
  public void setText(String text) {
    this.text = text;
    if (textView != null) {
      textView.setText(text);
    }
  }
}

I think it's clear how this model would benefit from Butter Knife - a more complicated view would have many calls to findViewById and would have to remember to set all view fields to null in onDestroyView.

Now, the reason I need non-final ID support is because of the build system we use at Google. The recommended practice is to create an Android library for every package rather than one big build target. This allows us to have package-specific resources and improves build times by caching builds. However, this means that all of our IDs are non-final, which means we cannot use Butter Knife without support for non-final IDs.

I have created #591 to add support, take a look and let me know what you think. It may also be possible to use the extensions feature proposed in #573, but that hasn't been updated in over two weeks.

@ghost
Copy link
Author

ghost commented Jun 2, 2016

One concern that came up in previous solutions was using getIdentifier makes the generated code potentially incorrect if an ID is changed without changing the annotation. #591 uses the generated R class directly, so invalid IDs cause a compile-time error.

@JackCho
Copy link

JackCho commented Jun 3, 2016

@GrahamRogers If you want to use ButterKnife in a library project, maybe you can check this plugin project: ButterCookie

@ghost
Copy link
Author

ghost commented Jun 3, 2016

@JackCho we don't use Gradle to build apps, so a Gradle plugin won't help. Thanks for the suggestion though.

@kageiit
Copy link
Contributor

kageiit commented Jun 5, 2016

Great to see work going on in this regard. We faced the same problem recently when trying to build with buck/bazel.

I am trying out an alternate approach (not a gradle plugin) that allows using non final ids in butterknife i.e support for libraries for all bind annotations including extensions. Will open a PR soon :)

This approach also does not need any modifications to the annotation classes themselves, so I believe it may be cleaner than what is proposed in #591

@ghost ghost mentioned this issue Jun 10, 2016
4 tasks
@ghost
Copy link
Author

ghost commented Jun 10, 2016

@JackCho I looked in more detail at your ButterCookie project and I thought I should let you know that making all the R.id fields final like you have is dangerous. I think it would cause an error if Android attempts to set the value of the ID to avoid a collision with another library. Or maybe Android uses clever reflection that ignores the field being final, which seems like a scary way to do it. If the value does get changed though, the value of the ID will then be different form the value used by the annotation, so it will bind the wrong view or no view at all.

ButterFork appears to be a safer alternative, it generates a B (for Binding) class which mirrors the structure of the generated R class but uses String as the type for IDs which are essentially references to the R fields. The implementation then becomes similar to my pull request. However, it uses a Gradle plugin to generate the B class, and I would prefer not to rely on the project keeping up-to-date with ButterKnife.

#497 and #573 are interesting, extension support for ButterKnife. While the pull request only supports listeners for now, if it was made to be more flexible it would be possible to create an extension for non-final IDs, allowing the implementation to be in a separate project from ButterKnife (and hence not be officially endorsed by the team). I think that would be the best long-term solution.

@JackCho
Copy link

JackCho commented Jun 11, 2016

@GrahamRogers maybe you didn't follow ButterCookie completely, ButterCookie as a gradle plugin, it does the following five things, not just the first one:
1 before java compile, change all fields in R to final, make sure *$$ViewBinder will be generated
2 after java compile, all *
$$ViewBinders generated should be replaced ID constant value with field in R
3 change all fields in R to non-final
4 copy source code to build dir, and delete all ButterKnife annotations
5 recompile source code

so good to go...

@JackCho
Copy link

JackCho commented Jun 11, 2016

@JakeWharton you may check this issue, supporting non-final ids does really work here, and nothing is changed when using ButterKnife

@kageiit
Copy link
Contributor

kageiit commented Jun 11, 2016

@GrahamRogers I am planning to open a PR this weekend with full library support using gradle with extensible hooks for both buck and bazel.

It would also support all butterknife-extensions out of the box. I suggest you to check it out

@kageiit
Copy link
Contributor

kageiit commented Jun 12, 2016

@GrahamRogers See #613

@JakeWharton
Copy link
Owner

Support is on master

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

3 participants