No support for programmatically generated views about epoxy HOT 18 CLOSED

No problem, I really enjoy this project as I think it is a very clean way to solve one of the most frustrating aspects of Android's recycler views. Last night I took some more time to research issues/problems others have had with annotation processing/code generation and their smooth integration into an IDE such as IntelliJ, AndroidStudio and Eclipse.

Here is my thought process on a Gradle plugin —

Yes, we currently run into the issue of the multi-module problem which can be solved (or so I believe) like so —
1. hook into the current build task graph with plugin
2. For each class that contains an epoxy annotation copy it to $projectDir/gen-src
3. Call doLast which will then call our annotation processor with $projectDir/gen-src on the classpath.
4. Once completed, we can simply delete $projectDir/gen-src and I think we should be Gucci.

What are your thoughts? I see it as this — we now have the ability to remove the epoxy-processor dependency since the plugin will handle it work (in all cases). Obviously, users can still choose to add the suggested grade plugin, or not.

There were a few other ways I was thinking could work that revolved around generating the sources through a plugin, but by compiling ourselves. (i.e. the processor & plugin live together). This means we can traverse the submodules and find all classes that contain annotations & have our Gradle plugin keep a counter which would provide a Unique id.

As far as your solution goes — I definitely like it as well! It may not be elegant per-se, but I think it's a great solution that can be iterated on. However, as you stated there is the slight possibility of collisions, and that is always a bummer when you have to add in a bug. Sometimes you gotta thug life it though, especially when the chances are basically non-existent.

from epoxy.

from epoxy.

@niccorder That would be great! Keep us posted on progress, I'm happy to look over a branch whenever you like.

from epoxy.

@niccorder No luck?

I am planning to go ahead with my basic solution for now to get this supported in the 2.0 release. I don't think multi module projects are too common, so it is fine to not support it completely for now.

from epoxy.

I like the idea of supporting this. I'm thinking that a model that subclasses EpoxyModel could support it. Something like

abstract class EpoxyModelWithView<T> extends EpoxyModel<T> {

         @Override
         protected int getDefaultLayout() {
             return 0;
         }

         abstract int getItemType();

         abstract T buildView(ViewGroup parent);
     }

I don't like the idea of having to manually give an item type. That could hopefully be generated.

I'm also thinking that we could automatically make a buildView implementation in the generated class that just returns a new instance of the view.

One other tricky part is that the adapter only has the item type onCreateViewHolder(ViewGroup parent, int layoutRes) when creating the viewholder, so we would need some way to link that to the model.

Alternatively, maybe we should separate the view building from the epoxy model, and have a factory interface that builds a view for a given item type.

Do you have an example use case or ideas for what this would look like?

from epoxy.

For an example usecase, I've been using Anko to create my views that's why I needed to have a way to use views instead of inflating layouts.

What I came up with but not really tested it yet is to have a constructor for EpoxyViewHolder that takes a view.

public EpoxyViewHolder(View itemView) { super(itemView); }

For the adapter maybe something like this

private SparseArray<EpoxyModelWithView> views = new SparseArray<>();

@Override
public EpoxyViewHolder onCreateViewHolder(ViewGroup parent, int layoutRes) {
    return new EpoxyViewHolder(views.get(layoutRes));
}
protected void addModel(EpoxyModelWithView<?> epoxyModelWithView) {
    super.addModel(epoxyModelWithView);
    views.put(epoxyModelWithView.hashCode(), epoxyModelWithView);
}

I can see that creating the ID manually might cause a lot of careless bugs. Maybe we could make use of hashCode()?

from epoxy.

using hashCode() for item types (maybe hashing the fully qualified name of the model) would be easiest, but I would worry about the chance of collisions. The chance would be exceedingly small, but if you got unlucky with multiple item types it just wouldn't work.

One other question about item types - do you foresee a model needing to programatically change its model type? Maybe the programmatic view it builds can differ and it needs to have its item type change? In that case we can't tie the item type to the model class.

from epoxy.

I think models shouldn't change its item type. Basically what is being done is, instead of inflating, the view is being created programmatically. So whatever changes with the view it's like changing its properties like how we do it in xml. I think it would be better for the model to not really care about how the view is built.

For the ids, how about annotation processing? A model is annotated with a string (something like @id("view_type")) that will be a basis of its id. The annotation will then generate int view types based on the annotation. Also, it needs to check for duplicates and throw them as errors during compile time. Just an idea, I don't have any experience in creating annotation processing stuff.

from epoxy.

Generating the ids with the annotation processor would be ideal. That was my first thought. The only complication I can think of is handling multi modules. Since modules are processed independently the annotation processing in one module doesn't know about anything in another module. This makes it hard to avoid collisions.(http://stackoverflow.com/questions/38492208/multi-module-annotation-processing-in-android-studio)

I can't think of a good way to manage the multi module case right now but will keep thinking. For a first iteration we could either not support multi module, or require an item type to be set manually.

from epoxy.

Also, going back to you proposal of

protected void addModel(EpoxyModelWithView<?> epoxyModelWithView) {
    super.addModel(epoxyModelWithView);
    views.put(epoxyModelWithView.hashCode(), epoxyModelWithView);
}

This isn't ideal since models can also be added directly to the models ArrayList. Since a common pattern is to remove all models and rebuild/add them when data changes it would also mean doing this operation for every model when data changes. I would like to think of a more optimized way of doing it.

from epoxy.

Well we could do something like this to handle adding and removing models

protected void addModel(EpoxyModelWithView<?> epoxyModelWithView) {
    super.addModel(epoxyModelWithView);
    views.put(epoxyModelWithView.getId(), epoxyModelWithView);
}

protected void addModels(EpoxyModelWithView<?>... modelsToAdd) {
    super.addModels(modelsToAdd);
    for (EpoxyModelWithView epoxyModelWithView : modelsToAdd) {
        views.put(epoxyModelWithView.getId(), epoxyModelWithView);
    }
}

protected void addModelsFromCollection(Collection<EpoxyModelWithView<?>> modelsToAdd) {
    super.addModels(modelsToAdd);
    for (EpoxyModelWithView epoxyModelWithView : modelsToAdd) {
        views.put(epoxyModelWithView.getId(), epoxyModelWithView);
    }
}

protected void insertModelBefore(EpoxyModelWithView<?> modelToInsert, EpoxyModel<?> modelToInsertBefore) {
    super.insertModelBefore(modelToInsert, modelToInsertBefore);
    views.put(modelToInsert.getId(), modelToInsert);
}

protected void insertModelAfter(EpoxyModelWithView<?> modelToInsert, EpoxyModel<?> modelToInsertAfter) {
    super.insertModelAfter(modelToInsert, modelToInsertAfter);
    views.put(modelToInsert.getId(), modelToInsert);
}

protected void removeModel(EpoxyModelWithView<?> model) {
    super.removeModel(model);
    int index = getModelPosition(model);
    if (index != -1) {
        views.remove(index);
    }
}

protected void removeAllAfterModel(EpoxyModelWithView<?> model) {
    super.removeAllAfterModel(model);
    List<EpoxyModel<?>> modelsToRemove = getAllModelsAfter(model);
    for (EpoxyModel<?> epoxyModel : modelsToRemove) {
        if(epoxyModel instanceof EpoxyModelWithView) {
            views.remove(((EpoxyModelWithView) epoxyModel).getId());
        }
    }
}

from epoxy.

I have the ideas on how to do this pretty fleshed out and have started working on it. I'm on vacation and traveling for the next two weeks though so I probably won't have anything to show until after that

from epoxy.

@elihart I have been meaning to solve the issue with the multi-project build for a while now, but have been on vacation/extremely busy at work. I suspect you are on the right path already, but I'd highly recommend you look at yodle/griddle's fantastic solution to a VERY similar problem.

