PlaceHolderView icon indicating copy to clipboard operation
PlaceHolderView copied to clipboard

Nested PlaceHolderView: rapid scroll up and down cause crash

Open jjhesk opened this issue 7 years ago • 16 comments

There is an dimension issue when scrolling up and down on the placeholderview.

com.mindorks.placeholderview.PlaceHolderView#onMeasure() did not set the measured dimension by calling setMeasuredDimension()

It seems to be too heavy when calling refresh on the parent placeholderview when the child has an update on the dimensions and items.

I think the problem would be better to ask how to make the parent placeviewholder to refresh at the marked child placviewholder.

jjhesk avatar Oct 11 '16 09:10 jjhesk

@jjhesk You have two methods for that in 0.2.7

  1. <T>void refreshView(T resolver): This will refresh the child placeholderview. Pass the instance of the class containing child placeholderview. Call mainPlaceholderview.refreshView(obj)
  2. public void refreshView(int position): This will refresh the child placeholderview after new data is added based on the position of child placeholderview in the mainPlaceholderview.
    1. <T>int getViewResolverPosition(T resolver): This will give the position of the class object which is added as the child placeholderview in the mainPlaceholderview, even before the item view is binded to the window.

janishar avatar Oct 11 '16 09:10 janishar

I think this is a possible leak from recyclerview. Still not sure why it will crash after several scroll up and down.

jjhesk avatar Oct 11 '16 11:10 jjhesk

@jjhesk Please use this only

mProfileList.getBuilder()
                .setHasFixedSize(false)
                .setItemViewCacheSize(5)
                .setLayoutManager(new GridLayoutManager(mContext, 4, LinearLayoutManager.VERTICAL, false));

In stead of

mProfileList.getBuilder()
                .setHasFixedSize(false)
                .setItemViewCacheSize(5)
                .setLayoutManager(gridLayoutManager);

I checked that, providing the global gridLayoutManager onResolved was the reason from com.mindorks.placeholderview.PlaceHolderView#onMeasure() error.

janishar avatar Oct 11 '16 13:10 janishar

@janishar As you have suggested the optimization in #11 will have to revert because having any kind of layout manager initialized at the constructor will lead to placeholderview on measure error crash.

jjhesk avatar Oct 12 '16 02:10 jjhesk

@jjhesk I have created a demo project: This project has 4 child placeholderview in the main placeholderview the repository for the project: NestedPlaceHolderView

This project fetch google news and show in the the child placeholderview's

janishar avatar Oct 12 '16 12:10 janishar

@janishar thank you for the example supports. I have noticed that you have commented out the animation on the class.@Animate(Animation.CARD_LEFT_IN_DESC) are there any significance or caveat?

jjhesk avatar Oct 12 '16 15:10 jjhesk

@janishar looks like this code shown as below is a kind of self automation.

    private final int DO_NOTHING = 0;
    private final int ADD_VIEW = 1;
    private final int UPDATE_VIEW = 2;

    private HashMap<GridItemView, Integer> listItemGoogleNewsMap;

...

 for(GridItemView gridItemView : listItemGoogleNewsMap.keySet()){
            switch (listItemGoogleNewsMap.get(gridItemView)){
                case DO_NOTHING:
                    break;
                case ADD_VIEW:
                    mainListView.addView(gridItemView);
                    listItemGoogleNewsMap.put(gridItemView, DO_NOTHING);
                    break;
                case UPDATE_VIEW:
                    mainListView.refreshView(gridItemView);
                    listItemGoogleNewsMap.put(gridItemView, DO_NOTHING);
                    break;
            }
        }
...


                    new Handler(Looper.getMainLooper()).post(new Runnable() {
                        @Override
                        public void run() {
                            for(GoogleNews.entry entry : googleNews.getResponse().getFeedData().getEntryList()){
                                listItemGoogleNewsMap.put(new GridItemView(entry), ADD_VIEW);
                            }
                            newsListView.refreshView(RowListView.this);
                        }
                    });

Is that a good idea to wrap them into annotation so the implementation of late update event has less lines?

jjhesk avatar Oct 12 '16 15:10 jjhesk

@jjhesk No there is no particular reason to comment those lines. It was just that the news example required no animation as such. Do try the example by commenting them out too. Thanks

