search

turing-tech / MaterialScrollBar

An Android library that brings the Material Design 5.1 sidebar to pre-5.1 devices.
Apache License 2.0
778 stars 126 forks source link

MaterialScrollBar does not useful when you got many RVs with the same Id #48

Closed MarcinOrlowski closed 8 years ago

MarcinOrlowski commented 8 years ago

Hi,

MaterialScrollBar Is completely useless in the scenario where you have few RecyclerViews in the layout (i.e. tab layout), all using the same android:id. This is perfectly valid scenario, however using the same Id for more than object can cause some troubles. And you just stepped on that :)

For example let's have 2 RVs, both are using android:id="@+id/recycler". 1st is just recycler with plain adapter, no fancy stuff. But we want MaterialScrollBar to be used with 2nd one, so it uses different adapter and implements i.e. ICustomScroller etc. All is fine. Then you set up the layout, plant MaterialScrollBar. But then you to have add recycler reference:

app:msb_recyclerView="@id/recycler"

And now we have 50% of chance for crash due to uncaught CustomExceptions$AdapterNotSetupForIndicatorException.

The problem lies in this line:

https://github.com/krimin-killr21/MaterialScrollBar/blob/master/lib/src/main/java/com/turingtechnologies/materialscrollbar/MaterialScrollBar.java#L136

But things can go even worse, if adapters of both recyclers are implementing expected interface - in such case, scroller will be scrolling wrong recycler and showing hints from wrong dataset.

To fix this issue msb_recyclerView must not be mandatory at instantiation time, but at run time + you must be able to set the same from code (i.e. via setRecyclerView()) which will allow to wire scrollbar with proper recycler.

MarcinOrlowski commented 8 years ago

Hi, Any feedback on the issue?

turing-tech commented 8 years ago

I'll add it in, but I'm busy taking exams right now so I can't do it immediately.

MarcinOrlowski commented 8 years ago

That's fine. I was mostly wondering if you see this as bug and going to fix it or I shall look for other library. I can wait a bit longer - no real rush as of yet. Thanks for the answer and good luck with exams :)

MarcinOrlowski commented 8 years ago

Any ETA for this bugfix? I need this feature in my app so I need to know if I shall look for any other solution/lib or I can wait for your fix to come

turing-tech commented 8 years ago

I can start working on it in about a week. Expect it by the weekend after next.

turing-tech commented 8 years ago

Now you can omit the declaration of the RV in the XML and invoke setRecyclerView() during onCreate() or some other pre-render method.

MarcinOrlowski commented 8 years ago

Mind showing working sample? I experience crashes here after the update and all I changed was removing XML reference and calling setRecyclerView() in fragment's onCreateView(). Stacktracke does not help here really.