Improve support for dynamic layout #608
Comments
|
@zepumph replied:
|
|
|
From phetsims/sun#541 (comment) SR: Should we investigate a strategy where the layout code is factored out to a separate function, and can be re-run when necessary? KP: Let's leave the "hack" strategy for now, and see how the relayout strategy works out. KP: Should there be a button in studio that runs the relayout? So you can control where the holes appear? SR: Should there be a "participatesInLayoutProperty"? |
|
To prototype with this, I'd like to choose a simulation that satisfies the following criteria:
|
|
Even though I'm not the lead, and it's been a while since it was worked on, it seems like Beer's Law Lab is a good candidate. I'll work in a branch so as not to disturb anything. |
|
On second thought, these changes are spanning several repos, so I'll use a patch instead: Index: sun/js/ComboBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBox.js (revision 38c1b0aaafd92d8ab725eb509a1ccc51e523701e)
+++ sun/js/ComboBox.js (date 1581654326206)
@@ -169,6 +169,8 @@
listParent.addChild( this.listBox );
this.listParent = listParent; // @private
+ this.listBox.on( 'bounds', () => {this.moveListBox();} );
+
this.mutate( options );
// Clicking on the button toggles visibility of the list box
Index: sun/js/ComboBoxListBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBoxListBox.js (revision 38c1b0aaafd92d8ab725eb509a1ccc51e523701e)
+++ sun/js/ComboBoxListBox.js (date 1581654326210)
@@ -138,6 +138,13 @@
children: listItemNodes
} );
+ listItemNodes.forEach( listItemNode => {
+ listItemNode.on( 'visibility', () => {
+ content.updateLayout();
+ this.trigger( 'bounds' );// FAKE
+ } );
+ } );
+
// Adjust margins to account for highlight overlap
options.xMargin = options.xMargin / 2;
options.yMargin = options.yMargin / 2;
Index: scenery/js/nodes/LayoutBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/nodes/LayoutBox.js (revision 4ace157efd412f4f193fc86cd3503b2a5c71ac91)
+++ scenery/js/nodes/LayoutBox.js (date 1581644166199)
@@ -161,8 +161,8 @@
let position = 0;
for ( let i = 0; i < children.length; i++ ) {
const child = children[ i ];
- if ( !child.bounds.isValid() ) {
- continue; // Skip children without bounds
+ if ( !child.bounds.isValid() || !child.visible ) {
+ continue; // Skip children without bounds, and skip invisible children
}
child[ layoutPosition ] = position;
child[ layoutAlignment ] = this._align === 'origin' ? 0 : alignmentBounds[ layoutAlignment ];
Index: beers-law-lab/js/beerslaw/view/WavelengthControls.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- beers-law-lab/js/beerslaw/view/WavelengthControls.js (revision e03aa26a38dd282d3b744502c49dcd555b8f9b20)
+++ beers-law-lab/js/beerslaw/view/WavelengthControls.js (date 1581643656277)
@@ -59,15 +59,12 @@
const valueDisplay = new Text( formatWavelength( light.wavelengthProperty.get() ), {
font: new PhetFont( 20 ),
fill: 'black',
- y: label.y, // align baselines
tandem: tandem.createTandem( 'valueDisplay' )
} );
const valueBackground = new Rectangle( 0, 0, valueDisplay.width + xMargin + xMargin, valueDisplay.height + yMargin + yMargin, {
fill: 'white',
- stroke: 'lightGray',
- left: label.right + 10,
- centerY: valueDisplay.centerY
+ stroke: 'lightGray'
} );
valueDisplay.right = valueBackground.right - xMargin; // right aligned
@@ -150,10 +147,24 @@
} );
// sync displayed value with model
- light.wavelengthProperty.link( function( wavelength ) {
+ const updateValueDisplayLayout = function( wavelength ) {
valueDisplay.text = formatWavelength( wavelength );
valueDisplay.right = valueBackground.right - xMargin; // right aligned
- } );
+ };
+ light.wavelengthProperty.link( updateValueDisplayLayout );
+
+ this.layoutWavelengthControls = () => {
+ valueDisplay.y = label.y; // align baselines
+
+ valueBackground.mutate( {
+ left: label.right + 10,
+ centerY: valueDisplay.centerY
+ } );
+
+ updateValueDisplayLayout( light.wavelengthProperty.value );
+ };
+
+ setInterval( () => this.layoutWavelengthControls(), 1000 );
}
beersLawLab.register( 'WavelengthControls', WavelengthControls );
@@ -164,6 +175,10 @@
return inherit( Panel, WavelengthControls, {
+ layout: function() {
+ this.layoutWavelengthControls();
+ },
+
// @public
reset: function() {
this.variableWavelengthProperty.reset();
This patch changes VBox so that invisible things don't take up space, and a layout is triggered when something's visibility changes. I'm not yet sure whether this is a reasonable approach to take for all our code.
This same patch also works nicely for control panels:
The main questions are:
|
|
This approach would be usable by babel--translators could immediately see the new strings appear in the sim as they are typed. Maybe overkill? |
|
What about this as a general pattern?
Could this lead to cycles? |
|
@samreid and I looked into the above patch, scratched it, and made a simpler changeset that may do similar logic. Index: gravity-and-orbits/js/common/view/CheckboxPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/CheckboxPanel.js (revision 16d020fc0a29224df0bdffae56d975c820599219)
+++ gravity-and-orbits/js/common/view/CheckboxPanel.js (date 1581703258727)
@@ -158,7 +158,7 @@
super( merge( {
children: children,
- resize: false,
+ resize: true,
spacing: SPACING,
align: 'left',
bottom: -12,
Index: sun/js/ComboBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBox.js (revision e11d932d7aa6e612cb406c911c19e4c65871edf1)
+++ sun/js/ComboBox.js (date 1581702832849)
@@ -229,6 +229,8 @@
};
this.enabledProperty.link( enabledObserver );
+ this.listBox.on( 'bounds', () => this.moveListBox() );
+
// @private for use via PhET-iO, see https://github.com/phetsims/sun/issues/451
// This is not generally controlled by the user, so it is not reset when the Reset All button is pressed.
this.displayOnlyProperty = new BooleanProperty( false, {
Index: scenery/js/nodes/LayoutBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/nodes/LayoutBox.js (revision 4ace157efd412f4f193fc86cd3503b2a5c71ac91)
+++ scenery/js/nodes/LayoutBox.js (date 1581703368529)
@@ -161,13 +161,14 @@
let position = 0;
for ( let i = 0; i < children.length; i++ ) {
const child = children[ i ];
- if ( !child.bounds.isValid() ) {
+ if ( !child.bounds.isValid() || !child.visible ) {
continue; // Skip children without bounds
}
child[ layoutPosition ] = position;
child[ layoutAlignment ] = this._align === 'origin' ? 0 : alignmentBounds[ layoutAlignment ];
position += child[ layoutDimension ] + this._spacing; // Move forward by the node's size, including spacing
}
+ this.trigger( 'bounds' ); // this didn't solve the GOA checkbox panel issue like we thought it would.
},
/**
@@ -204,6 +205,7 @@
onLayoutBoxChildInserted: function( node ) {
if ( this._resize ) {
node.onStatic( 'bounds', this._updateLayoutListener );
+ node.onStatic( 'visibility', this._updateLayoutListener );
}
},
@@ -216,6 +218,7 @@
onLayoutBoxChildRemoved: function( node ) {
if ( this._resize ) {
node.offStatic( 'bounds', this._updateLayoutListener );
+ node.offStatic( 'visibility', this._updateLayoutListener );
}
},
Index: gravity-and-orbits/js/common/view/GravityAndOrbitsControlPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/GravityAndOrbitsControlPanel.js (revision 16d020fc0a29224df0bdffae56d975c820599219)
+++ gravity-and-orbits/js/common/view/GravityAndOrbitsControlPanel.js (date 1581702832853)
@@ -51,7 +51,7 @@
assert && assert( sections.length === 5, 'There should be 5 sections in the GravityAndOrbitsControlPanel' );
- const vBox = new VBox( { children: sections, spacing: 4, y: 5, resize: false, align: 'left' } );
+ const vBox = new VBox( { children: sections, spacing: 4, y: 5, resize: true, align: 'left' } );
super( vBox, options );
// resize the separators to allow them to go inside the panel margins
```</details> |
|
This patch is more general and has more coverage for different components. LayoutBox is resizing nicely in Gravity and Orbits, ComboBox listboxes, Beer's Law Lab. Changes were concentrated in Node.js Index: gravity-and-orbits/js/common/view/CheckboxPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/CheckboxPanel.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/CheckboxPanel.js (date 1581765610411)
@@ -158,7 +158,7 @@
super( merge( {
children: children,
- resize: false,
+ resize: true,
spacing: SPACING,
align: 'left',
bottom: -12,
Index: gravity-and-orbits/js/common/view/TimeControlPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/TimeControlPanel.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/TimeControlPanel.js (date 1581770018429)
@@ -52,7 +52,7 @@
}
super( merge( {
- resize: false,
+ resize: true,
spacing: 10,
children: [ rewindButton, playPauseButton, stepButton ]
}, options ) );
Index: gravity-and-orbits/js/common/view/GravityAndOrbitsControlPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/GravityAndOrbitsControlPanel.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/GravityAndOrbitsControlPanel.js (date 1581765610418)
@@ -51,7 +51,7 @@
assert && assert( sections.length === 5, 'There should be 5 sections in the GravityAndOrbitsControlPanel' );
- const vBox = new VBox( { children: sections, spacing: 4, y: 5, resize: false, align: 'left' } );
+ const vBox = new VBox( { children: sections, spacing: 4, y: 5, resize: true, align: 'left' } );
super( vBox, options );
// resize the separators to allow them to go inside the panel margins
Index: gravity-and-orbits/js/common/view/MassControlPanel.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/MassControlPanel.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/MassControlPanel.js (date 1581770018447)
@@ -65,7 +65,7 @@
fontWeight: 'bold',
fill: GravityAndOrbitsColorProfile.panelTextProperty,
maxWidth: 175,
- resize: false,
+ resize: true,
tandem: options.tandem.createTandem( massSettableBody.labelTandemName )
} );
@@ -81,7 +81,7 @@
const sliderVBox = new VBox( {
top: labelHBox.bottom + 8,
- resize: false,
+ resize: true,
children: [
new HStrut( 220 ),
new BodyMassControl(
@@ -99,7 +99,7 @@
children.push( sliderNode );
}
- const vBox = new VBox( { children: children, spacing: 15, y: 5, resize: false, align: 'left' } );
+ const vBox = new VBox( { children: children, spacing: 15, y: 5, resize: true, align: 'left' } );
super( vBox, options );
}
}
Index: sun/js/ComboBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sun/js/ComboBox.js (revision e11d932d7aa6e612cb406c911c19e4c65871edf1)
+++ sun/js/ComboBox.js (date 1581765610401)
@@ -229,6 +229,8 @@
};
this.enabledProperty.link( enabledObserver );
+ this.listBox.on( 'bounds', () => this.moveListBox() );
+
// @private for use via PhET-iO, see https://github.com/phetsims/sun/issues/451
// This is not generally controlled by the user, so it is not reset when the Reset All button is pressed.
this.displayOnlyProperty = new BooleanProperty( false, {
Index: gravity-and-orbits/js/common/view/GravityAndOrbitsScreenView.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/GravityAndOrbitsScreenView.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/GravityAndOrbitsScreenView.js (date 1581770176390)
@@ -58,17 +58,25 @@
const sceneView = scene.sceneView;
const massControlPanel = new MassControlPanel( scene.getMassSettableBodies(), {
- top: controlPanel.bottom + MARGIN,
- right: this.layoutBounds.right - MARGIN,
// Nest under massesControlPanel, see https://github.com/phetsims/gravity-and-orbits/issues/284#issuecomment-554106611
tandem: massesControlPanelTandem.createTandem( scene.massControlPanelTandemName )
} );
scene.massControlPanel = massControlPanel;
+ const updateLayout = () => {
+ massControlPanel.mutate( {
+ top: controlPanel.bottom + MARGIN,
+ right: this.layoutBounds.right - MARGIN
+ } );
+ };
+ updateLayout();
+ controlPanel.on( 'bounds', updateLayout );
+
playAreaNode.addChild( sceneView );
massesControlPanel.addChild( massControlPanel );
} );
+
this.addChild( playAreaNode );
this.addChild( massesControlPanel );
Index: scenery/js/nodes/Node.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/nodes/Node.js (revision 4ace157efd412f4f193fc86cd3503b2a5c71ac91)
+++ scenery/js/nodes/Node.js (date 1581770773896)
@@ -1115,7 +1115,11 @@
i = this._children.length;
while ( i-- ) {
- this._childBounds.includeBounds( this._children[ i ]._bounds );
+
+ // Invisible children are not included in the bounds, to facilitate relayout for PhET-iO
+ if ( this._children[ i ].visible ) {
+ this._childBounds.includeBounds( this._children[ i ]._bounds );
+ }
}
// run this before firing the event
@@ -3037,6 +3041,9 @@
// Defined in Accessibility.js
this._accessibleDisplaysInfo.onVisibilityChange( visible );
+ // When the visibility of a Node changes, signify this as a bounds change so that parents relayout
+ this.invalidateBounds();
+
this.trigger0( 'visibility' );
}
return this;
Index: gravity-and-orbits/js/common/view/TimeCounter.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/TimeCounter.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/TimeCounter.js (date 1581770018425)
@@ -61,7 +61,7 @@
clock.timeProperty.link( this.timeListener );
this.addChild( new VBox( {
- resize: false,
+ resize: true,
spacing: 4,
children: [
dayText,
Index: gravity-and-orbits/js/common/GravityAndOrbitsConstants.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/GravityAndOrbitsConstants.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/GravityAndOrbitsConstants.js (date 1581770018450)
@@ -29,7 +29,7 @@
stroke: CONTROL_PANEL_STROKE,
lineWidth: 2,
cornerRadius: 5,
- resize: false,
+ resize: true,
xMargin: PANEL_X_MARGIN,
scale: 1.05,
fill: GravityAndOrbitsColorProfile.panelBackgroundProperty
Index: gravity-and-orbits/js/common/view/GravityControl.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/GravityControl.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/GravityControl.js (date 1581770018443)
@@ -42,7 +42,7 @@
const offTextNode = new Text( offString, TEXT_OPTIONS );
this.addChild( new HBox( {
- spacing: 10, bottom: 2, resize: false, children: [
+ spacing: 10, bottom: 2, resize: true, children: [
gravityTextNode,
new AquaRadioButton( gravityEnabledProperty, true, onTextNode, merge( { tandem: options.tandem.createTandem( 'gravityOnRadioButton' ) }, RADIO_OPTIONS ) ),
new AquaRadioButton( gravityEnabledProperty, false, offTextNode, merge( { tandem: options.tandem.createTandem( 'gravityOffRadioButton' ) }, RADIO_OPTIONS ) )
Index: scenery/js/nodes/LayoutBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- scenery/js/nodes/LayoutBox.js (revision 4ace157efd412f4f193fc86cd3503b2a5c71ac91)
+++ scenery/js/nodes/LayoutBox.js (date 1581770450895)
@@ -132,6 +132,11 @@
for ( let i = 0; i < this._children.length; i++ ) {
const child = this._children[ i ];
+
+ // TODO: copied with below
+ if ( !child.bounds.isValid() || !child.visible ) {
+ continue; // Skip children without bounds
+ }
maxWidth = Math.max( maxWidth, child.width );
maxHeight = Math.max( maxHeight, child.height );
}
@@ -161,7 +166,7 @@
let position = 0;
for ( let i = 0; i < children.length; i++ ) {
const child = children[ i ];
- if ( !child.bounds.isValid() ) {
+ if ( !child.bounds.isValid() || !child.visible ) {
continue; // Skip children without bounds
}
child[ layoutPosition ] = position;
@@ -204,6 +209,7 @@
onLayoutBoxChildInserted: function( node ) {
if ( this._resize ) {
node.onStatic( 'bounds', this._updateLayoutListener );
+ node.onStatic( 'visibility', this._updateLayoutListener );
}
},
@@ -216,6 +222,7 @@
onLayoutBoxChildRemoved: function( node ) {
if ( this._resize ) {
node.offStatic( 'bounds', this._updateLayoutListener );
+ node.offStatic( 'visibility', this._updateLayoutListener );
}
},
@@ -408,10 +415,12 @@
// If we are now resizable, we need to add listeners to every child
if ( resize ) {
child.onStatic( 'bounds', this._updateLayoutListener );
+ child.onStatic( 'visibility', this._updateLayoutListener );
}
// Otherwise we are now not resizeable, and need to remove the listeners
else {
child.offStatic( 'bounds', this._updateLayoutListener );
+ child.offStatic( 'visibility', this._updateLayoutListener );
}
}
Index: gravity-and-orbits/js/common/view/SceneSelectionControls.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- gravity-and-orbits/js/common/view/SceneSelectionControls.js (revision 6aad879ef8afe80c68539e0fa61fd050252a1c6f)
+++ gravity-and-orbits/js/common/view/SceneSelectionControls.js (date 1581770018436)
@@ -60,7 +60,7 @@
buttonContentXMargin: 5,
buttonContentYMargin: 5,
spacing: 0,
- resize: false,
+ resize: true,
deselectedOpacity: 1,
cornerRadius: 5,
touchAreaYDilation: 0, // reduce to 0 to prevent overlap between buttons
Here are just the changes for scenery: Index: js/nodes/Node.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/nodes/Node.js (revision 4ace157efd412f4f193fc86cd3503b2a5c71ac91)
+++ js/nodes/Node.js (date 1581770773896)
@@ -1115,7 +1115,11 @@
i = this._children.length;
while ( i-- ) {
- this._childBounds.includeBounds( this._children[ i ]._bounds );
+
+ // Invisible children are not included in the bounds, to facilitate relayout for PhET-iO
+ if ( this._children[ i ].visible ) {
+ this._childBounds.includeBounds( this._children[ i ]._bounds );
+ }
}
// run this before firing the event
@@ -3037,6 +3041,9 @@
// Defined in Accessibility.js
this._accessibleDisplaysInfo.onVisibilityChange( visible );
+ // When the visibility of a Node changes, signify this as a bounds change so that parents relayout
+ this.invalidateBounds();
+
this.trigger0( 'visibility' );
}
return this;
Index: js/nodes/LayoutBox.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- js/nodes/LayoutBox.js (revision 4ace157efd412f4f193fc86cd3503b2a5c71ac91)
+++ js/nodes/LayoutBox.js (date 1581770450895)
@@ -132,6 +132,11 @@
for ( let i = 0; i < this._children.length; i++ ) {
const child = this._children[ i ];
+
+ // TODO: copied with below
+ if ( !child.bounds.isValid() || !child.visible ) {
+ continue; // Skip children without bounds
+ }
maxWidth = Math.max( maxWidth, child.width );
maxHeight = Math.max( maxHeight, child.height );
}
@@ -161,7 +166,7 @@
let position = 0;
for ( let i = 0; i < children.length; i++ ) {
const child = children[ i ];
- if ( !child.bounds.isValid() ) {
+ if ( !child.bounds.isValid() || !child.visible ) {
continue; // Skip children without bounds
}
child[ layoutPosition ] = position;
@@ -204,6 +209,7 @@
onLayoutBoxChildInserted: function( node ) {
if ( this._resize ) {
node.onStatic( 'bounds', this._updateLayoutListener );
+ node.onStatic( 'visibility', this._updateLayoutListener );
}
},
@@ -216,6 +222,7 @@
onLayoutBoxChildRemoved: function( node ) {
if ( this._resize ) {
node.offStatic( 'bounds', this._updateLayoutListener );
+ node.offStatic( 'visibility', this._updateLayoutListener );
}
},
@@ -408,10 +415,12 @@
// If we are now resizable, we need to add listeners to every child
if ( resize ) {
child.onStatic( 'bounds', this._updateLayoutListener );
+ child.onStatic( 'visibility', this._updateLayoutListener );
}
// Otherwise we are now not resizeable, and need to remove the listeners
else {
child.offStatic( 'bounds', this._updateLayoutListener );
+ child.offStatic( 'visibility', this._updateLayoutListener );
}
}The next step for this issue is to discuss with @jonathanolson to determine the practicality of this approach, will it cause layout problems or performance problems? Is there a better way to accomplish this goal? What effort will be necessary to apply this pattern across our sims? I'll reach out to @jonathanolson to schedule a meeting. |
|
A note that @jonathanolson also has nice working copy changes for sun and gravity-and-orbits. |
|
It looks like some sims might have minor layout differences with the scenery patch. Should we add checks/assertions to see where nodes are invisible currently in layout containers? Also for developer discussion: is it ok if layout containers (LayoutBox in particular) both does NOT layout invisible nodes, AND does not include their bounds? This seems like a general improvement to me. |
|
I added this assertion: isChildIncludedInLayout: function( child ) {
assert && assert( child instanceof Node );
if ( this._excludeInvisibleChildrenFromBounds && !child.visible ) {
assert && assert( false, 'invisible child in layout container' );
}and got reports for these sims: gravity-and-orbits is expected, @jonathanolson and I added code to that repo to leverage this new layout feature. |
|
I compared the control panels in Waves Intro. Top is before the scenery patch, bottom is after the scenery patch:
In the "after", the tools in the top seem more centered and the sliders seem a little larger. Here's a gif that might make it easier to visualize:
I haven't visualized the other differences, but this one seems like an improvement. |
…eInvisibleChildrenFromBounds, and was failing in the playground), see phetsims/joist#608
It has "empty" bounds, which is what I believe is correct and desired in this case. Ideally we should be able to handle cases like this in parents (as layoutbox/etc. should), although I can now imagine a lot of code that would break in these cases (e.g. manually positioning a layout box)
Panel treats it as zero-size, but AccordionBox and buttons fail out with empty bounds, and it looks like ComboBoxListBox will fail out (likely). Sounds worthy of dev discussion about what to do for sure.
AccordionBox with resize:true would likely fail. Most other failure options based on sun classes would only fail on startup (as they don't watch bounds). Most manual positional code would fail (trying to position something with empty bounds based on This seems like a problem, tagging for dev meeting. @samreid thoughts about ways to resolve this? |
…undsDirty with our call, so child bounds were not being correctly recomputed in some cases. See phetsims/sun#588, phetsims/joist#608
|
Applied a bug fix above, where visibility changes were NOT properly forcing childBounds recomputations in some cases on parents. |
|
And assuming this feature is good to keep, we can probably remove the |
|
I was curious about how call sites needing Chris Malley 10:08 AM Jonathan Olson 10:12 AM Jonathan Olson 10:13 AM Chris Malley 10:13 AM |
|
After the issues in #608 (comment) are resolved, the next step is probably to identify other UI components that should optionally resize based on the bounds of visible children, and add Off the top of my head:
|
|
In case responsible devs want to inspect uses of occurrences of excludeInvisibleChildrenFromBounds: false |
…to validate and compute bounds on access) with other quality of life improvements, see phetsims/scenery#1044 and phetsims/joist#608
…to validate and compute bounds on access) with other quality of life improvements, see #1044 and phetsims/joist#608
|
From dev meeting on 04/30/20: Regarding #608 (comment) |
|
Aqua tests are passing after I've made positional modifiers no-op on invalid bounds, AND adjusted Panel/AccordionBox to themselves have empty bounds when their contents bounds are empty. Tagging for dev meeting for further discussion. |
|
5/14/2020 dev meeting: This is looking good. We will address more containers/layouts as they come up. Closing. |
…invalid content), see phetsims/joist#608
I mentioned this in Slack on Friday Feb 7:
We have been struggling with how to reflow the UI when the layout of something changes, or when items are added or removed, or change size/contents. We haven’t taken the plunge into LayoutManagers yet, and I question the added value of going all in for LayoutManagers because the strategy we have been using for positioning things relative to each other has been very successful--not complicated, we get full control, no unexpected behavior, etc. But what about consolidating layout code in a new function that can be called dynamically at runtime?
current pattern
proposed pattern
Then we could call
component.layout()orscreenView.layout()when something is added/removed/resized.The text was updated successfully, but these errors were encountered: