pakerfeldt / android-viewflow

A horizontal view scroller library for Android
1.78k stars 694 forks source link

Get previous view in ViewSwitchListener.onSwitched #44

Open tadfisher opened 13 years ago

tadfisher commented 13 years ago

I would like to see an additional parameter in ViewSwitchListener.onSwitched to get the view that's being switched from, so I can manipulate it as soon as it's not visible. I implemented it by taking a reference to the current view at the beginning of ViewFlow.setSelection and ViewFlow.postViewSwitched and passing it along to the listener, but I'm not so sure this is the correct thing to do.

pakerfeldt commented 13 years ago

That might make sense. But consider this scenario.

You have 10 elements in the adapter and side buffer is set to 1 and you're currently focusing on element 5. Hence, ViewFlow has loaded view 4, 5, 6. Then, let's assume setSelection(1) is called. That would result in the currently loaded views: 0, 1, 2, hence, the previously shown View 5 is not loaded any more.

Question: What would be passed as previousView in the new onSwitched method? I am very tempted to answer null on that since the ViewFlow has removed the view from the buffer and should hold no reference to it. Passing a view that is expected to be freed from memory doesn't sound too good.

tadfisher commented 13 years ago

I agree with this, and passing null will also prevent memory leaks caused by references on the application side. Some might view null-valued parameters as poor API design, however. Google does it anyway, such as in adapter-view binding.

pakerfeldt commented 13 years ago

I have the fix almost done but need to close down for the evening. Hopefully I will push it tomorrow.

tadfisher commented 13 years ago

Thanks. I was just thinking that a second callback might be more appropriate, since taking these references might influence GC unnecessarily... for example, if you take a reference to the view prior to removing it from the buffer. But checking for this case would also prevent that.

pakerfeldt commented 13 years ago

First of all, a previous view will never be passed if it was just removed.

But we still might have the case that a view is removed later on, if the user is storing a reference to the view or doing some asynchronous task. But this issue is applicable for the current view as well. Separating the interface into two callbacks won't solve that problem I think. We need to remember that ViewFlow is re-using old Views as well when appropriate. Therefore, modifying a View from a ViewSwitchListener asynchronously might have strange effects if ViewFlow is trying to re-use the View before the asynchronous task is complete.

If you have control over both the ViewSwitchListener and the Adapter you can avoid these situations by checking which View is being re-used when the getView(...) method is called in the Adapter. Better yet, if you can modify the View synchronously, this will never occur.

I should probably clarify things in the Javadoc for the ViewSwitchListener.

pakerfeldt commented 13 years ago

Implementing this in postViewSwitched (which is called when the user swipes) is very straight-forward. But I just figured out, doing this in setSelection requires bigger changes. In setSelection I'm taking a shortcut and remove all views from the buffer, and then request to get a new view from the adapter for each element. Of course re-using the actual view objects, but still, if I would simply save the previous view in the beginning of setSelection, that view would no longer correspond to the previous view as it has been re-used by another (arbitrary) element.

I certainly need to re-do setSelection anyway because it is very sub-optimal. Especially if someone uses setSelection to just go back and forth one view at a time, as setSelection will ask the adapter for all views and not really make use of the buffer.