Add subitem doesn't account for other expanded subitems

[gwailoTr0n5000]

In our latest round of testing we've found another small bug with regard to adding subitems. Additions work well except in the case that the incoming item is being placed after another subitem that itself is expanded. In this case, the position does not account for the additional flat list positions occupied by the children and the incoming item is placed either between the sub-subitems or in another incorrect position of the list.

I've tracked the relevant code down to the following:

Jump to code in GitHub

private boolean addSubItems(@IntRange(from = 0L) int parentPosition, @IntRange(from = 0L) int subPosition, @NonNull IExpandable parent, @NonNull List<T> subItems, boolean expandParent, @Nullable Object payload) {
        boolean added = false;
        if(expandParent && !parent.isExpanded()) {
            this.expand(parentPosition);
        }

        if(parent.isExpanded()) {
            added = this.addItems(**parentPosition + 1 + Math.max(0, subPosition)**, subItems);
        }

        if(payload != null) {
            this.notifyItemChanged(parentPosition, payload);
        }

        return added;
}

A similar issue occurs when an expandable item H is expanded followed by the expansion of a subitem and then the collapse of the top expandable item H. The collapse only removes k items where k = the number of direct subitems for the expandable item H. A method to quickly return the total number of visible subitems (recursively counting expansions when present) seems most useful to help handle these cases. Does such a method exist? Perhaps I missed it? If not, I'm happy to try my hand at a version but the modifications will be more significant than my last effort and I appreciate your input with regards to other areas where issues may arise due to this bug.

Thanks!

P.S. I can submit the expansion count bug as a separate issue or append it here if you prefer since they are similar.

[davideas]

@gwailoTr0n5000, could you please explain better what is the cause of the problem of not adding subitems at the sub-position?

From the comment in the code: that method expands parent if requested and not already expanded; Notify the adapter of the new addition to display it and animate it. If parent is collapsed there's no need to add sub items; Notify the parent about the change if requested.
So we insert at parentPosition + 1 always, + the relative position of the sub list the expandable (parent).
I do not see any bug.

You may want to do this (with else if)?

boolean added = false;
// Expand parent if requested and not already expanded
if (expandParent && !parent.isExpanded()) {
    expand(parentPosition);

// Notify the adapter of the new addition to display it and animate it.
// If parent is collapsed there's no need to add sub items.
} else if (parent.isExpanded()) {
    added = addItems(parentPosition + 1 + Math.max(0, subPosition), subItems);
    // Notify the parent about the change if requested
    if (payload != null) notifyItemChanged(parentPosition, payload);
}

return added;

Basically, the method is an helper of what you had to do if expandable is expanded or not.

Regarding collapsing, a recursive collapse is already done.

[gwailoTr0n5000]

I'm sorry for not doing a better job earlier. I will do an example below.

Header

• s 1
• s 2
• ss 1
• ss 2
• s 3

if adding at position 5 (where "s 3" is currently), the insert I see is the following:

Desired:

Header

• s 1
• s 2
• ss 1
• ss 2
  **- * new 1 ***
• s 3

Actual:

Header

• s 1
• s 2
  **- * new 1 ***
• ss 1
• ss 2
• s 3

The flat position is 3 if the sub-subitems (ss 1 and ss2) are not considered, however the true position actually requires adding position + recursive(num of sub items) before the insert position. Perhaps I have called the incorrect method?

[davideas]

Ok, I see the issue now. It should count expanded subItems recursively.
I can do this in the next days, I don't have time now, but you can try to create a new private function and create a pull request.

When I created that method I didn't update for multilevel, indeed it doesn't support it and should be fixed.

[gwailoTr0n5000]

I'll take a shot at it. It seems the places to add this at the moment are within the following two methods:

private boolean addSubItems(@IntRange(from = 0L) int parentPosition, @IntRange(from = 0L) int subPosition, @NonNull IExpandable parent, @NonNull List<T> subItems, boolean expandParent, @Nullable Object payload) {
}

and

private int expand(int position, boolean expandAll, boolean init, boolean notifyParent) {
}

If anything else comes to mind let me know and I'll include it in the pull request.

Thanks!

-Pedro

[davideas]

Ok for me

[davideas]

@gwailoTr0n5000, are you going to make the pull request?

[gwailoTr0n5000]

Initial changes led to another issue which I'm debugging this morning. Will update you as soon as possible.

…

On Tue, Jun 27, 2017 at 7:19 AM Davide Steduto ***@***.***> wrote: @gwailoTr0n5000 <https://github.com/gwailotr0n5000>, are you going to make the pull request? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#394 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABwHW9fgFBYWYJNfbyTeB1v-hoANSbLMks5sIPMdgaJpZM4OC2E4> .

[gwailoTr0n5000]

I've been able to get it most of the way but am ending up with inconsistencies in some cases causing crashes, the mPosiion is greater than the adapter count:

        boolean validateViewHolderForOffsetPosition(ViewHolder holder) {
            // if it is a removed holder, nothing to verify since we cannot ask adapter anymore
            // if it is not removed, verify the type and id.
            if (holder.isRemoved()) {
                if (DEBUG && !mState.isPreLayout()) {
                    throw new IllegalStateException("should not receive a removed view unless it"
                            + " is pre layout");
                }
                return mState.isPreLayout();
            }
            if (holder.mPosition < 0 || holder.mPosition >= mAdapter.getItemCount()) {
                throw new IndexOutOfBoundsException("Inconsistency detected. Invalid view holder "
                        + "adapter position" + holder);
            }
            if (!mState.isPreLayout()) {
                // don't check type if it is pre-layout.
                final int type = mAdapter.getItemViewType(holder.mPosition);
                if (type != holder.getItemViewType()) {
                    return false;
                }
            }
            if (mAdapter.hasStableIds()) {
                return holder.getItemId() == mAdapter.getItemId(holder.mPosition);
            }
            return true;
        }

My branch is here if you'd like to take a look:
https://github.com/gwailoTr0n5000/FlexibleAdapter

[gwailoTr0n5000]

Figured it out, was using a recursive call in prepareItemsForUpdate that is unnecessary as that method already traverses through subitems as they expand. Pull request being sent now.

[davideas]

Thanks to this fix/code improvement we can now add a new feature see #401.

[gwailoTr0n5000]

Excellent! That was something we were considering for the future.