Multiselect + Subitems + Deletion problem

[MFlisar]

Based on your multi select and expandable list example, I'm trying to combine those two - I think I did not change anything relevant.

Showing the data with headers works fine, expanding/collapsing works fine. BUT as soon as I start deleting something, it get's messed up.

Observations

• deleting items and not collapsing anything works visually fine

Problem

• I select the first sub item and delete it, then I collapse the first header and see, the second one is missing
• I select the first two sub items and delete them, then I collapse the first header and see, the second and third header is missing

Afterwards, more problems occur (expanding a header that still exists will show selected items for example, ...)

How I set up my adapter

    mFastAdapter = new FastItemAdapter<>();
    mActionModeHelper = new ActionModeHelper(mFastAdapter, R.menu.menu_folders, new ActionBarCallBack());

    // 2) FastAdapter Setup
    mFastAdapter.withSelectable(true);
    mFastAdapter.withMultiSelect(true);
    mFastAdapter.withSelectOnLongClick(true);
    mFastAdapter.withOnPreClickListener(new FastAdapter.OnClickListener<IItem>() {
        @Override
        public boolean onClick(View v, IAdapter<IItem> adapter, IItem item, int position) {
            //we handle the default onClick behavior for the actionMode. This will return null if it didn't do anything and you can handle a normal onClick
            Boolean res = mActionModeHelper.onClick(item);
            return res != null ? res : false;
        }
    });
    mFastAdapter.withOnPreLongClickListener(new FastAdapter.OnLongClickListener<IItem>() {
        @Override
        public boolean onLongClick(View v, IAdapter<IItem> adapter, IItem item, int position) {
            ActionMode actionMode = mActionModeHelper.onLongClick(mActivity, position);

            if (actionMode != null) {
                //we want color our CAB
                mActivity.findViewById(R.id.action_mode_bar).setBackgroundColor(UIUtils.getThemeColorFromAttrOrRes(mActivity, R.attr.colorPrimary, R.color.material_drawer_primary));
            }

            //if we have no actionMode we do not consume the event
            return actionMode != null;
        }
    });

How I fill my data

    // List of sub items without headers
    List<FolderItem> folders = ...;

    // Test - group items in groups of 10
    List<IItem> items = new ArrayList<>();
    int count = folders.size();
    int headers = (int)Math.ceil((float)folders.size() / 10f);
    for (int i = 0; i < headers; i++)
    {
        FolderItemHeader header = new FolderItemHeader()
                .withHeader("Header " + (i + 1))
                .withName("Header " + (i + 1))
                .withDescription("Description...")
                .withSubItems(folders.subList(i * 10, Math.min(count, i * 10 + 10)))
                // ALL folder items have an id that represents the rowid in the database, so
                // the ids are > 0 AND unique for sure!
                // using ids < 0 will result in no confilcting ids for sure
                .withIdentifier(-1 - i)
                .withIsExpanded(true);
        items.add(header);
    }

    // set data
    FastItemAdapter<IItem> fastAdapter = (FastItemAdapter<IItem>)mBinding.rvData.getAdapter();
    fastAdapter.setNewList(items);

[mikepenz]

@MFlisar sorry for not yet answering. I am very busy at the moment.

Having collapsables + deleting is a really tricky topic. :/. Will I be able to just copy paste your above code and debug it?

EDIT ok just have seen that it comes with long click etc. can you provide a small sample application to reproduce this issue? Thank you.

[MFlisar]

I'll make an example for you tomorrow. Thanks for looking into it

[MFlisar]

Here's the example:

https://github.com/MFlisar/FastAdapter/blob/develop/app/src/main/java/com/mikepenz/fastadapter/app/ExpandableMultiselectSampleActivity.java

You can use my repo directly, it can be compiled and I added the new activity to the main activity and you can start it via the drawer

Bug

Expand the "Test 1" row, then select 1 to n sub items of "Test 1". Click delete. Until now, everything is fine. Now collapse the "Test 1" row again and you will see that "Test 2" until "Test [2+n]" are gone after collapsing. If you then expand the "Test 1" row again, you will see that "-- Test 1" until "-- Test [n]" are there again and selected...

[mikepenz]

@MFlisar thanks for the demo applicaiton.

After testing it. There is a simple answer to this. Deleting subitems via the normal methods is not supported.

Long answer: To be extrem fast and extrem performant, the FastAdapter has to keep track on which items are selected / not selected. It will also treat sub items not as normal items. They are temporary and will not exist in the normal item level. So if you delete with the deleteAllSelectedItems it goes over the list via the positions removes the items. as subItems are temporary shown there it will delete the elements at these positions. the ui will hide them as the recyclerview just animates them away. but in the background the original items (the one not on a sublevel) are deleted.

I am also not yet really surewhat the most performant solution for this scenario would be. :/ Especially as removing sub items should result in removing the items from the list inside its parent. not inside the main list.

The whole thing add some complexity. That's also why you can't currently notify about sub item changes for sub-sub hirachies.

[MFlisar]

I would write the algorithm to search for the correct item and remove it from it's parent. One question though - is it enough to remove the item from the parent items sub list and then call notifyAdapterItemRemoved on the adapter? Or is there some other special handling of those temporary items necessary? Looks like it is that easy from your description.

Performance wise I would do following:

• sort the selected indizes descending
• for each selected item search for the parent - if you find it remove ALL selected children (easy, because selectionindizes are sorted). Parent will be searched based on the highest selected index, looking upwards until the parent is found

I could than make a pull request for it as well, still, where would you like to have this? Maybe in a SubItemUtility class or so?

[mikepenz]

@MFlisar if sub items change there is the notifyAdapterSubItemsChanged method which needs to be called. It will then take care if items were deleted or added.

But it's true there should be some SubItemUtils class which adds some helpers for sbitems

[MFlisar]

So the utility class needs to take care of following:

• notifyAdapterSubItemsChanged only works for first level children, correctly?
• what must happen for sub sub items? notifyAdapterItemRemoved with the total index is fine, isn't it? I won't use sub sub items for now, but I would like to repect them as well

[mikepenz]

notifyAdapterSubItemsChanged correct. The hope is anyway that this will do also sub, sub, sub ... hirachies.

Sub sub sub hirachies would not really work via all the notify methods. that's the point. sub items are outside of the normal items range, as they should not alter the general list.

(as an alternative I would have been able to do a simple implementation of the "subItems" where the parent would just add those items, and remove them again, and they would be in the normal list, etc... but I think this would have some other ba^d side effects)

[MFlisar]

So far, it looks quite good already.

I will allow to notify the parent in the utility class automatically, so that it can update the count (if it is displaying the count of sub items). I experienced, that calling notifyAdapterItemChanged (I call this on the parent only) will collapse it. Is that really necessary?

I solve that with following:

expanded = ((IExpandable)parent).isExpanded();
adapter.notifyAdapterItemChanged(parentIndex);
// expand the item again if it was expanded before calling notifyAdapterItemChanged
if (expanded)
    adapter.expand(parentIndex);

Though shouldn't this be the default behaviour?

[MFlisar]

Another problem with multi selection is that if you collapse an item and expand it again, the selection is lost...

I am handling the expand/collapse action manually anyways, so I just disable collapsing when you are in the selection mode for now. I am not sure, where this thing should be handled if I don't disallow collapsing.

I had following idea, the utility class could help remembering selected items of collapsed items, but this looks not really beautiful to me. But because you are handling sub items as temporary items, maybe this is the best solution? This will only work if you collapse/expand items manually of course, as you have to handle these events in the utility to add/remove selected sub items ids correctly to a helper class...

Any other ideas?

[mikepenz]

@MFlisar the selection is only lost in the position based state management, but not if you disable this one and use the identifier based statemanagement

[mikepenz]

https://github.com/mikepenz/FastAdapter/blob/develop/library/src/main/java/com/mikepenz/fastadapter/FastAdapter.java#L228

[mikepenz]

The reason for this is, that the position based statemanagement can't keep the selection for an item which is no longer there. The alternative statemanagement can do this as it will keep the identifiers of the items. which adds a bit more flexibility

[MFlisar]

