Fp 2644: UI is frozen in certain split configurations in the ide #371
Conversation
quirinpa
commented
Nov 12, 2024
•
quirinpa
commented
- FP-2644: UI is frozen in certain split configurations in the IDE
| tabExists = _layout.dockbox.children[0].tabs.find( | ||
| (t) => t.id === layoutActiveId, | ||
| ); | ||
| if (_layout.dockbox.children[0].tabs) |
There was a problem hiding this comment.
This is incorrect. It completly alters the logic here.
First we were checking if the tab existed in the maxbox (maximized), if it didn't, then we went look for it in the dockbox.
The way you did it we'll always overwrite tabExists even if it exists in the maxbox.
There was a problem hiding this comment.
It only alters the logic in the sense that it doesn't bug out if that object "tabs" is does not have a "find" method.
So I think you are incorrect about your pointing out of non-correctness.
It does not alter the logic at all.
There was a problem hiding this comment.
Nevermind, it seemed like you replaced the previous if with the new one. But apparently you just added a new one inside it.
Good call 👍
There was a problem hiding this comment.
Only suggestion here would be to do:
const dockboxChildren = _layout.dockbox.children[0].tabs;
And use it in both places.
| tabExists = _layout.dockbox.children[0].tabs.find( | ||
| (t) => t.id === layoutActiveId, | ||
| ); | ||
| if (_layout.dockbox.children[0].tabs) |
There was a problem hiding this comment.
Nevermind, it seemed like you replaced the previous if with the new one. But apparently you just added a new one inside it.
Good call 👍
| tabExists = _layout.dockbox.children[0].tabs.find( | ||
| (t) => t.id === layoutActiveId, | ||
| ); | ||
| if (_layout.dockbox.children[0].tabs) |
There was a problem hiding this comment.
Only suggestion here would be to do:
const dockboxChildren = _layout.dockbox.children[0].tabs;
And use it in both places.
|
Since I have to update the branch anyway, I would like to integrate your suggestion. Thank you @manuelnogueiramov for checking this out. |
…n-certain-split-configurations-in-the-ide
|
The new code looks weird... where are we checking for the |
|
You are incorrect, Manuel. The variable you suggested was already defined. |
…rations-in-the-ide
…rations-in-the-ide
| let tabExists = maxboxChildren?.tabs.find((t) => t.id === layoutActiveId); | ||
|
|
||
| if (!tabExists) { | ||
| tabExists = _layout.dockbox.children[0].tabs.find( | ||
| (t) => t.id === layoutActiveId, | ||
| ); | ||
| if (maxboxChildren?.tabs) | ||
| tabExists = maxboxChildren.tabs.find((t) => t.id === layoutActiveId); | ||
| } | ||
| activeTabId.current = layoutActiveId; |
There was a problem hiding this comment.
I looked at this for some time, and this makes 0 sense. We're NEVER checking dockbox anymore for the tab. The first approach you did, seemed to work correctly. This is just nonsense. line 144 that you introduced is exactly the same as line 140. Where it used to be a check inside dockbox, not maxbox again.
There was a problem hiding this comment.
Ok. In this case you seem correct. I'll be back on this shortly, to test it.
There was a problem hiding this comment.
Testing it, it just works. Maybe that's due to the fact that the maxbox and the dockbox children are the same.
Do you have any reason to think of a situation where they aren't?
There was a problem hiding this comment.
Testing what? Did you test every possible scenario? If you did and nothing broke, we're good to go. As I doubt that happened, my request changes still mantains. I know I added that a few years ago because of an edge case scenario. Approving this code as is will fix YOUR issue and cause at least 1 regression.
Before you ask, I don't remember what, but if you dig a bit you'll find the ticket that first originated that code and then you can test it.
…rations-in-the-ide
|
