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.

Sign up for GitHub

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

Jump to bottom

Correctly register weak property change listener on InputOutput #7430

Merged
merged 2 commits into from
Jul 17, 2024
Merged

Correctly register weak property change listener on InputOutput #7430

merged 2 commits into from
Jul 17, 2024

Conversation

matthiasblaesing
Copy link
Contributor

This warning was observed:

WARNING [org.openide.util.WeakListenerImpl]: Can't remove java.beans.PropertyChangeListener using method org.netbeans.modules.terminal.ioprovider.TerminalInputOutput.removePropertyChangeListener from org.netbeans.modules.terminal.ioprovider.TerminalInputOutput@10efb8c9
WARNING [org.openide.util.WeakListenerImpl]: Can't remove java.beans.PropertyChangeListener using method org.netbeans.modules.terminal.ioprovider.TerminalInputOutput.removePropertyChangeListener from org.netbeans.modules.terminal.ioprovider.TerminalInputOutput@3a72784a

This was one of the candidates for the source of the problem.

@matthiasblaesing matthiasblaesing added this to the NB23 milestone Jun 2, 2024
@matthiasblaesing matthiasblaesing added the CI continuous integration changes label Jun 2, 2024
@matthiasblaesing
Copy link
Contributor Author

Label only added to shutup CI complaint.

@apache apache locked and limited conversation to collaborators Jun 2, 2024
@apache apache unlocked this conversation Jun 2, 2024
@neilcsmith-net
Copy link
Member

Isn't this because the pcs and vcs in TerminalInputOutput are firing with the wrong source? No objection to fixing like this, but could be done without additional API.

@matthiasblaesing
Copy link
Contributor Author

Isn't this because the pcs and vcs in TerminalInputOutput are firing with the wrong source? No objection to fixing like this, but could be done without additional API.

The source of the events is indeed the TerminalInputOutput, but the registration is not done on it. That is a capability implemented by the IONotifier implementation. This is hidden by the IONotifier#addPropertyChangeListener implementation. The problem is now, that to create a WeakListener you need access to the object you need to detach (in this case the IONotifier implementation) from when the listener is collected.

I though about directly implementing the registration logic (essentially copying the IONotifier#find method), but discarded that so that the logic is all unified in IONotifier.

@neilcsmith-net
Copy link
Member

The source of the events is indeed the TerminalInputOutput, but the registration is not done on it.

Which is the problem IMO. ie. the pcs() method construction code at https://github.com/apache/netbeans/blob/master/ide/terminal.nb/src/org/netbeans/modules/terminal/ioprovider/TerminalInputOutput.java#L151 should be passing in the instance of MyIONotifier not this.

@matthiasblaesing
Copy link
Contributor Author

I think it is even more ugly as I though. I'll have to revisit this later.

@matthiasblaesing matthiasblaesing marked this pull request as draft June 3, 2024 17:57
@matthiasblaesing
Correctly register weak property change listener on InputOutput
1c0095b 
This warning was observed:

WARNING [org.openide.util.WeakListenerImpl]: Can't remove java.beans.PropertyChangeListener using method org.netbeans.modules.terminal.ioprovider.TerminalInputOutput.removePropertyChangeListener from org.netbeans.modules.terminal.ioprovider.TerminalInputOutput@10efb8c9
WARNING [org.openide.util.WeakListenerImpl]: Can't remove java.beans.PropertyChangeListener using method org.netbeans.modules.terminal.ioprovider.TerminalInputOutput.removePropertyChangeListener from org.netbeans.modules.terminal.ioprovider.TerminalInputOutput@3a72784a

This was one of the candidates for the source of the problem.


A custom implementation of WeakReference was used as the Implementation
from org.openide.util.WeakListeners makes the assumption that the 
source of the property change is the place where the property change
listener can be/has to be disconnected from. The architecture of the
IONotifer though is different in that the registering logic is defined
on IONotifer, but the source of the PropertyChange is the InputOutput.
@matthiasblaesing matthiasblaesing force-pushed the invalid_weak_listener_registration branch from ca111d2 to 1c0095b Compare June 16, 2024 13:36
@matthiasblaesing matthiasblaesing marked this pull request as ready for review June 16, 2024 14:33
@matthiasblaesing
Copy link
Contributor Author

@neilcsmith-net I reworked this to not require API changes. I decided against changing the source of the events or the way of the intregration, so that the change does not affect behavior outside the target handler. For that handler I implemented a WeakListener myself, which is not as universal as the one from the openide package, but is tailored to the situation.

@matthiasblaesing
Copy link
Contributor Author

I intent to merge this early next week. If anyone wants to object, please do so now.

@matthiasblaesing
Copy link
Contributor Author

As I was reminded, that we are already freezing, lets get this in now.

@matthiasblaesing matthiasblaesing merged commit bc7446e into apache:master Jul 17, 2024
32 checks passed
@matthiasblaesing matthiasblaesing deleted the invalid_weak_listener_registration branch July 21, 2024 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CI continuous integration changes
Projects
None yet
Milestone
NB23
Development

Successfully merging this pull request may close these issues.
2 participants
@matthiasblaesing @neilcsmith-net