Adding views in decorators

[magneticflux-]

Adding new views in decorators that depend on the date seems extremely buggy.

Steps to reproduce:
Create a CalendarCellDecorator that adds a TextView to the CellView containing the provided date only if the day of the month is 30.

Expected behavior:
The date is the same as the date shown on the original TextView and the new view only appears the 30th of the month.

Actual behavior:
The date always says 30, but the view appears on dates that are not the 30th of the month.

[magneticflux-]

During breakpoint debugging and stepping through, I found that the issue wasn't always present. That may indicate a race condition, which would be extremely problematic.

[edenman]

@magneticflux- hard to say without more info. can you try checking out the repo and see if you can repro the issue in the sample app? if so, push your fork and ping me here and i'll take a look.

[magneticflux-]

@edenman Here is the repo that I'm trying it on, but I'll try to reproduce it in the sample app as well.

[edenman]

One thing that might be the issue: you're adding the view if calendar.get(Calendar.DAY_OF_MONTH) == 30 but you're not ever removing it. This means that when the row gets recycled, it'll still have that extra view.

[magneticflux-]

@edenman I looked around in the library's source for where rows were being recycled, but I couldn't find it. Could you explain where I should remove it?

[edenman]

@magneticflux- CalendarPickerView is a ListView, so MonthAdapter is responsible for the view recycling. Your decorator should just do something like:

if (calendar.get(Calendar.DAY_OF_MONTH) == 30) {
  // add custom view
} else {
  // check to see if custom view is there, remove it
}

[magneticflux-]

@edenman Removing the custom view in the else branch prevents the views from appearing on days that are incorrect, but now views are not appearing in places that they should after scrolling the screen down.

[edenman]

@magneticflux- not sure. try adding some logging to see what's happening?

[magneticflux-]

@edenman I'm going to try to isolate it down to the simplest setup that reproduces it in the sample app.

[magneticflux-]

@edenman Here it is: https://github.com/magneticflux-/android-times-square/tree/decorator-view-add-bug
To see the behavior, open the app and click on the "Decorator" button. Then, scroll up and down until buttons start showing up in odd places.

[edenman]

Not sure why, but I had to manually measure+layout the view to get this working. Maybe because you're manually constructing the view instead of inflating it with the parent set?

[edenman]

Anyway, you should be good to go

[edenman]

(i pushed the fix to your branch)

[magneticflux-]

@edenman The measure+layout solution worked for me, but it's extremely unwieldy when I'm trying to add several views inside of a LinearLayout. I tried using a LayoutInflater to attach a layout to the root (the CalendarCellView), but I'm still having the same problem. Then, I forced the MonthAdapter class to never recycle Views, and it started functioning as expected without any manual measure+layout hacks. Unfortunately, this causes rampant memory consumption due to many inflated views.
I'm not sure why re-inflating the month.xml layout inside of MonthView.create(...) fixes the issue, but it does. The two paths, recycling the view or creating a new one, both call monthView.init(...) which runs the decorators. Maybe extra cleaning on the recycled view must be done?

[edenman]

no idea. let me know if you figure it out!

[magneticflux-]

Upon inspecting the layout, the buttons are part of the view hierarchy but they have their width/height/measuredWidth/measuredHeight set to 0. I'm going to look through the measurement process of the custom views because I think that something might be missing there.

[magneticflux-]

@edenman I have come to the conclusion that the views are not being shown because the decorator is run during the onLayout pass. Unfortunately, I'm not sure to fix this issue.

[edenman]

@magneticflux- you might just want to use setCustomDayView to add your custom view to every cell and just have your decorator toggle the visibility

[edenman]

(or just manually measure/layout, hacky though it may be)

[magneticflux-]

@edenman FYI, I tested measure+layout with a LinearLayout and multiple children and it did not show up. I only measure/layout-ed the LinearLayout though.

Unfortunately, I was planning to add a dynamic number of views to each date so I could achieve a Google Calendar-like functionality. I'm going to try adding a ListView/RecyclerView to each cell and populate them with the decorator, but I'm worried that won't work either.

[magneticflux-]

Adding items in a ListView appears to work at first, but ultimately does not. It suffers from the same problem as the original: the views are somehow not cleared properly.

[magneticflux-]

In addition, the fact that decorators operate inside of a layout pass means that merely setting the background drawable of a cell results in dozens of warnings like the following:

06-30 16:37:40.032 12213-12213/com.squareup.timessquare.sample W/View: requestLayout() improperly called by com.squareup.timessquare.CalendarCellView{9a3559c VF....C.. ......ID 0,0-142,142} during layout: running second layout pass

I think a fix for this and several other issues would be to move all of the decoration loop out of the layout stage and later in the setup of the views, but I don't know how that could be accomplished.
This seems to relate to #103.