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

Is it a good practice to get current list inside PagingEpoxyController #314

Closed
pcqpcq opened this issue Oct 13, 2017 · 4 comments
Closed

Comments

@pcqpcq
Copy link
Contributor

pcqpcq commented Oct 13, 2017

I found nothing like getCurrentData() in PagingEpoxyController, so I create one like this:

public class MyController extends PagingEpoxyController<Foo> {

    // ...other stuff

    private PagedList<Foo> currentData;

    @Override
    public void setList(@Nullable PagedList<Foo> list) {
        super.setList(list);
        this.currentData = list;
    }

    protected PagedList<Foo> getCurrentData() {
        if (this.currentData != null) {
            return this.currentData;
        }
        return (PagedList<Foo>) Collections.<Foo>emptyList();
    }

}

with Placeholders disabled:

new PagedList.Config.Builder()
        .setPageSize(10)
        .setPrefetchDistance(50)
        .setEnablePlaceholders(false)
        .build())

On the other hand, there is getCurrentList() in PagedListAdapter

    @Nullable
    public PagedList<T> getCurrentList() {
        return this.mHelper.getCurrentList();
    }

And, before I make a pr:
Why there is nothing like this in PagingEpoxyController?
Is it a bad practice for PagingEpoxyController?

@pcqpcq pcqpcq changed the title Is it a good practices to get current list of PagingEpoxyController Is it a good practices to get current list inside PagingEpoxyController Oct 13, 2017
@pcqpcq pcqpcq changed the title Is it a good practices to get current list inside PagingEpoxyController Is it a good practice to get current list inside PagingEpoxyController Oct 13, 2017
@elihart
Copy link
Contributor

elihart commented Oct 14, 2017

I don't think this is bad practice, I just didn't consider it. Seems valid, thanks for the idea!

One concern, since both a List and PagedList can be set, which should be returned? There could be a separate method for each.

@elihart
Copy link
Contributor

elihart commented Oct 14, 2017

There could be getPagedList which returns the current list or null, and then getCurrentList which returns the current data being displayed - either the current paged list snapshot or the java list that was set ( or empty list if nothing is set). What do you think?

@pcqpcq
Copy link
Contributor Author

pcqpcq commented Oct 14, 2017

It's very nice, I've make a pr here: #317

@pcqpcq
Copy link
Contributor Author

pcqpcq commented Oct 17, 2017

closed as #317 being merged.

@pcqpcq pcqpcq closed this as completed Oct 17, 2017
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

2 participants