Carousel - Filter duplicates and swallow exceptions

[mradzinski]

Hey @elihart!

I'm currently having some users with crashes due to duplicate models (honestly weird, it happens to just a few users out of 20K, so I'm having a hard time tracing the reason) and would be great if the carousel could filter duplicates and allow swallowing exceptions to avoid those sporadic crashes.

Is there a way to filter duplicates and swallow exceptions on a Carousel? I couldn't seem to find public methods which allowed me to access the underlying EpoxyController :/

Cheers and thanks!

[elihart]

Yep :) EpoxyController#setFilterDuplicates

we actually enable this in all our controllers (via a base controller) but we also implement onExceptionSwallowed to throw an exception in debug.

Our main source of this issue is in API quirks that are hard to control for and we don't want prod crashing for that

[mradzinski]

@elihart I'm actually talking about the Carousel. It extends from EpoxyRecyclerView AFAIK which uses EpoxyController, but I can't seem to be able to access that instance of EpoxyController that's being used on the Carousel.

As for crashes in production, the Carousel does crash here: com.airbnb.epoxy.DiffHelper.createStateForPosition (DiffHelper.java:234) which pretty much means Two models have the same ID. It's crashing in a production build, so I was expecting to be able to control that by swallowing the exception.

I'm using an extended version of Carousel for which I only disable the SnapHelper, so I don't think that has something to do with it.

[mradzinski]

Maybe since I'm already extending from Carousel I can do something like:

    override fun setController(controller: EpoxyController) {
        controller.setFilterDuplicates(true)
        super.setController(controller)
    }

Would that help enabling the exception swallowing in case of duplicates?

[elihart]

Yeah, so there are two ways to use the Carousel, either setting your own controller or setting a list of models. If you use the carousel via CarouselModel_ you can only set a list of models.

Anyway, I was thinking of the case where you set your own EpoxyController, in which case you would have access to setFilterDuplicates. Supporting deduping when just a list of models is provided seems important.

your override approach should work. The controller is only ever assigned via the setController so you should capture all cases.

Here is a general fix I have in mind:

• In SimpleEpoxyController enable duplicate filtering (this is the default controller AirRecyclerView uses)
• Allow a default error handler to be set statically on EpoxyController that will get called with all exceptions.

So by default duplicates would not crash, but a handler could be set up on app init if you want to crash in debug or log in prod.

Thoughts?

[mradzinski]

Yeah, I'm not setting my own EpoxyController, so the override approach seems to be the way to go. I just released a new prod version today, so far so good. I'll wait a day or two for adoption before calling it a victory though.

As for the fixes you have in mind, the second one seems to be extremely useful, specially when you have plenty of controllers and you want the same error handling behavior in all of them (that would be my scenario for example. I throw when on dev and just log when on prod).

Cheers and have a great weekend!

[elihart]

👍 great :)

I'll try to have those changes up soon, shouldn't be too bad

[mradzinski]

Hey @elihart A few days in production already and no crashes so far so I can attest that overriding setController did the trick, so feel free to close this issue (or leave it open while implementing those fixes).

Cheers and thanks for the support!

[elihart]

Good to know! Glad that worked. I definitely still want to add those other fixes, so I'll leave this open until then.

[elihart]

@mradzinski Addressed with #394

One difference is I didn't end up enabling duplicate filtering by default. I instead opted to allow people to also toggle it with a global setting.

Would appreciate your thoughts on the interface!

[mradzinski]

@elihart Interface looks good to me, pretty straight forward.

One question though: would setting a global default mean loosing granularity over individual controllers? I think giving granular control over certain controller (maybe having it not filtering duplicates) while allowing global defaults is the key. If that's the case then I totally love the idea of having defaults (mostly because I have this tendency of forgetting to override the exception swallowing method in the new controllers I write xD) and the way you implemented it.

[elihart]

@mradzinski thanks for looking! and individual controllers still retain the ability to manually override the global setting, so you should have that granularity. Let me know if you notice a case where you need more control

fwiw we used to have duplicate filtering disabled by default, and only enabled it if needed. but after we had too many prod crashes and had to keep doing one off enabling it made sense to just enable it globally (bug crash in debug and log in production)

[mradzinski]

@elihart Yeah, that's pretty much what we do, crash while debugging, log while in prod, but on some controllers we do some other stuff aside of logging, like provide default data if there's none and an exception occurs, so being able to still do that while also enabling standard logging by default is pretty much a life saver.

Thanks again for the incredible work you've put on Epoxy, TRULY appreciate it. Been using it for months already and proved to be, not only a great way for decoupling and keeping a codebase healthy, but a life saver in other scenarios where complexity was high enough it would've taken ages to handle everything through more standard mediums.

[elihart]

Thanks for the feedback! Great to hear it is working well for you, and thanks for the appreciation ❤️

If you have feature requests or other feedback please let me know :)

[TheLester]

@elihart Hi, thanks for great work with this library! I have one question regarding setFilterDuplicates method documentation:
If set to true, Epoxy will search for models with duplicate ids added during {@link #buildModels()} and remove any duplicates found. If models with the same id are found, the first one is left in the adapter and any subsequent models are removed
What if I need another logic - to replace duplicate with a new model instead of keeping the old? Any way to change this behavior?
Upd: Hm, after second thought I assume it will be handled by model's equals and hashcode checks

[elihart]

@TheLester I think an interceptor would be your best bet if you want to replace the duplicate model with something else - https://github.com/airbnb/epoxy/wiki/Epoxy-Controller#interceptors

You'll just have to write the logic manually