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

Divide OnUpdateListener in two diferentes Listener #10

Closed
danielpassos opened this issue Dec 3, 2015 · 10 comments
Closed

Divide OnUpdateListener in two diferentes Listener #10

danielpassos opened this issue Dec 3, 2015 · 10 comments
Milestone

Comments

@danielpassos
Copy link
Contributor

Sometimes you want track onDeleteConfirmed but not necessarily onLoadComplete and vice and versa

@davideas
Copy link
Owner

davideas commented Dec 5, 2015

Hello @danielpassos, during the development I also thought to divide the listeners, but in the end I opted for 1 only. I can separate them again now.
But I will not remove the parameter from the constructor because it's easier for a developer (the adapter can do these assignments for him), because changing the signature will change the current implementation of hundreds of people, because developers don't know the internal names of the listeners and they have to check the adapter code.

@danielpassos
Copy link
Contributor Author

because changing the signature will change the current implementation of hundreds of people

I agree with that, I'll put it back in my PR, but pass a Object in the contructor seems wrong to me. Maybe deprecate that and create a new one or only deprecate that to remove in future releases. wdyt?

@davideas
Copy link
Owner

davideas commented Dec 5, 2015

Wait to change the PR.
Also I was thinking that the method startUndoTimer can accept the listener for confirmation of deletion: basically it's only in that occasion and it is not necessary to have listener as member but can be kept as final parameter.

@danielpassos
Copy link
Contributor Author

Wait to change the PR.

Done. I put it back as @deprecated but leave the example app using the new one

Also I was thinking that the method startUndoTimer can accept the listener for confirmation of deletion: basically it's only in that occasion and it is not necessary to have listener as member but can be kept as final parameter.

Not sure if I got you point here.

@davideas
Copy link
Owner

davideas commented Dec 5, 2015

I mean to pass the listener to this function:

public void startUndoTimer(final OnDeleteCompleteListener listener, long timeout) {
    mHandler = new Handler(Looper.getMainLooper(), new Handler.Callback() {
            public boolean handleMessage(Message message) {
                if (listener != null) listener.onDeleteConfirmed();
                ...
            }
        });
    ...
}

@danielpassos
Copy link
Contributor Author

Gotcha. Must better and done! :)

@davideas
Copy link
Owner

davideas commented Dec 6, 2015

@danielpassos, so about OnUpdateListener, I am not sure to keep it, because after all I don't see a big advantage of an Asynchronous loading: usually the list is already loaded Asynchronously somewhere else.
I wanted a suggestion from you.

@danielpassos
Copy link
Contributor Author

I totally agree with you but we need to keep in mind it will broke the old project/compatibility. I know it's sucks, but it's a library and what we do in libraries/frameworks is:

  1. Make it as deprecated
  2. Update the docs
  3. Announce in some place we are about to remove it.
  4. In a new version bumping MAJOR (see the semver) and remove it. It make clear for the developers we broke the compatibility.

@davideas
Copy link
Owner

davideas commented Dec 6, 2015

Indeed it will break compatibility, but before release as major version, I wanted to add new functionalities, such as animation on loading. I already know how to do it, maybe multi ViewHolder, fix the way how ItemAnimator are used, use or import a nice SearchView, that I have identified, in the example App.
So your changes will not be released soon, and we have time to inform ppl about these deprecation.
In the meantime I have to work also on others projects...
Thanks for the suggestion.

@danielpassos
Copy link
Contributor Author

You can release a small version bumping MINOR with the new listeners or for every new feature you add, and than release a new one with a new big improvement/removing deprecation bumping the MAJOR

@davideas davideas added this to the 4.2.0 milestone Dec 6, 2015
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