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

Ability to filter AdapterCommands #4

Closed
wants to merge 2 commits into from
Closed

Ability to filter AdapterCommands #4

wants to merge 2 commits into from

Conversation

nomisRev
Copy link

I am in a scenario where I want to filter delete commands. If I do so, I enter an inconsistent state since oldList now thinks all deletes were executed.

Since oldList is private, I cannot get away with composition. As an alternative to the PR I made providing an overload method to provide "oldList". You could also opt to make oldList protected so I could use composition to add this behaviour myself which perhaps would be less confusing for users of your library.

Hope to hear back from you.

Thanks

@sockeqwe
Copy link
Owner

sockeqwe commented Nov 28, 2016

I am in a scenario where I want to filter delete commands.

Couldn't you just filter out the delete commands after having calculated the difference:

List<AdapterCommand> commands = commandsCalculator.diff(newItems);
// TODO iterate over commands and remove all delete commands

or

Write your own AdapterCommandProcessor (extend from this class) and do in this something different with delete commands (or ignore them)

@nomisRev
Copy link
Author

Yes, I could but if I filter out the DeleteCommands, DiffCalculator is left in an ambiguous state.

@sockeqwe
Copy link
Owner

Could you explain why do you think that DiffCalculator could reach an ambiguous state?
The result produced by calculator.diff() is not depending nor has any reference to newList nor oldList. So modifying the result of calculator.diff() is safe and has no side effects to the internal behavior of DiffCalculator. Am I overlooking something?

@nomisRev
Copy link
Author

nomisRev commented Nov 28, 2016

If I look at the source code, the oldList is set by the first time you calculate a diff. So lets say this is [1,2,3], this results in insertRange. Now if I calculate a diff with a new list, lets say [1,2,4] that would result in a delete and a insert if I filter out the delete the list in my RV would be [1,2,3,4] but DiffCalculator thinks it is [1,2,4] because calling diff with oldList != null sets the oldList to [1,2,4].

You assume here that all AdapterCommands get executed resulting in an ambiguous state.

oldList.clear();
oldList.addAll(newList);

There was an issue in my logic. Because in following situation it wouldn't work anymore.
[1,2,3] -> [1,3,4] => delete 2 at position 1, move 3 to position 1, insert 4 at position 2.
If I filter out the delete, I get a conflict with move 3 to position 1 because 2 is still at that position.

I'm currently working around this by manipulating my data in the data layer so that a delete will not occur instead of filtering out the delete commands. And I guess that's also a better solution instead of doing logic in my view. With my requirements it would require a lot more logic in the view to get it working, and nobody wants that :D

I'm not sure if my requirements could be solved tho, but you definitely will end up in an ambiguous state if you filter out commands! Users may be unaware of that.

EDIT: I don't think my PR will be the solution for that problem (so feel free to close it). You'd still end up with inconsistency in your RecyclerView if you for example filter out deletes. As explained above, a move might conflict and RecyclerView will throw an Exception. Perhaps changing DiffCalculator to be more open for composition could offer a solution to some. But still I think the responsibility doesn't lie with DiffCalculator for this issue.

@nomisRev
Copy link
Author

Closed due to inactivity

@nomisRev nomisRev closed this Dec 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants