evrencoskun
Some issues with scrolling and adding/removing rows
Merhabalar,
Oncelikle bu library ile ilgili yaptiginiz her sey icin tesekkur ederim. Bu ise ciddi zaman ayirarak benim gibi yazilimcilari binlerce kod satiri yazmaktan kurtariyorsunuz. Bu yuzden tekrar tesekkur ederim. Buradan sonra Ingilizce devam edecegim, belki benzer sorunlarla karsilasmis birilerine yardimci olabilir.
The library works really well. However, I have found some issues, three of which I have implemented a workaround for. I'm hoping we can come up with a solution or confirm that it's not possible to fix them. I can definitely dig around to the library and see if I can find what the issue is, but I wanted to create this thread first in case you or some other person knows what's wrong.
I'll give a little bit of background on how I'm using the library first. I basically have some rows that are expandable, which when expanded show more rows. You can also expand the sub-rows further if possible. So it goes like row (set) > subrow (interval) > sub-subrow (length group). The gifs will make it more clear how it's designed. Here are the issues I found:
1. Scroll snaps to start and end
Here's a gif showing the issue.
This also happens with vertical scrolling, not just horizontal.
Steps to Reproduce:
- Expand a set from a data cell (so don’t press the row header to expand, expand from the right of the header separator)
- Immediately scroll right or down as the set is being expanded/collapsed
- Observe the horizontal scroll state (it should snap to first and last columns immediately)
Below are some logs I noted down when I found the issue. Cell means when you click on a cell (i.e. onCellClicked is invoked), whereas row header means when onRowHeaderClicked is invoked.
Row header expanded:
D/VerticalRecyclerViewListener: mRowHeaderRecyclerView scroll listener added
Row header collapsed:
W/CellRecyclerView: mIsVerticalScrollListenerRemoved has been tried to add itself before remove the old one
D/VerticalRecyclerViewListener: mRowHeaderRecyclerView scroll listener added
Cell expanded:
D/VerticalRecyclerViewListener: mRowHeaderRecyclerView scroll listener removed from last touched
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener added
D/HorizontalRecyclerViewListener: Scroll listener has been added to 8 at action down
Cell collapsed:
W/CellRecyclerView: mIsVerticalScrollListenerRemoved has been tried to add itself before remove the old one
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener added
D/HorizontalRecyclerViewListener: Scroll listener has been removed to 8 CellRecyclerView at last touch control
D/HorizontalRecyclerViewListener: Scroll listener has been added to 20 at action down
Row header expanded and scroll right:
E/CellRecyclerView: mIsVerticalScrollListenerRemoved has been tried to remove itself before add new one
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener removed from last touched
D/VerticalRecyclerViewListener: mRowHeaderRecyclerView scroll listener added
D/VerticalRecyclerViewListener: mRowHeaderRecyclerView scroll listener removed from last touched
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener added
D/HorizontalRecyclerViewListener: Scroll listener has been added to 29 at action down
D/HorizontalRecyclerViewListener: Scroll listener has been removed to 29 at onScrollStateChanged
E/CellRecyclerView: mIsVerticalScrollListenerRemoved has been tried to remove itself before add new one
D/VerticalRecyclerViewListener: mRowHeaderRecyclerView scroll listener removed from last touched
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener removed from last touched
Cell expanded and scroll right:
E/CellRecyclerView: mIsVerticalScrollListenerRemoved has been tried to remove itself before add new one
D/VerticalRecyclerViewListener: mRowHeaderRecyclerView scroll listener removed from last touched
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener added
D/HorizontalRecyclerViewListener: Scroll listener has been added to 32 at action down
W/CellRecyclerView: mIsVerticalScrollListenerRemoved has been tried to add itself before remove the old one
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener added
D/HorizontalRecyclerViewListener: Scroll listener has been added to 20 at action down
D/HorizontalRecyclerViewListener: Scroll listener has been removed to 20 at onScrollStateChanged
D/VerticalRecyclerViewListener: mCellRecyclerView scroll listener removed from last touched
Below is the logic of how I handle expanding/collapsing. Note that I use add and remove functions for rows, as opposed to hide and show. That's how I did it back then, and because I was on a deadline, I couldn't play around with hiding and showing too much. I am going to look into refactoring and possibly optimizing it to use hide and show instead, but that will take some time. Not sure if it would fix this issue though.
// add the rows
mRowHeaderItems.addAll(startIndex, tableViewSubData.rowHeaderCells)
mCellItems.addAll(startIndex, tableViewSubData.dataCells)
addRowRange(startIndex, tableViewSubData.rowHeaderCells, tableViewSubData.dataCells)
rowHeader.isExpanded = true
changeRowHeaderItem(position, rowHeader)
// required to adjust separators for data cells
for (i in 0 until mColumnHeaderItems.size) {
changeCellItem(i, position, mCellItems[position][i])
}
This is just the logic to expand a row, but collapsing works similarly (instead removes from lists).
For some reason, this issue only occurs if you expand a row from the cell recycler view. It doesn't seem to occur when I expand from row header recycler view (or at least I couldn't reproduce it by doing so). Not sure what's going on, but I'm thinking maybe the scroll listeners are cleared and don't get reassigned or something. Would appreciate any help!
2. savedInstanceState crash
I think this issue is related (or actually the same) to #185. Basically what happens is, if the activity is being restored and the fragment that had the table view is part of the savedInstanceState, the app crashes due to a Parcelable issue. The linked issue actually has more detail on it so I'll leave it at that.
Right now my workaround is to just not use savedInstanceState at all. I wasn't using it for anything in particular before, so this didn't cause much issues. However, it is a crash, so would be nice to find a fix for it.
3. Outdated data when expanding/collapsing (i.e. adding and removing rows)
The issue is that, upon adding rows, the cells are not getting the updated row header information, so they have the wrong data when you scroll (e.g. a row with a different view type shows data from another view type). I tried using the "update row header" parameter when removing rows; however, it didn't work (and actually messed up the animation).
One thing about this issue is that, it works fine if you don't scroll down initially (thinking something to do with the rows not being bound yet). However, it is broken if you scrolled down first (and back up) and expanded a row (i.e. it didn’t work after the views were initially bound).
I fixed this issue with a workaround, where I listen to the animation of the rowHeaderRecyclerView (upon row insertion/removal), and notify the cellRecyclerView of data set change when the animation finishes (i.e. calling notifyDataSetChanged() on it).
This is not a perfect fix, since if the user scrolls down immediately after expanding a row, they can see the "outdated" data for a split second. But I figured it was better than no animation at all (calling notifyDataSetChanged() cancels any animations on the view). Is this a familiar issue that we can look into? Would be nice to fix it if so.
4. Issues with fixed layout heights in different screen size/zoom modes
This is relatively minor, as the workaround works pretty well. It might be something to be informed of though, in case it comes up in the future. I found that layout heights that are fixed (specified in the layout xmls) don't work as expected for different screen size/zoom modes.
By default, phones come with the default screen size setting (or "zoom", depending on manufacturer). You can change this to be smaller or larger (e.g. smaller meaning you'll see more stuff on the screen). When this setting is not set to default, rows start getting misaligned as you scroll (row header vs cell height mismatch), even though both layouts have the same fixed height declared.
Not sure if this is a library issue that can be fixed easily, but the workaround is to set the height dynamically for each cell when onBindRowHeaderViewHolder and onBindCellViewHolder are called. Just wanted to note in case someone faces this issue in the future.
5. Can vertical scroll independently, causing misalignment of rows
I'm not sure if this is fixable to be honest, I feel like it can only have a workaround instead of an actual fix. Basically, the issue is you can misalign the rows by having one hand touching the right side (i.e. cellRecyclerView) and scrolling on the left side (i.e. rowHeaderRecyclerView).
Here's a gif to see what I mean.
The workaround would be to not propagate touch events if there already is one that is ongoing (i.e. wait for ACTION_UP event if there was an ACTION_DOWN or something). I think this has to be a touch handler on the TableView itself though, so not sure if it's easy to implement or would cause other issues. Basically need to prevent users from scrolling on the row headers if they are touching the cells. I haven't tried implementing a workaround for it yet, but just noting here in case we want to investigate. Would like to get your thoughts on whether it's an issue that can be fixed easily.
Sorry for the relatively long post. These are all the issues I faced so far. Would really appreciate your help (or anybody else who knows what's going on for that matter). Let me know if you need more details and I'd be more than happy to provide more information and help out with looking into why these issues might be happening.
Looking forward to your response. Thanks again for the amazing work you've done on the library!
Hello again @evrencoskun , any ideas about any of these issues? It would really help in having a head start for fixing them. Otherwise I am gonna have to fork the library and try to look into the issues myself.
Selamlar @epekel
Geç geri dönüşüm için çok üzgünüm. :/
- Could you please reupload your first gif to be more understandable?
- Please check out with the newest version which 0.8.9
- Could you give me a sample repository to check your issue on my development environment? Because I couldn't create your case on my demo app.
- Could you please reupload the gif, please?
İyi Haftalar
Tekrardan selamlar @evrencoskun,
Estagfurullah, sonucta bu arta kalan zamaninizda ilgilendiginiz bir sey. Ben de anca geri donus yapabiliyorum kusura bakmayın.
- Here’s the reuploaded gif: https://imgur.com/Knghcfm
- Thanks for the fix! I’ll be sure to upgrade to the new version soon.
- I unfortunately cannot give a sample repository. However, here are some details on my implementation, and a gif to help see what’s going on when I remove the
notifyDataSetChangedcall. Here’s a gif to see what the issue is: https://imgur.com/gq1K6Zp
Here’s the logic to add rows (i.e. expand a row):
// add the rows
mRowHeaderItems.addAll(startIndex, tableViewSubData.rowHeaderCells)
mCellItems.addAll(startIndex, tableViewSubData.dataCells)
addRowRange(startIndex, tableViewSubData.rowHeaderCells, tableViewSubData.dataCells)
rowHeader.isExpanded = true
changeRowHeaderItem(position, rowHeader)
// required to adjust separators for data cells
for (i in 0 until mColumnHeaderItems.size) {
changeCellItem(i, position, mCellItems[position][i])
}
And the logic to remove rows (i.e. collapse a row):
// remove the rows
val itemCount = tableViewSubData.rowHeaderCells.size
mRowHeaderItems.subList(startIndex, startIndex + itemCount).clear()
mCellItems.subList(startIndex, startIndex + itemCount).clear()
removeRowRange(startIndex, itemCount, false)
rowHeader.isExpanded = false
changeRowHeaderItem(position, rowHeader)
// required to adjust separators for data cells
for (i in 0 until mColumnHeaderItems.size) {
changeCellItem(i, position, mCellItems[position][i])
}
- Here’s the reuploaded gif for item 5: https://imgur.com/Qp0sjgx
I also have a question. I briefly mentioned in item 1 that I am using add and remove methods to handle expanding and collapsing a row (to show the sub-rows). Would it be better (in terms of efficiency and robustness) to use show and hide instead? Off the top of my head, I’d say the difference is time vs space. Please correct me if I’m mistaken (as I don’t fully know how these operations work internally in the library), but adding and removing will take more time due to higher cost of operations, and showing and hiding will take up more memory due to everything already being in the data structure, which could be undesirable when dealing with large data. Having add/remove allows for only increasing the size when the user actually expands a row, but the tradeoff is slower operations probably. So, just wondering what would be your recommendation for this use case of having expandable rows that reveal more detailed data: add/remove or show/hide? Or do they not have that much difference in terms of performance?
Tesekkurler, iyi haftalar!
Note: all the gifs are recorded on version 0.8.8
I am having the same issue, what seems to be the problem here? I think it is caused by calling changeCellItem function.
I fixed the first issue by moving the mIsMoved = true; to onScrollStateChanged instead of setting it when ACTION_MOVE occurs in HorizontalRecyclerViewListener. So my onScrollStateChanged looks something like this:
`@Override public void onScrollStateChanged(@NonNull RecyclerView recyclerView, int newState) { super.onScrollStateChanged(recyclerView, newState);
if (newState == RecyclerView.SCROLL_STATE_IDLE) {
// Renew the scroll position and its offset
renewScrollPosition(recyclerView);
recyclerView.removeOnScrollListener(this);
Log.d(LOG_TAG, "Scroll listener has been removed to " + recyclerView.getId() + " at "
+ "onScrollStateChanged");
mIsMoved = false;
// When a user scrolls horizontally, VerticalRecyclerView add vertical scroll
// listener because of touching process.However, mVerticalRecyclerViewListener
// doesn't know anything about it. So, it is necessary to remove the last touched
// recyclerView which uses the mVerticalRecyclerViewListener.
boolean isNeeded = mLastTouchedRecyclerView != mColumnHeaderRecyclerView;
mVerticalRecyclerViewListener.removeLastTouchedRecyclerViewScrollListener(isNeeded);
}
if(newState == RecyclerView.SCROLL_STATE_DRAGGING){
mIsMoved = true;
}
}`
I have noticed that ACTION_MOVE does not necessarily mean that the CellRecyclerView has actually scrolled.
@epekel @evrencoskun Hi, I have to implement expand, collapse functionality in my code. could you share me full code where you have made changes and github link will be helpful? thanks.
