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

Deregister v2 #18

Merged
merged 4 commits into from
Mar 1, 2017
Merged

Deregister v2 #18

merged 4 commits into from
Mar 1, 2017

Conversation

anoop44
Copy link
Collaborator

@anoop44 anoop44 commented Feb 28, 2017

No description provided.

Copy link
Owner

@yshrsmz yshrsmz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome work!
beside from minor issue, it looks good to me.

private WeakReference<ViewTreeObserver.OnGlobalLayoutListener> mOnGlobalLayoutListenerWeakReference;

public SimpleDeRegister(Activity activity, ViewTreeObserver.OnGlobalLayoutListener globalLayoutListener){
this.mActivityWeakReference = new WeakReference<Activity>(activity);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you don't need this

* on 28/02/2017
*/

public interface Deregister {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason you prefer deregister over unregister?
I think Android people are more familiar with register - unregister pair than register - deregister(such as Context#registerReceiver and Context#unregisterReceiver).

Also, class/interface name should be noun.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we will go with the common usgea 'unregister'. Will update it with required changes

@@ -21,7 +21,7 @@
* @param activity Activity
* @param listener KeyboardVisibilityEventListener
*/
public static void setEventListener(final Activity activity,
public static Deregister setEventListener(final Activity activity,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, to meet the naming consistency, this should be renamed to registerEventListener

@yshrsmz yshrsmz merged commit 0b1bb0d into yshrsmz:master Mar 1, 2017
@yshrsmz
Copy link
Owner

yshrsmz commented Mar 1, 2017

thanks! will release in a few days after some cleanup :)

yshrsmz added a commit that referenced this pull request Mar 15, 2017
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.

None yet

2 participants