Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecate OnActionListener in UndoHelper #438

Closed
davideas opened this issue Sep 3, 2017 · 8 comments
Closed

Deprecate OnActionListener in UndoHelper #438

davideas opened this issue Sep 3, 2017 · 8 comments
Milestone

Comments

@davideas
Copy link
Owner

davideas commented Sep 3, 2017

Despite the interface was introduced for the new UndoHelper, it is easy to perform same actions (preAction, postAction) by just using normal flow:

  • PreAction code can be simply moved before the undo instantiation.
  • As well as PostAction code can be performed after the undo instantiation.

Execution flow is preserved. So, no need of this callback interface.

@rmunk
Copy link

rmunk commented Nov 29, 2017

On item swipe I want to delete only some files belonging to item but not the item itself using UndoHelper so user can undo the action. Before OnActionListener was deprecated I used onPreAction() to prevent item removal from adapter as per Javadoc documentation:

boolean onPreAction()
Performs the custom action before item deletion.
Returns:
true if action has been consumed and should stop the deletion, false to continue with the deletion

This was the code:

@Override
public void onItemSwipe(int position, int direction) {
    //Create list for single position (only in onItemSwipe)
    List<Integer> positions = new ArrayList<>(1);
    positions.add(position);

    final String videoPath = ((MeasurementItem) adapter.getItem(position)).getModel().getFileName();
    if (videoPath != null && new File(videoPath).exists()) {
        filesToDelete.push(videoPath);
        new UndoHelper(adapter, this)
                .withAction(UndoHelper.ACTION_UPDATE, new UndoHelper.SimpleActionListener() {
                    @Override
                    public boolean onPreAction() {
                        return true;
                    }
                })
                .remove(positions, layoutMain, getString(R.string.measurements_video_deleted),
                        getString(R.string.undo), UndoHelper.UNDO_TIMEOUT);
    } else adapter.notifyItemChanged(position);
}

@Override
public void onDeleteConfirmed(int action) {
    while (!filesToDelete.isEmpty()) {
        final String videoPath = filesToDelete.pop();
        new File(videoPath).delete();
    }
    adapter.notifyDataSetChanged();
}

@Override
public void onUndoConfirmed(int action) {
    filesToDelete.clear();
    adapter.notifyDataSetChanged();
}

What would now be the correct way for implementing custom action which does not result with item deletion and removal from adapter?

@davideas
Copy link
Owner Author

davideas commented Dec 3, 2017

@rmunk, the event of swipe occurred so you check and do custom action even before creating the UndoHelper, if undo SnackBar is shown, swiped item is removed from the adapter, and when action button is pressed the undo callback is still called.
If you don't need to delete then don't create the undo. I don't see any difference from what you where doing, that's why I deprecated the action listener.

PS. For single element list better to use Collections.singletonList(position).
PS2. I will need to fix the undo for RC4, as it is now, multiple consecutive swipe don't work well.

@rmunk
Copy link

rmunk commented Dec 4, 2017

@davideas That's what I did in the end. I created a plain SnackBar with undo action for left swipe and for the right swipe which deletes the item I still use UndoHelper. I just thought it would be more convenient to use same UndoHelper callbacks for both swipes and distinguish them by action.

I figured that's why you have UndoHelper.ACTION_REMOVE and UndoHelper.ACTION_UPDATE but update also removes the item from the adapter so this doesn't work. Maybe the name ACTION_UPDATE is a bit misleading and you might consider changing it.

@davideas
Copy link
Owner Author

davideas commented Dec 4, 2017

Thank you @rmunk, the issue comes from the action swipe that once done, I cannot restore the view to the original state, trust me i tried everything, but the RecyclerView has an internal X value that I cannot change even if, I update the item or I animate the view back you will see a glitch and still the view at wrong coordinates, so the only thing is to remove it and to press the undo to insert it again.

This week I will work again on this component to move it to the UI package, to fix the multiple swipe and maybe to rename it or change the behavior.

@davideas
Copy link
Owner Author

davideas commented Dec 4, 2017

@rmunk, but if you were not removing the item, how did you manage the swipe back then?

@rmunk
Copy link

rmunk commented Dec 5, 2017

@davideas I didn't swipe it back, I just called adapter.notifyDataSetChanged() in callbacks and items would become "un-swiped" again.

I didn't use adapter.notifyItemChanged(position) because of possible consecutive swipes which I wanted to handle together with one undo SnackBar. I know it isn't most efficient way to do it but I don't handle a lot of data so I am fine with the possible overhead.

@davideas
Copy link
Owner Author

davideas commented Dec 6, 2017

@rmunk, thanks, I've tried the notifyChange after the swipe and it works very well if we know the positions. So, I've modified the behavior for Action.UPDATE (no change for Action.REMOVE), now it doesn't remove items anymore:

  • If timeout is over, user is responsible to proceed with a confirmation action (that can still be a deletion).
  • If undo Snackbar button is pressed the action is cancelled and user should perform a notifyChange on those positions.

List of changes (for now):

// Class UndoHelper is now available in package UI, with same signature.
// More on this in the migration wiki page when ready.
implementation 'eu.davidea:flexible-adapter-ui:<version>'
package eu.davidea.flexibleadapter.helpers.UndoHelper;

// Renamed callback interface
from OnUndoListener to OnActionListener

// Use of the constants from annotation interface "Action" instead of class constants
constant ACTION_REMOVE to Action.REMOVE
constant ACTION_UPDATE to Action.UPDATE

// New parameter for method onActionCancelled
// (when the action button in Snackbar is pressed)
// providing the positions affected by the action
void onActionCanceled(@Action int action);
// to
void onActionCanceled(@Action int action, List<Integer> positions);

I still need to fix the consecutive swipe.

@davideas davideas reopened this Dec 6, 2017
@davideas davideas modified the milestones: 5.0.0-rc3, 5.0.0-rc4 Dec 6, 2017
@rmunk
Copy link

rmunk commented Dec 6, 2017

@davideas it looks good now. Btw. thanks for this great library!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants