fix layout#296228
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the layout() method in ActionList by extracting its logic into two helper methods (computeHeight() and computeMaxWidth()) to improve code organization. However, it also introduces a significant behavioral change to how the widget calculates its maximum height, making it position-aware rather than using a fixed percentage of window height.
Changes:
- Extracts height computation logic into a new
computeHeight()private method with enhanced position-aware height calculation - Extracts width computation logic into a new
computeMaxWidth()private method with early-return optimizations - Simplifies the main
layout()method to orchestrate the extracted computation methods
Comments suppressed due to low confidence (3)
src/vs/platform/actionWidget/browser/actionList.ts:698
- After restoring visible items at line 698, the list layout needs to be updated. The list was previously laid out with allItemsHeight at line 682 for width measurement purposes, but after restoring the visible items, the list is left in an incorrect layout state. In the original code, after restoring visible items, it would call this._list.layout(listHeight) to restore the proper layout. The new code is missing this restoration call, which means the list will briefly have an incorrect height until the subsequent layout() call at line 724, potentially causing visual glitches.
This issue also appears on line 721 of the same file.
this._list.splice(0, allItems.length, visibleItems);
src/vs/platform/actionWidget/browser/actionList.ts:647
- The height computation logic has been significantly changed, not just refactored. The old code used a simple 70% of window height as the maximum height. The new code:
- Calculates available height based on the widget's actual position (windowHeight - widgetTop - padding)
- Falls back to 70% of window height only when widgetTop <= 0
- Ensures a minimum height of 3 action lines plus filter height
This is a behavioral change that makes the widget position-aware and more adaptive, which seems intentional but goes beyond a simple "fix layout" refactoring. Consider whether this change should be documented or split into a separate commit, as it modifies the user-visible behavior of how the widget sizes itself.
const filterHeight = this._filterContainer ? 36 : 0;
const padding = 10;
const targetWindow = dom.getWindow(this.domNode);
const windowHeight = this._layoutService.getContainer(targetWindow).clientHeight;
const widgetTop = this.domNode.getBoundingClientRect().top;
const availableHeight = widgetTop > 0 ? windowHeight - widgetTop - padding : windowHeight * 0.7;
const maxHeight = Math.max(availableHeight, this._actionLineHeight * 3 + filterHeight);
const height = Math.min(listHeight + filterHeight, maxHeight);
return height - filterHeight;
src/vs/platform/actionWidget/browser/actionList.ts:721
- The list.layout() call at line 721 appears redundant since line 724 immediately calls layout() again with both dimensions. The first call layouts the list with only height, and the second call at line 724 provides both height and width. Consider removing line 721 to avoid the unnecessary layout operation, as the final call at line 724 will correctly set both dimensions.
this._list.layout(listHeight);
No description provided.