fix some DSS bugs #112142
fix some DSS bugs #112142
Conversation
There was a problem hiding this comment.
Currently, the new controller is also related to the old extent, so we need to switch the callbacks to the new extent.
There was a problem hiding this comment.
Without this may cause some memory leaks.
4fb0476 to
cf18488
Compare
There was a problem hiding this comment.
Can we have two lists, to verify that the event is not sent to the wrong controller?
There was a problem hiding this comment.
Actually, the old extent will never trigger an event but keep a reference for the DraggableScrollableController.
|
#111445 was reverted in #112293 The internal screenshot failure was an SVG icon in the shape of an arrow that broke because it became a flat line. Custom semantics were applied to the icon to make it a button. The semantics were also lost in the test failure, the button did not appear in the semantics tree anymore. @xu-baolin would you like to re-add the changes from #111445 here and I can test it to see if your additional changes fix these issues? Or would you like to re-land your change separately @moffatman? #111445 did fix a customer issue (#112020) where the customer was swapping out the entire DSS for another one in response to the extents changing, so re-landing #111445 is important to them. Let me know how you'd like to proceed, I am happy to help. |
|
Can we have a code sample to reproduce the failure, I don't understand how that could have happened, unless the SVG is relying on being rebuilt when only layout is changed? Which is an error in the SVG side. |
|
I am working on trying to get some sample code. The customer code is complex, I am trying to get as simple a repro as possible. |
|
The svg and semantics failures were a red herring. The customer code takes a different path with these changes. I've been working on the customer code directly to see if I can resolve it on their end, and I think I have it. They are passing widget states around so I am thinking this is a bug in their code, I am chatting with them to see if we can resolve it internally and re-land the changes in #111445 along with this PR. |
|
Before #111010, you would get a rebuild every frame during drag. After that PR you should only get 1 build, at the beginning. It sounds like I guess their SVG was depending on a rebuild to update itself? |
cf18488 to
1ba5b9d
Compare
|
@xu-baolin If you can combine with #111010, i would really appreciate it 😀 |
|
We should better separate them in case of one of those needing reverting again.😀 |
|
OK, but this one contains #111445 already, though, right? So I don't need to reopen that one? |
|
Yes, could you help try to relanding #111010? |
|
#112479 has landed 🎉 |
Piinks
left a comment
There was a problem hiding this comment.
LGTM, although there are some merge conflicts to resolve. Thanks everyone for working to re-land all of these changes
1ba5b9d to
8c41c5e
Compare
Fixes #112020
This is a further improvement of #111445, and fixes two issues,
CC @moffatman @Piinks