janishar avatar Oct 12 '16 15:10 janishar

@jjhesk I think the framework should be generic and the control should be in the hands of the programmer. Above example is just one of the solutions for not repeating addition of views when the parent view is re bound to the window.

The recyclerview uses the same view type to repopulate the views, so it requires the view to re make whenever it come in the bound state. Hence this method will be called every time the binding takes place and any addView written will add that view every time it is called.

How will such an annotation be designed I am not able to visualise. Annotations are metadatas and will not be changed dynamically. So how that will work if you want to add or refresh with updated data of the view depending on conditions and based on annotation?

If you have any suggestion do share.

Thanks

janishar avatar Oct 12 '16 16:10 janishar

just a quick question. testing against of your nested placeholder with late update events. There is a weird thing happened. when the rapid scroll was taken in action and it ended up mixed two different placeholder using the same set of children.

First thing I have noticed that when DO_NOTHING was found on the memory hashmap I quickly noticed that the original set of children were gone. Then I added the children back to the placeholder as illustrated: itemProfileCircle is the first type of child and there is another type of child called itemB. for itemProfileCircle there are 6 small items display in a linear horizontal layout. for itemB there are 12 items display in grid layout.


   private HashMap<itemProfileCircle, Integer> itemProfileCircleList;

   @View(R.id.placeholderview)
    private PlaceHolderView mProfileList;

... 


    @Resolve
    private void onResolved() {

  if (mProfileList.getAllViewResolvers().size() == 0) {
           // here to initialize my layout manager
        }


   for (itemProfileCircle item : itemProfileCircleList.keySet()) {
    switch (itemProfileCircleList.get(item)) {
              case DO_NOTHING:

              // here i found out that eventually over the time all children will be removed on the placeholder and i added the patch in here..


                    int viewsize = mProfileList.getAllViewResolvers().size();
                    if (viewsize < itemProfileCircleList.keySet().size()) {
                        mProfileList.addView(item);
                    }


                    break;
                case ADD_VIEW:
                    mProfileList.addView(item);
                    itemProfileCircleList.put(item, DO_NOTHING);
                    break;
                case UPDATE_VIEW:
                    mProfileList.refreshView(item);
                    itemProfileCircleList.put(item, DO_NOTHING);
                    break;

       }

 }

There I have two different nested placeholders and they are written with the same pattern.

It messed up that the children from first nested placeholder were also displayed at the second nested placeholder that mixed into itemB children. Some itemB were display in the second placeholder. Their order are mixed up and not the same as before. I am not sure how it would behave like this. Maybe @janishar can understand this design pattern. Alternatively if i remove the additional lines from DO_NOTHING then it will really doing nothing and the placeholder will display nothing after scrolling it a while I believed that they might have been recycled. I will post the leakage log when I can reproduce it again. I also not sure if it is related to #17.

jjhesk avatar Oct 16 '16 16:10 jjhesk

@jjhesk If you are using same xml layout for the sub placeholderviews then the main Placeholderview will try to reuse the same layout when rebinded. This feature is from recyclerview, that uses the same view of particular type to be used to repopulate the views.

Try using

mProfileList.refresh(); in onResolved()

below code is not required. But I will check to be sure

int viewsize = mProfileList.getAllViewResolvers().size(); if (viewsize < itemProfileCircleList.keySet().size()) { mProfileList.addView(item); }

If adding mProfileList.refresh() doesn't solve the issue then try giving different names to the layout xml file used in subplaceholderview class in @Layout

janishar avatar Oct 16 '16 17:10 janishar

@janishar I have followed your suggestion and did the patch and now the mixup issue is gone. I now confirm you that using the same @Layout(R.layout.item_placeholder) to inflate the holder will cause the mix up issue. Maybe you can issue a fix for the next version. Now i try to looking into the mix up ordering issue from rerecycle, rebind. Now I suspecting on the Hashmap itself. I know that Hashmap is not stored in order. would that be the reason?

jjhesk avatar Oct 17 '16 02:10 jjhesk

Now I confirm that Hashmap is the reason to cause random children order to from the item list in the sub placeholder. I changed to LinkHashMap and all the problem solved. Now Im thinking the performance. Is LinkHashMap slower than Hashmap. Just a quick review from the concept and they are similar in all aspects.

jjhesk avatar Oct 17 '16 03:10 jjhesk

@jjhesk Let me work a bit more on Nested Placeholderviews so the apis are more simpler. I am thankful that You have provided a lot of insights till now.

I don't think the performance will be much of difference if you use LinkHashMap.

Another way of replacing HashMap is to add a variable in the class representing the item views and then maintain the state through those variables.

eg. https://github.com/janishar/Tutorials/tree/master/NestedPlaceHolderView

In class GridItemView we add

private int state;
public int getState() {
     return state;
}

public void setState(int state) {
     this.state = state;
 }

RowListView class in modified to remove HashMap and instead use List

@Layout(R.layout.news_row_list)
public class RowListView {

    @View(R.id.mainListView)
    private PlaceHolderView mainListView;

    private final int DO_NOTHING = 0;
    private final int ADD_VIEW = 1;
    private final int UPDATE_VIEW = 2;
    private PlaceHolderView newsListView;
    private List<GridItemView> gridItemViewList;

    public RowListView(PlaceHolderView newsListView) {
        this.newsListView = newsListView;
        gridItemViewList = new ArrayList<>();
        new NetworkCall();
    }

    @Resolve
    public void onResolved(){
        if(mainListView.getViewResolverCount() == 0) {
            mainListView.getBuilder()
                    .setHasFixedSize(false)
                    .setLayoutManager(new StaggeredGridLayoutManager(2, LinearLayoutManager.VERTICAL));
        }
        for(GridItemView gridItemView : gridItemViewList){
            switch (gridItemView.getState()){
                case DO_NOTHING:
                    if (mainListView.getViewResolverCount() < gridItemViewList.size()) {
                        newsListView.addView(gridItemView);
                    }
                    break;
                case ADD_VIEW:
                    mainListView.addView(gridItemView);
                    gridItemView.setState(DO_NOTHING);
                    break;
                case UPDATE_VIEW:
                    mainListView.refreshView(gridItemView);
                    gridItemView.setState(DO_NOTHING);
                    break;
            }
        }
    }

    private class NetworkCall implements Runnable{

        public NetworkCall() {
            new Thread(this).start();
        }

        @Override
        public void run() {
            try {
                URL obj = new URL(GoogleNews.URL);
                HttpURLConnection con = (HttpURLConnection) obj.openConnection();
                con.setRequestMethod("GET");

                int responseCode = con.getResponseCode();
                Log.d("Debug", "Response Code : " + responseCode);

                if (responseCode == HttpURLConnection.HTTP_OK) {
                    BufferedReader in = new BufferedReader(
                            new InputStreamReader(con.getInputStream()));
                    String inputLine;
                    StringBuffer response = new StringBuffer();

                    while ((inputLine = in.readLine()) != null) {
                        response.append(inputLine);
                    }
                    in.close();

                    Log.d("Debug", "Response : " + response);
                    GsonBuilder builder = new GsonBuilder();
                    Gson gson = builder.create();

                    final GoogleNews googleNews = gson.fromJson(response.toString(), GoogleNews.class);

                    new Handler(Looper.getMainLooper()).post(new Runnable() {
                        @Override
                        public void run() {
                            for(GoogleNews.entry entry : googleNews.getResponse().getFeedData().getEntryList()){
                                gridItemViewList.add(new GridItemView(entry, ADD_VIEW));
                            }
                            newsListView.refreshView(RowListView.this);
                        }
                    });

                }
            }catch (MalformedURLException e){
                e.printStackTrace();
            }catch (IOException e){
                e.printStackTrace();
            }
        }
    }
}

janishar avatar Oct 17 '16 12:10 janishar

private int state;
public int getState() {
     return state;
}

public void setState(int state) {
     this.state = state;
 }

Could this part able to be supported by annotation so such repetitive code can be eliminated from using extend or copy and paste into every child class which is in the nestplaceholderview. It is just the idea, but i am sure as more developers writing the code would like to achieve this.

jjhesk avatar Oct 18 '16 06:10 jjhesk

@jjhesk The suggestion is good. I am thinking about it. Will add in the new release. Thanks

janishar avatar Oct 18 '16 16:10 janishar