If I get time this weekend, I have a very basic outline of a Gradle plugin that will solve this issue. I'll clean it up and link it here for you to reference/build upon/ignore 😜 . Feel free to reach out to me for any help/work to fix this bug along the way as well!

EDIT - I was mistaken and thought we could solve this issue similar to the way that griddle does, but after spending some time on this I think I have found a solution that will fix both the multi-module linking, as well granting us the ability to guarantee a unique id. Seems as though the butter master @JakeWharton has already solved this issue https://github.com/JakeWharton/butterknife/tree/master/butterknife-gradle-plugin

from epoxy.

@niccorder Interesting idea to use a gradle plugin! I hadn't considered that. We use the butterknife gradle plugin but I've never looked at the source until now.

Thanks for looking into this and contributing; I'm curious to see your approach. I don't quite see how it would work yet. For Butterknife there is just the one R file that needs to be copied, but for the Epoxy use case we would have an unknown number of EpoxyModel classes.

I've also thought about generating an xml file for each module with id values in it for each model with a programmatic view. Then that model would have a R.dimen.... value to guarantee a unique id. I've never generated XML in an annotation process, and I'm not sure how feasible it is to generate an dimen file like that. I would also like to avoid it since AAPT is much slower than java compilation so keeping all the files in Java would be great.

The approach I've started working on is simple, but not very elegant. https://github.com/airbnb/epoxy/compare/eli/views

The base class looks like this

public abstract class EpoxyModelWithView<T extends View> extends EpoxyModel<T> {

  @Override
  protected final int getDefaultLayout() {
    return 0;
  }

  @Override
  public EpoxyModel<T> layout(@LayoutRes int layoutRes) {
    throw new UnsupportedOperationException();
  }

  @Override
  protected abstract int getViewType();

  @Override
  protected abstract T buildView(ViewGroup parent);
}

I haven't finished the annotation processor work yet. The generated class will implement getViewType with just a hashcode based on the file name, and make it negative so it won't clash with generated layout resource ids like R.layout.... I figure the chance of a hash collision is extremely small. If somebody wants to specify their own view type id they can override the getViewType method. The generated class will only implement it if it isn't already implemented.

The change in the adapter is also fairly simple, if a little bit ugly. The current RecyclerView implementation always calls getItemViewType right before calling onCreateViewHolder. In onCreateViewHolder we only have the item type, but no good way to link that to the related model. I really want to avoid any sort of look up in that method since it is called often and I don't want to slow down performance. My approach just remembers that last model referenced in getItemViewType and uses it when creating the view holder. This will work as long as the RecyclerView implementation doesn't change drastically. If it does change I added fallback code to look up the correct model if necessary. This approach is the fastest thing I could think of, and shouldn't hurt performance at all or use hardly any extra memory.

What do you think of these ideas?

from epoxy.

Thanks for outlining your ideas, that does seem like it could work. I'm not very familiar with gradle scripts, so I'm not clear on what exactly steps 2 and 3 would look like but I would love to see your attempt at it!

Especially for step 3, is it possible to manually invoke an annotation processor class via the script? That would be great if so since we could reuse the existing processor. Looking at how Butterknife's plugin works though it uses the javaparser library - if we went that route it seems like we'd have to rewrite the whole processor.

Your idea to combine the plugin and processor is also interesting. That could be something good to try if just using the plugin doesn't work.

How much work do you think the plugin approach would be? If you're willing to do it that's great! Otherwise I probably wouldn't have much time to spend on it so I would settle for my solution for now.

from epoxy.

@niccorder Have you been able to spend any more time on the gradle plugin?

from epoxy.

I'm also curious about this :)

from epoxy.

This will be released in 2.0 #151. I still need to add the annotation processor portion to generate view type, but that should be easy.

from epoxy.