Bug 222990

<rdar://problem/75270368>

Created attachment 423009 [details] Video of Patch v1.0-1.1 (With Exaggerated Checkbox Labels)

Created attachment 423010 [details] Patch v1.0

Comment on attachment 423010 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=423010&action=review > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:177 > + if (!flag && this.isAttached) Please use `this.collapsed` instead as that has the fallback `|| false`. Also, I think `flag` should be removed altogether because the callsite doesn't provide any arguments 😅 > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:208 > + if (!this._requiredMinimumWidthForMultipleSidebars && this.isAttached) Rather than have this here, I think it may be worth inverting this and moving it to `_updateMinimumWidthForMultipleSidebars` (and probably add a `force` parameter that will overrule `!this._requiredMinimumWidthForMultipleSidebars`) so that we don't try to set `_requiredMinimumWidthForMultipleSidebars` until we know we're ready. > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:91 > + if (!this.selectedSidebarPanel) > + this.selectedSidebarPanel = sidebarPanel; this.selectedSidebarPanel ||= sidebarPanel; > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:105 > + if (!this.collapsed) Why? I would consider it a bug to try to `set selectedSidebarPanel` while `collapsed`. > Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:115 > + if (!this.collapsed) ditto (:105)

Comment on attachment 423010 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=423010&action=review >> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:177 >> + if (!flag && this.isAttached) > > Please use `this.collapsed` instead as that has the fallback `|| false`. > > Also, I think `flag` should be removed altogether because the callsite doesn't provide any arguments 😅 Based on you comment below, this check should probably just be a part of `_updateMinimumWidthForMultipleSidebars` as well. >> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:208 >> + if (!this._requiredMinimumWidthForMultipleSidebars && this.isAttached) > > Rather than have this here, I think it may be worth inverting this and moving it to `_updateMinimumWidthForMultipleSidebars` (and probably add a `force` parameter that will overrule `!this._requiredMinimumWidthForMultipleSidebars`) so that we don't try to set `_requiredMinimumWidthForMultipleSidebars` until we know we're ready. Sounds reasonable to me. >> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:91 >> + this.selectedSidebarPanel = sidebarPanel; > > this.selectedSidebarPanel ||= sidebarPanel; How quickly I forget after spending so long working in the backend 😅 >> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:105 >> + if (!this.collapsed) > > Why? I would consider it a bug to try to `set selectedSidebarPanel` while `collapsed`. I need to dive deeper on this. I’m not sure I understand the ramifications of r270134 on this logic, but without this check it was possible to attempt to add the sub view twice. It seems as if we should be able to set a selected panel before uncollapsing a sidebar, though. Otherwise we need to wait until the sidebar is showing to set the selected panel. I’ll look tomorrow.

Comment on attachment 423010 [details] Patch v1.0 View in context: https://bugs.webkit.org/attachment.cgi?id=423010&action=review >>> Source/WebInspectorUI/UserInterface/Views/SingleSidebar.js:105 >>> + if (!this.collapsed) >> >> Why? I would consider it a bug to try to `set selectedSidebarPanel` while `collapsed`. > > I need to dive deeper on this. I’m not sure I understand the ramifications of r270134 on this logic, but without this check it was possible to attempt to add the sub view twice. It seems as if we should be able to set a selected panel before uncollapsing a sidebar, though. Otherwise we need to wait until the sidebar is showing to set the selected panel. I’ll look tomorrow. There are a few places where a selectedSidebarPanel will be set regardless of the collapsed state of said sidebar: • `ContentBrowserTabContentView.__prototype__.showNavigationSidebarPanel::140-149` - Last user setting is honored for collapsed state, and then the only panel is set as the selected panel. • `ContentBrowserTabContentView.__prototype__.showDetailsSidebarPanels::195-203` - Last user setting is honored for collapsed state, and then a panel is selected based on the last panel the user selected. • `MultiSidebar.__prototype__._makeSidebarPanelExclusive` - Will need to set a selected panel as well, either transitively through addSidebarPanel or explicitly. Since this is the case I'm needing to fix, I'm going to just reorder here and make sure the panel is set as selected after un-collapsing it.

Created attachment 423058 [details] Patch v1.1 - Revised setting selected panel for exclusive SingleSidebars

Comment on attachment 423058 [details] Patch v1.1 - Revised setting selected panel for exclusive SingleSidebars View in context: https://bugs.webkit.org/attachment.cgi?id=423058&action=review r=me > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:189 > + _updateMinimumWidthForMultipleSidebars({ignoreExistingComputedValue} = {}) NIT: While I do appreciate you using an object for options like this, personally for things that are inside the same class (and only have one argument) I'd just make this a `force` parameter that's not in an object. Nothing wrong with how you have it here tho so your call :) > Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:256 > sidebar.selectedSidebarPanel = sidebarPanel; Remove this?

Comment on attachment 423058 [details] Patch v1.1 - Revised setting selected panel for exclusive SingleSidebars View in context: https://bugs.webkit.org/attachment.cgi?id=423058&action=review >> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:189 >> + _updateMinimumWidthForMultipleSidebars({ignoreExistingComputedValue} = {}) > > NIT: While I do appreciate you using an object for options like this, personally for things that are inside the same class (and only have one argument) I'd just make this a `force` parameter that's not in an object. Nothing wrong with how you have it here tho so your call :) I went back and forth on this myself, but erred on the side of being slightly more verbose so it was clear at the callsites what was being configured, although I can appreciate that the answer in this case would be less than 50 lines below each call site. >> Source/WebInspectorUI/UserInterface/Views/MultiSidebar.js:256 >> sidebar.selectedSidebarPanel = sidebarPanel; > > Remove this? Oh, yep...

Created attachment 423295 [details] Patch v1.2 - Review notes

Committed r274465: <https://commits.webkit.org/r274465> All reviewed patches have been landed. Closing bug and clearing flags on attachment 423295 [details].