Make sure Launch Bar Manager sends event at a correct time - #1486
DangMinhTam382 wants to merge 3 commits into
Conversation
|
Hi Mr. @betamaxbandit , Before making this PR official, could you take a quick look and let me know what you think? Thanks! |
| // Set active mode | ||
| syncActiveMode(); | ||
| // Send notifications | ||
| fireActiveLaunchDescriptorChanged(); |
There was a problem hiding this comment.
Just moving this doesn't really solve the issue, the events for target & mode are still sent too early. This was why I suggested we create a new event that was for when any of them changed then we can just fire that once
There was a problem hiding this comment.
Thanks for the comment, Mr. @Kummallinen
About your suggestion, I did spend quite some time testing it out.
Tried posting a "LaunchBarChanged" event at the very end of these 3 methods: "setActiveLaunchDescriptor()", "setActiveLaunchMode()", "setActiveLaunchTarget()".
And it worked.
Then I realized that existing events "activeLaunchTargetChanged" & "activeLaunchModeChanged" are sent at the same point as the newly added event. The only exception is "activeLauchDescriptorChanged", which is fired too early.
With the changes in this commit, all three existing events and the new event are sent at the same time.
From an event-handling perspective, there is no noticeable difference between the existing events and the new event. That's why I decided to go with the approach in this commit.
--
About your concern:
the events for target & mode are still sent too early
Since "syncActiveTarget()" and "syncActiveMode()" is calling to "setActiveLaunchMode()" and "setActiveLaunchTarget()", respectively, the change events would still be fired too early even with the new event.
-> Possible suggestion to fix for this would be creating internal method that does NOT fire event,
Ex:
"syncActiveTarget()" -> "internalSetActiveLaunchTarget()" -> "internalSetActiveLaunchMode()"
However, this would be a much larger change and could potentially impact existing ISV implementations that relies on the current event behavior.
Therefore, instead of trying to ensure that events are fired at desired time, this commit focuses on ensuring that all Launch Bar components are up to date when a change event is fired/caught, even if the triggering method has not yet finished executing.
There was a problem hiding this comment.
Hi I am ok with this change.
However, could we add a regression test for this change? The bug is specifically about the state visible to a listener when activeLaunchDescriptorChanged() is called. I think the test should switch to a descriptor which results in a different target/mode and, from inside activeLaunchDescriptorChanged(), assert that getActiveLaunchTarget() and getActiveLaunchMode() already contain the values for the new descriptor. That should fail with the old ordering and protects the exact behaviour being fixed.
Also could you detail somewhere what other testing should be performed please? Is there a failing test at the moment that shows the problem obviously?
There was a problem hiding this comment.
Hi Mr. @betamaxbandit ,
Is there a failing test at the moment that shows the problem obviously?
ATM, there is no test case that could detect this issue.
I think the test should switch to a descriptor which results in a different target/mode and, from inside activeLaunchDescriptorChanged(), assert that getActiveLaunchTarget() and getActiveLaunchMode() already contain the values for the new descriptor. That should fail with the old ordering and protects the exact behaviour being fixed.
Thanks for the suggestion.
I added 3 UT that could detect this issue and should be enough to cover for future changes relate to event firing order.
Test for 3 events:
- activeLaunchDescriptorChanged
- activeLaunchTargetChanged
- activeLaunchModeChanged
| // Set active mode | ||
| syncActiveMode(); | ||
| // Send notifications | ||
| fireActiveLaunchDescriptorChanged(); |
There was a problem hiding this comment.
Hi I am ok with this change.
However, could we add a regression test for this change? The bug is specifically about the state visible to a listener when activeLaunchDescriptorChanged() is called. I think the test should switch to a descriptor which results in a different target/mode and, from inside activeLaunchDescriptorChanged(), assert that getActiveLaunchTarget() and getActiveLaunchMode() already contain the values for the new descriptor. That should fail with the old ordering and protects the exact behaviour being fixed.
Also could you detail somewhere what other testing should be performed please? Is there a failing test at the moment that shows the problem obviously?
| syncActiveTarget(); | ||
| // Set active mode | ||
| syncActiveMode(); | ||
| // Send notifications |
There was a problem hiding this comment.
This also changes the observable ordering of ILaunchBarListener notifications: target/mode notifications may now occur before the descriptor notification. I think that's reasonable for this fix, but because ILaunchBarListener is public API it would be useful for the regression test to make the intended semantics clear, in particular that listeners should rely on the manager state being consistent rather than on descriptor/target/mode notification ordering.
There was a problem hiding this comment.
Hi Mr. @betamaxbandit ,
listeners should rely on the manager state being consistent rather than on descriptor/target/mode notification ordering.
Agree! This is what this change is aiming for.
Whenever an event is fired, launch bar manager is expected to have up-to-date fields.
I added 3 UT to cover for this.
When active descriptor changed, and fireActiveLaunchDescriptorChanged() is called, current active mode and target are out of sync. This puts listeners at risk of handling event with false data for the changed descriptor. Changed it so that only fire event when related fields are finished updating
571f0e4 to
17292ff
Compare
17292ff to
aa7a299
Compare
DangMinhTam382
left a comment
DangMinhTam382
left a comment
There was a problem hiding this comment.
Hi Mr. @betamaxbandit ,
CC: Mr. @Kummallinen ,
I added some UTs and replied to your comments on this PR.
Please have a look at it again.
Also, build is failed atm with errors:
[ERROR] org.eclipse.linuxtools.docker.editor.ls 1.0.1.202609160755 requires Execution Environment that matches (&(osgi.ee=JavaSE)(version=25)) but the current resolution context uses [a.jre.javase 21.0.0]
[ERROR] Cannot resolve target definition:
[ERROR] Software being installed: org.eclipse.linuxtools.docker.feature.feature.group 5.25.0.202609160755
[ERROR] Missing requirement for filter properties ~= $0: org.eclipse.linuxtools.docker.editor.ls 1.0.1.202609160755 requires 'osgi.ee; (&(osgi.ee=JavaSE)(version=25)), filter=(!(org.eclipse.equinox.p2.disable.require.capability.osgi.ee=true))' but it could not be found
[ERROR] Cannot satisfy dependency: org.eclipse.linuxtools.docker.editor.ls.feature.feature.group 5.25.0.202609160755 depends on: org.eclipse.equinox.p2.iu; org.eclipse.linuxtools.docker.editor.ls [1.0.1.202609160755,1.0.1.202609160755]
[ERROR] Cannot satisfy dependency: org.eclipse.linuxtools.docker.feature.feature.group 5.25.0.202609160755 depends on: org.eclipse.equinox.p2.iu; org.eclipse.linuxtools.docker.editor.ls.feature.feature.group [5.25.0.202609160755,5.25.0.202609160755]
[ERROR]
Maybe something not working on the server?
Will look into this.
3 participants
When active descriptor changed, and fireActiveLaunchDescriptorChanged() is called, current active mode and target are out of sync. This puts listeners at risk of handling event with false data for the changed descriptor.
Changed it so that only fire event when related fields are finished updating