I have a working example with expandable headers + multi selection. As soon as I activate the id based state management and start my app, instead of showing all items expanded they are shown collapsed. Expanding the first item and collapsing it again removes all other headers...

I will add an example to demonstrate this feature (multi selection + expandable items + deletion of items). It's working fine so far. And upload it to my fork.

Maybe you can check what's the problem with the id based state management then? Or do you have an idea straight away?

PS: additionally I've added a RangeSelectorHelper. It will take care of selecting a range of items via long pressing the first and the last item (QuickPic has this for example). It's a nice feature an can be easily activated with the helper...

[MFlisar]

An example is online, it shows multi selection + expandable items + deletions + range selection via long pressing first/last item of range. All with position based statement manager. It's working so far.

You can test it, it's here: https://github.com/MFlisar/FastAdapter/blob/develop/app/src/main/java/com/mikepenz/fastadapter/app/ExpandableMultiselectDeleteSampleActivity.java

As soon as you enable the id based state management, it breaks. Just uncomment line 60 to test it.

[mikepenz]

@MFlisar currently at a conference. will have to check later.

[MFlisar]

There is another problem I'm currently investigating. So wait until I have described this problem here.

[MFlisar]

I disabled notifyParent for now (as the bug has something to to with expanding items and this function will expand headers, so I wanted to make the reproduction example as simple as possible) and added some debugging logs, they show which items my algorithm deleted. And you see, that the algorihm is working fine. Still there is a bug.

Bug description

In some cases it happens, that expanding a header shows wrong (double) items.

Bug reproduction - finally I found a simple reproduction

Start the demo and do following:

• select all 5 sub items of "Test1"
• delete them
• collapse item "Test 3" => this results in expanding the item and suddenly it has all it's sub items twice
• collapsing "Test 3" again only removes half of the items...

Ignore the deleteSelectedTEST function... I tried a few other methods to solve the problem but none really worked, I think it's something in the adapter... (especially handling sub items notifications seperately and similar)

Info

• my function calls notifyAdapterSubItemsChanged after EVERY update of a parents sub items (sub items are deleted directly from the parents sub items list before calling this)
• items without a parent are deleted via the adapter.remove(pos) function

I think that's correct so far...

[mikepenz]

@MFlisar sorry that it took so long for me getting time to have a look.

There is quite an "easy" description on why things start getting weird when you delete items and just call notifyAdapterSubItemsChanged, and I think it is a problem of the current core.

the FastAdapter keeps track of the count of expanded items inside themExpanded SparseIntArray so I will always know on how many items are expanded, and at which position. This allows to quickly get counts etc, which eliminates unnecessary loops. This only is used with the PositionBasedStateManagement. An additional problem is that this is also used to check when expanding collapsing. As the list will contain the positions of the expanded items, but the indexes were moved it will fail also.

Inside the notify method there is no update for this, which is resulting in some problems.

[mikepenz]

@MFlisar so if you remove the withIsSelected(true) from your sample, use the (newly added) .expand() method (which will expand all on the root level). and then delete it will work fine.

Perhaps we should just limit this use case to the id based state management as it will cause quite some overhead for the normal state management

[mikepenz]

@MFlisar ok after some more digging, I was able to detect 2 problems which were causing this issue when deleting. After applying the fixes it works now fine, inside your sample application

[MFlisar]

I shortly tried it in my app, seems to work, thanks. I'll test it a little more though...

One thing though, being forced to use adapter.expand() is somehow weird as long as the withIsExpanded(true) is possible. It should be possible to implement the same logic in the setList, setNewList, add functions of the adapter, shouldn't it? I think both cases should behave the same...

[MFlisar]

ID based state management

You said, the adapter should remember selections if we use the id based state management, it does not work in the example...

I've already submitted my changes to my fork...

[mikepenz]

@MFlisar are you sure? https://github.com/mikepenz/FastAdapter/blob/develop/library/src/main/java/com/mikepenz/fastadapter/FastAdapter.java#L1279 deselect is not called in this case

[MFlisar]

I am. The example shows it. I'll check it though and check why this happens. Could it be that the items themself save the selection? The example shows, that when I select items and deselect them again, that if I debug it and check the state at this position https://github.com/mikepenz/FastAdapter/blob/develop/library/src/main/java/com/mikepenz/fastadapter/FastAdapter.java#L407, the mSelection is always empty...

