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

fix(6913): set/unset touchListener.owner onLoaded/onUnloaded #6922

Merged
merged 10 commits into from
Mar 15, 2019

Conversation

m-abs
Copy link
Contributor

@m-abs m-abs commented Feb 18, 2019

PR Checklist

What is the current behavior?

The TouchListener added to View on android have a owner-reference that is never cleared like it is on other native classes like the clickListener in button.android.ts.
So the TouchListener and View will keep pointing to each other as seen in #6913.

What is the new behavior?

Like on clickListener in button.android.ts owner is bound to the touchListener when onLoaded is called and removed again when onUnloaded is called.

Fixes #6913

@ghost ghost added the ♥ community PR label Feb 18, 2019
@m-abs
Copy link
Contributor Author

m-abs commented Feb 18, 2019

The one of the existing tests failed with:

JS: Test: --- [UTILS.test_releaseNativeObject_canBeCalledWithNativeObject] FAILED: __releaseNativeCounterpart is not defined, Stack: ReferenceError: __releaseNativeCounterpart is not defined

I don't think I caused it, but cannot check the "All existing tests are passing:"-box.

@ghost ghost assigned dtopuzov Feb 21, 2019
@ghost ghost added in progress and removed ♥ community PR labels Feb 21, 2019
@ghost ghost assigned vakrilov Feb 26, 2019
Copy link
Contributor

@vakrilov vakrilov left a comment

Choose a reason for hiding this comment

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

I would suggest to just use a WeakRef for the owner so that we don't have to worry about when and if we should clear it up. We do this in many other cases when there is a Listener or Handler object that is just used for a native callback (like here)

@m-abs
Copy link
Contributor Author

m-abs commented Feb 26, 2019

Hi @vakrilov

That was something I wondered when I reported the issue.

On iOS tns-core-modules uses a WeakRef for the owner references, but most cases I've seen for Android don't use a WeakRef for the same.
Like here:

const menuItemClickListener = new MenuItemClickListener(this);

class MenuItemClickListenerImpl extends java.lang.Object implements android.support.v7.widget.Toolbar.OnMenuItemClickListener {
constructor(public owner: ActionBar) {
super();
return global.__native(this);
}
onMenuItemClick(item: android.view.MenuItem): boolean {
let itemId = item.getItemId();
return this.owner._onAndroidItemSelected(itemId);
}
}
MenuItemClickListener = MenuItemClickListenerImpl;
appResources = application.android.context.getResources();
}

That was why I choose not to use one.

I'll update my PR as requested.

@vakrilov
Copy link
Contributor

test

@ghost ghost assigned manoldonev Mar 8, 2019
@manoldonev
Copy link
Contributor

test

@manoldonev manoldonev merged commit f056167 into NativeScript:master Mar 15, 2019
@ghost ghost removed the in progress label Mar 15, 2019
@m-abs m-abs deleted the fix/view-android-touchlistener branch March 15, 2019 12:18
@lock
Copy link

lock bot commented Mar 17, 2020

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Mar 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is there a memory problem with the owner references on android?
5 participants