FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(6913): set/unset touchListener.owner onLoaded/onUnloaded by m-abs · Pull Request #6922 · NativeScript/NativeScript · GitHub

/ NativeScript Public

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

Merged
manoldonev merged 10 commits into
NativeScript:masterfrom
m-abs:fix/view-android-touchlistener
Mar 15, 2019
ConversationCommits (10)ChecksFiles changed
Merged

fix(6913): set/unset touchListener.owner onLoaded/onUnloaded#6922
manoldonev merged 10 commits into
NativeScript:masterfrom
m-abs:fix/view-android-touchlistener

Conversation

m-abs commented Feb 18, 2019

Copy link
Copy Markdown
Contributor

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 added the ♥ community PR label Feb 18, 2019

m-abs commented Feb 18, 2019

Copy link
Copy Markdown
Contributor Author

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 assigned dtopuzov Feb 21, 2019
ghost added in progress and removed ♥ community PR labels Feb 21, 2019
ghost assigned vakrilov Feb 26, 2019
vakrilov suggested changes Feb 26, 2019
View reviewed changes

vakrilov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Feb 26, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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:

NativeScript/tns-core-modules/ui/action-bar/action-bar.android.ts

Line 145 in 23147ae

const menuItemClickListener = new MenuItemClickListener(this);

NativeScript/tns-core-modules/ui/action-bar/action-bar.android.ts

Lines 39 to 53 in 23147ae

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 commented Feb 26, 2019

Copy link
Copy Markdown
Contributor

test

ghost assigned manoldonev Mar 8, 2019

manoldonev commented Mar 14, 2019

Copy link
Copy Markdown
Contributor

test

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

lock Bot commented Mar 17, 2020

Copy link
Copy Markdown

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 Bot locked and limited conversation to collaborators Mar 17, 2020
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
vakrilov vakrilov approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

manoldonev

vakrilov

Labels

cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

Back | FazBrowse Home | New Git URL