Problem that I found while testing

In my own project I have another problem, my items use their database id as identifier, so I thought I make headers, that start at -1 and decrease the count... I now found out, this breaks your internal logic, because -1 is consider as no id... Any reason for this? Identiefier could be a Long instead of long and no id could be null...

My solution is to start with -2 now instead, still, not very intuitive I think. but working. Is that ok like that? Or may there be places where you check for i < 0 instead of i == -1?

[MFlisar]

PS: I think I'm right, you save the selection in the items if the id based state management is used:

https://github.com/mikepenz/FastAdapter/blob/develop/library/src/main/java/com/mikepenz/fastadapter/FastAdapter.java#L818

[mikepenz]

Ok. I will have to debug this. The link to the code you sent may be the problem.

we use long as it is better to use primitive types. and you can still use any really high ID near MAX LONG or so.

To honest I am not sure if I never use i < 0

[MFlisar]

Ok thanks.

Only idea I have so far about sub items and selection is, that the parent could remember which of it's children are selected as an alternative (would even work for both state managements)...

Considering the id, I'm not sure how save MAX_LONG - headerCount would be, the database may create such ids one day as well (although it's VERY unlikely...)

[mikepenz]

It will get complicated again when restoring selections with the savedInstanceState when the parent remembers.

Issue with the selections is that we currently do not iterate over the childs. So they stay selected but you do not get them via the method. getSelections()

if you get near MAX_LONG - headerCount you will have a real problem anyway. Also some additional information. RecyclerViews in Android can have at a maximum MAX_INT entries

[MFlisar]

I'm not talking about having max long items, just the unique id may get there because of constant deletions and additions which will create a new unique id... Not sure what the database will do if it reaches max long, it should not start at 1 again, as relations may still point to this old id... But that's not a problem with this library..

I think it actually should be save to create the ids for the headers as you suggested

[mikepenz]

so the last "issue" is now getting the selections inside getSelections I may just need to add a recursively algorithm there

[MFlisar]

I think there is something else to consider: you can't handle the indizes for the collapsed items in index based selection functions like the one mentionied by me before:

https://github.com/mikepenz/FastAdapter/blob/develop/library/src/main/java/com/mikepenz/fastadapter/FastAdapter.java#L818

Getting selected items works with a recursive function though...

so Set<Integer> getSelections() returns only existing indizes, while Set<Item> getSelectedItems() returns collapsed selected items as well? Maybe adding a function like Set<Item> getSelectedItems(boolean includingCollapsed) ? Still this gets very confusing...

[MFlisar]

Problems I see

