luizgrp / SectionedRecyclerViewAdapter

An Adapter that allows a RecyclerView to be split into Sections with headers and/or footers. Each Section can have its state controlled individually.
MIT License
1.68k stars 372 forks source link

Does it make sense to provide setItemResourceId for Section class #45

Closed yccheok closed 7 years ago

yccheok commented 7 years ago

Currently, I have a use case.

The content of item, will based on network result from server. Its code more or less look as the following.

watchlistSection.setState(Section.State.LOADING);
...
...
// Perform some networking operation, to return user current status.
...
...
watchlistSection.setItemResourceId(getBestItemResourceIdBasedNetworkingOperationResult());
watchlistSection.setState(Section.State.LOADED);

Currently, the only way to provide item resource id for Section, is via constructor. We do not have setItemResourceId.

Does it make sense to add the following code in Section ?

// Section.java
public void setItemResourceId(int itemResourceId) {
    this.itemResourceId = itemResourceId;
}
luizgrp commented 7 years ago

Hi @yccheok,

Having the setItemResourceId will lead people to think that they can change the layout of the item dynamically. Changing the layout of the items before having any items displayed on the RecyclerView is fine, the problem is changing after items are displayed, the RecyclerView will send the old ViewHolder to the onBindViewHolder method.

I think this is related to #9. Do you think their suggestion would work for your scenario?

yccheok commented 7 years ago

Do you mean that, for #9, we need to modify Section class, to have the following abstract methods (or non abstract but designed to be override-able)?

public abstract RecyclerView.ViewHolder getItemViewHolder(View defaultItemView, int viewType)
public abstract int getItemViewType(int position)

Or, to reduce burden of developer to implement abstract method, the 2 new methods come with default implementation

public RecyclerView.ViewHolder getItemViewHolder(View defaultItemView, int viewType) {
    return getItemViewHolder(defaultItemView);
}

public int getItemViewType(int position) {
    return 0;
}

We also need to modify SectionedRecyclerViewAdapter, from

private RecyclerView.ViewHolder getItemViewHolder(ViewGroup parent, Section section) {
    View view = LayoutInflater.from(parent.getContext()).inflate(section.getItemResourceId(),
            parent, false);
    // get the item viewholder from the section
    return section.getItemViewHolder(view);
}

to

private RecyclerView.ViewHolder getItemViewHolder(ViewGroup parent, Section section, int position) {
    View view = LayoutInflater.from(parent.getContext()).inflate(section.getItemResourceId(),
            parent, false);
    // get the item viewholder from the section
    return section.getItemViewHolder(view, section.getItemViewType(position));
}

However, I feel that will add complexity to this library, and discourage adoption of this library.

Is it possible to still have setItemResourceId in Section, but throw run-time exception, if layout has been inflated?

luizgrp commented 7 years ago

Hey, just a follow up on this:

Is it possible to still have setItemResourceId in Section, but throw run-time exception, if layout has been inflated?

Yes, it would be possible and it sounds like a simple idea. However it wouldn't be a solution for the request in #9. I would like to put this on hold until we define a solution that could potentially be used for both. If we find out that there is no good solution to attend both then we go forward with this.

I also liked your proposal for for #9 and I agree with your statement that will add complexity and discourage adoption. Maybe we could create a separate ComplexItemsSection class with those capabilities and keep the old Section and StatelessSection alone?

What do you think of the proposed solution by @percula in #9?

yccheok commented 7 years ago

@luizgrp Currently, I tackle my use case using current production library. What I did is I still use a single loaded resource id. However, in my loaded resource, it contains multiple views. I just turn on/ turn off the view during run-time, based on network result.

So, having setItemResourceId is not a real concern for me, (at least at this moment)

I'm interested in looking into @percula #9. I will spend some time to look into it, and provide feedback if I have any.

luizgrp commented 7 years ago

Thanks for the follow up, that's the way it should be in my opinion 👍