• you can't offer a Set<Integer> getSelections() function that returns valid indizes AND contains all selected sub items (as sub items are temporary and collapsed sub items are not in the adapter)
• functions are deciding what to do based on Set<Integer> getSelections(), like the ActionModeHelper. This means, if you have selected sub items you not want to close the ActionMode for example (but the set of indizes can't contain collapsed selected items)
• if you delete a selected item, you have to check if it is in the adapter (it may be a collapsed item, so you always have to check the items position against -1 before working with the item on the adapter)

Just a few thoughts... Maybe there are more things to think of?

[MFlisar]

The recursive function is here if you want to test something, I've already tried it:

public Set<Item> getSelectedItems(boolean includingCollapsed) {
    if (!includingCollapsed)
        return getSelectedItems();

    Set<Item> selections = new HashSet<>();
    int length = getItemCount();
    List<Item> items = new ArrayList<>();
    for (int i = 0; i < length; i++)
        items.add(getItem(i));
    updateSelectedItemsWithCollapsed(selections, items);
    return selections;
}

private void updateSelectedItemsWithCollapsed(Set<Item> selected, List<Item> items) {
    int length = items.size();
    for (int i = 0; i < length; i++) {
        if (items.get(i).isSelected()) {
            selected.add(items.get(i));
        }
        if (items.get(i) instanceof IExpandable && ((IExpandable)items.get(i)).getSubItems() != null)
            updateSelectedItemsWithCollapsed(selected, ((IExpandable)items.get(i)).getSubItems());
    }
}

[MFlisar]

Further problems

• My SubItemUtil tries to get an items parent with the help of the adapter. This is already slow... But if I don't know, that the parent is in the adapter somewhere above the item, I must search for the parent from upside down, which means I have to search through every top item, check it's children and check the children recursively until I find my item, otherwise I can't find an items parent... Can be optimised, but still it's a little problem...

Alternatively, disabling collapsing items with selected sub items would be a "solution" as well... Although not the perfect one. But all dependent functions will work just as they are...

[mikepenz]

@MFlisar that's true. We may need to move the recursive function to the SubItemsUtil which then allows you to get the selections + sub items.

What may also could help is creating a connection between childs and parents. So we will always know which items are sub items, and which items are the main parent. (this will also allow to get ot the parent, because we know that it will be my total position - my position in the sublist)

So having

IItem (id = 1, level 0)
    ISubItem implements IItem (id = 2, parent=1, level 1)
         ISubItem implements IItem (id = 3, parent = 2, level 2)
         ISubItem implements IItem (id = 4, parent = 2, level 2)
         ISubItem implements IItem (id = 5, parent = 2, level 2)
    ISubItem implements IItem (id = 6, parent = 1, level 1)
    ISubItem implements IItem (id = 7, parent = 1, level 1)

getting the positions for the selections will always be a problem, thus it will always just return visible items.

deleting items only allows to delete normal items anyway so child items should be ignored here

[MFlisar]

I wanted to suggest the two way linking as well, there is no other way to make the slow search functions in the SubItemsUtil faster. This will make most of them as fast as O(1)...

So I would suggest following:

• SubItemsUtil gets a getSelectedItems(adapter) function, that returns a Set of selected items, including all sub items, independent of recursion depth (there won't be a function to get indizes, the one from the adapter will stay the only one and will return visible indizes only)
• a new ISubItem interface is added to the core
• SubItemsUtil will only work with items that implement ISubItem and/or IExpandable
• I suggest, SubItemsUtil ignores items that do not implement ISubItem and/or IExpandable (if you have to care about such items, the functions get slow again, as you have to search for this items manually again)

I would suggest following interface:

public interface ISubItem<T, Item extends IItem, S extends IExpandable<T, Item>>
{
    S getParent();

    // I would NOT add this, level can be easily calculated via traversing the parents and counting them
    int getLevel();
}

If you don't have any objections, I would make it like that and make the necessary changes...

[mikepenz]

@MFlisar the above metioned points sound fine.

One more addition. The IExpandable interface has to be changed to be:

public interface IExpandable<T, Item extends IItem & ISubItem > {

The additional question is if it shouldn't be required that the ISubItem has extends IItem as it will always have to be an IItem too

[mikepenz]

And yes the level can be dropped, as it would only result in problems when not correctly recalculated.

It is ok if you make the necessary changes :)

Afterwards I will have a look and perphaps it is possible to simplify the FastAdapter core code too with the above changes

[MFlisar]

If we change the IExpandable interface, that's a breaking change though... It's fine for me though and has the positive side effect, that I can be sure that sub items are ISubItems which makes everything safer... But people who use IExpandable already must adopt their items to link them with their parent. But I think it's worth it too.

Then I'll start with the adoptions ;-)

I will adopt the example afterwards and update my fork and inform you here...

[mikepenz]

@MFlisar I know :) But we will release the next version as v2.0.0 which can have some breaking changes ;)

Oh wait one more second I will merge the PR from @Rainer-Lang first. So you don't run into merge conflicts afterwards

EDIT MERGED

[MFlisar]

I think if you allow to collapse headers with selected sub items, it makes sense (from a user point of view) to show the selection count of sub items on the parent. Therefore I think it makes sense that the core adapter offers a possibility to listen to selection changes via a interface like:

public interface ISelectionListener {
    /**
     * is called, whenever the provided item is selected or deselected
     *
     * @param item the item who's selection state changed
     * param selected the new selection state of the item
     */
    void onSelectionChanged(IItem item, boolean selected);
}

What do you think about that? This may be useful for other cases as well and is a very small change with no side effects.

Example usage

I have made a simple HeaderItem for the demo app that displays it's selected children count and would like to add something simple as following:

fastItemAdapter.withSelectionListener(new ISelectionListener() {
    @Override
    public void onSelectionChanged(IItem item, boolean selected) {
        if (item instanceof SampleItem) {
            IItem headerItem = ((SampleItem)item).getParent();
            boolean expanded = ((IExpandable)headerItem).isExpanded();
            int pos = fastItemAdapter.getAdapterPosition(headerItem);
            // notify header, so that it can update the count of it's selected items on the view
            fastItemAdapter.notifyAdapterItemChanged(pos);
            if (expanded)
                fastItemAdapter.expand(pos);
        }
    }
});

[mikepenz]

@MFlisar yes that's a really good idea, as selection change must not directly come from the user clicking on a view (which then can also be blocked)

Really good idea. Thanks.

[MFlisar]

Ok.

I tried following to update the parent of an expanded item:

boolean expanded = ((IExpandable)headerItem).isExpanded();
int pos = fastItemAdapter.getAdapterPosition(headerItem);
fastItemAdapter.notifyAdapterItemChanged(pos);
if (expanded)
    fastItemAdapter.expand(pos, true);

This leads to all sub items losing their selection state on the view. Any (spontanous) idea why? I thought notifying the the sub items via the true in the fastItemAdapter.expand(pos, true); should do the trick...

[MFlisar]

EDIT

Following works as it bypasses the FastAdapter's internal functions:

fastItemAdapter.withSelectionListener(new ISelectionListener() {
        @Override
        public void onSelectionChanged(IItem item, boolean selected) {
            if (item instanceof SampleItem) {
                IItem headerItem = ((SampleItem)item).getParent();
                int pos = fastItemAdapter.getAdapterPosition(headerItem);
                fastItemAdapter.notifyItemChanged(pos);
            }
        }
    });

For this use case, that's actually fine, as I really just want to update the view and am sure, that nothing else should be done...

[MFlisar]

You said, you will look over it and make some improvements in the core anyways, so the state is following:

• ISelectionListener is called in a handful of places, currently it may be called multiple times => select/deselect functions would need an additional parameter boolean notifyListener for this I would say.
• The SubItemUtils have one place, where they work with a FastItemAdapter and not with a plain FastAdapter. I have not worked with wrapped adapters yet, so I'm not sure how this should be changed: https://github.com/MFlisar/FastAdapter/blob/develop/library-extensions/src/main/java/com/mikepenz/fastadapter_extensions/utilities/SubItemUtils.java#L202. This should be adopted... And afterwards the unsafe cast here https://github.com/MFlisar/FastAdapter/blob/develop/library-extensions/src/main/java/com/mikepenz/fastadapter_extensions/ActionModeHelper.java#L190 should be removed

That's it so far, I would say, the example is online and working

[MFlisar]

One important thing I just stumbled over in my app is following:

Whenever you update (in the sense of replace them with a new item) an item in your adapter, you must update parent/children as well. I for example load items in an empty state, display them, then I finish loading them and then I replace the old items with the fully loaded ones.

This brings up the question: shouldn't items that implement IIdentifyable overwrite hashCode and equals so that those are handled as equal in all functions? I'm not sure if something in this direction makes sense...

Edit this does not solve the problem, so nevermind... You have to copy the selected state to the new items/sub items in such a case... Or only update the wrapped data in the item

[mikepenz]

for the first comment. I have no idea why this happens. expand shouldn't remove the state the second comment. It is ok if you do this when really just changing general information, for more this will cause issues as the internal methods are not called with it third comment. yes I have to look at it anyway to make sure things work as expected. preventing multiple calls, yes I have to check this. will take a look, but it shouldn't be the FastItemAdapter the fourth comment. I don't think this is necessary. at the moment I use the identifier to check if items are equal so this should be fine. no general equals methods used

could you open a pr with the chagnes. will make it easier to check what is changed and to test. with your branch I can't directly see what is different

[MFlisar]

I'll make a PR soon, have to still do a small change...

[mikepenz]

@MFlisar ok great.