Carousel should relayout contents and repaginate when items are removed/restored

samreid commented 1 year ago

In https://github.com/phetsims/circuit-construction-kit-common/issues/630 it was requested that the carousel not leave empty layout holes when circuit elements are removed via visibleProperty = false in PhET-iO. The implementation could also support non-phet-io use cases, like adding/removing the non-ohmic bulb in CCK DC (Lab screen, Advanced, Add Real Bulbs). Sim-specific details are described in https://github.com/phetsims/circuit-construction-kit-common/issues/630. Summarizing the requests:

When an item visibleProperty becomes false, relayout the items on that page so there is no gap. Items from the following page move to the page.
If a page becomes empty, delete the page (note this would change the number of dots in the PageControl). If the Carousel was looking at the page that became empty, scroll back to the previous page (if there is one).

Circuit Construction Kit: DC: PhET-iO has the additional complexities:

We would like a Wire to be shown at the top of each page.
We need to distinguish between when an icon is removed from the carousel via phet-io (so that users cannot create elements of that type) vs. when the user is allowed to create elements of that type, but they have used up all of them so the icon becomes hidden (which should leave a layout hole). This relates to https://github.com/phetsims/circuit-construction-kit-common/issues/903. We normally implement this kind of behavior in PhET-iO by adding a level of nesting. i.e. the Carousel's Node visibleProperty determines whether the node is included in the layout, and it contains a child node that can be hidden if all circuit elements of that type are depleted.

As far as I understand from https://github.com/phetsims/circuit-construction-kit-common/issues/630, this feature is requested for the upcoming milestone release of CCK DC PhET-iO.

@pixelzoom can you please advise how to proceed, and help us estimate the time/complexity if we implement this?

pixelzoom commented 1 year ago

We might be able to hack our way around to make the current implementation do this, but I don't advise going down that path. Carousel and PageControl were not designed to support this behavior, and should be leveraging new scenery layout features to do so.

So my recommendation is:

Rewrite Carousel so that all items are in a Flowbox, separated with HSeparator or VSeparator, clipped to the Carousel's viewport.
As the visibility of items changes, adjust the position of the Flowbox within the viewport.
Rewrite PageControl as needed.

This will be a complex rewrite, and will take considerable time (20-30 hours optimistically?)

I don't know the relative priority of this. But my advice is put this on a wish list, defer it for now, and live with the "holes" until higher-priority developement work and goals are accomplished.

samreid commented 1 year ago

Some ideas and prototypes being developed in https://github.com/phetsims/circuit-construction-kit-common/issues/630

samreid commented 1 year ago

For https://github.com/phetsims/circuit-construction-kit-common/issues/630, there is a minimally invasive working prototype that has the desired behavior. I tested the behavior of function builder and the sun carousel test, and it doesn't seem disrupted. I'd like to commit and request review.

UPDATE: I'm not fully familiar with Function Builder, so I'm not sure if I'm testing all the functionality.

pixelzoom commented 1 year ago

... I'd like to commit and request review.

You've added CarouselOptions.isScrollingNodeLayoutBox, which seems like a workaround. There's no reason for this option, it's the behavior that all Carousel's should exhibit. Or am I missing something? Or is it temporary?

isScrollingNodeLayoutBox defaults to false, and I don't see it used in any sims. (I'm fully pulled.) Please describe how to test it.

Where have you addressed PageControl? Does it work correctly when the number of pages in the carousel has changed?

pixelzoom commented 1 year ago

Taking isScrollingNodeLayoutBox for a test-drive in function-builder, the patch below causes this assertion failure on startup:

assert.js:28 Uncaught Error: Assertion failed: itemIndex out of range: -1 at window.assertions.assertFunction (assert.js:28:13) at Carousel.itemIndexToPageNumber (Carousel.ts:485:15) at Carousel.scrollToItemIndex (Carousel.ts:458:39) at Carousel.scrollToItem (Carousel.ts:469:10) at getCarouselPosition (SceneNode.js? [sm]:449:12) at SceneNode.js? [sm]:352:44 at Array.forEach () at PatternsSceneNode.populateFunctionCarousels (SceneNode.js? [sm]:349:33) at PatternsSceneNode.completeInitialization (SceneNode.js? [sm]:340:10) at FBScreenView.js? [sm]:91:30

Patch

```diff Index: js/common/view/SceneNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/SceneNode.js b/js/common/view/SceneNode.js --- a/js/common/view/SceneNode.js (revision cccd761124fef78429a8fc8ba28577133f648ed3) +++ b/js/common/view/SceneNode.js (date 1670516391067) @@ -174,7 +174,8 @@ buttonTouchAreaXDilation: 15, buttonTouchAreaYDilation: 5, centerX: layoutBounds.centerX, - bottom: layoutBounds.bottom - 25 + bottom: layoutBounds.bottom - 25, + isScrollingNodeLayoutBox: true } ); // Page control for function carousel @@ -186,6 +187,11 @@ }, PAGE_CONTROL_OPTIONS ) ); controlsLayer.addChild( functionPageControl ); + // Hide some items in functionCarousel. + for ( let i = 0; i < 7; i++ ) { + functionContainers[ i ].visible = false; + } + //------------------------------------------------------------------------------------------------------------------ // Link the input and output carousels, so that they display the same page number. ```

pixelzoom commented 1 year ago

Since what's in master is now buggy, this issue is blocking until @samreid's prototype is removed or completed.

pixelzoom commented 1 year ago

Here's my recommendation:

Revert https://github.com/phetsims/sun/commit/fc9df1de5cb8513c26889e12f89b9b77c86bbb7f. This prototype does not address significant parts of the functionality of Carousel (and PageControl!), and in fact causes (failing) problems.
Defer changes to Carousel (and PageControl!) until a future date. This feature needs to be discussed, scoped, prioritized, scheduled, and (most importantly) done correctly. Prototyping before discussing and prioritizing feels premature.
Defer the CCK request in https://github.com/phetsims/circuit-construction-kit-common/issues/630 until a client actually asks for it. PhET spends too much time working on features that no one has asked for, while things that need to be completed are not.

samreid commented 1 year ago

Since what's in master is now buggy,

This is incorrect. The reason the patch above fails on startup is that function builder is telling the carousel to scroll to an item that is not displayed in the carousel.

This patch avoids scrolling to a non-displayed object, and starts up correctly:

```diff Subject: [PATCH] Add SimVersion for use in MigrationEngine, see https://github.com/phetsims/phet-io/issues/1899 --- Index: js/common/view/SceneNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/SceneNode.js b/js/common/view/SceneNode.js --- a/js/common/view/SceneNode.js (revision cccd761124fef78429a8fc8ba28577133f648ed3) +++ b/js/common/view/SceneNode.js (date 1670955288130) @@ -174,7 +174,8 @@ buttonTouchAreaXDilation: 15, buttonTouchAreaYDilation: 5, centerX: layoutBounds.centerX, - bottom: layoutBounds.bottom - 25 + bottom: layoutBounds.bottom - 25, + isScrollingNodeLayoutBox: true } ); // Page control for function carousel @@ -186,6 +187,11 @@ }, PAGE_CONTROL_OPTIONS ) ); controlsLayer.addChild( functionPageControl ); + // Hide some items in functionCarousel. + for ( let i = 0; i < functionContainers.length; i++ ) { + functionContainers[ i ].visible = false; + } + //------------------------------------------------------------------------------------------------------------------ // Link the input and output carousels, so that they display the same page number. @@ -440,7 +446,10 @@ */ function getCarouselPosition( carousel, container, worldParent ) { assert && assert( !carousel.animationEnabled ); - carousel.scrollToItem( container ); + + if ( container.visible ) { + carousel.scrollToItem( container ); + } return worldParent.globalToLocalPoint( container.parentToGlobalPoint( container.center ) ); } ```

Here is a patch that alternatively hides/shows the first item using setInterval:

```diff Subject: [PATCH] Add SimVersion for use in MigrationEngine, see https://github.com/phetsims/phet-io/issues/1899 --- Index: js/common/view/SceneNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/SceneNode.js b/js/common/view/SceneNode.js --- a/js/common/view/SceneNode.js (revision cccd761124fef78429a8fc8ba28577133f648ed3) +++ b/js/common/view/SceneNode.js (date 1670955361312) @@ -174,7 +174,8 @@ buttonTouchAreaXDilation: 15, buttonTouchAreaYDilation: 5, centerX: layoutBounds.centerX, - bottom: layoutBounds.bottom - 25 + bottom: layoutBounds.bottom - 25, + isScrollingNodeLayoutBox: true } ); // Page control for function carousel @@ -186,6 +187,10 @@ }, PAGE_CONTROL_OPTIONS ) ); controlsLayer.addChild( functionPageControl ); + setInterval( () => { + functionContainers[ 0 ].visible = !functionContainers[ 0 ].visible; + }, 1000 ); + //------------------------------------------------------------------------------------------------------------------ // Link the input and output carousels, so that they display the same page number. @@ -440,7 +445,10 @@ */ function getCarouselPosition( carousel, container, worldParent ) { assert && assert( !carousel.animationEnabled ); - carousel.scrollToItem( container ); + + if ( container.visible ) { + carousel.scrollToItem( container ); + } return worldParent.globalToLocalPoint( container.parentToGlobalPoint( container.center ) ); } ```

There is a layout overlap problem, where carousel items are cut off though:

pixelzoom commented 1 year ago

Since what's in master is now buggy,

This is incorrect. The reason the patch above fails on startup is that function builder is telling the carousel to scroll to an item that is not displayed in the carousel.

You changed the implementation such that Carousel crashes when one of it's supported features is used. How is that not a bug?

samreid commented 1 year ago

There's no reason for this option, it's the behavior that all Carousel's should exhibit. Or am I missing something? Or is it temporary?

This can be opt-in for now until it is well-established, then the option can be removed in the future.

pixelzoom commented 1 year ago

And your short-term plan for addressing PageControl is ...?

samreid commented 1 year ago

You changed the implementation such that Carousel crashes when one of it's supported features is used.

Is one of Carousel's supported features scrolling to an object that is not displayed?

And your short-term plan for addressing PageControl is ...?

A plan for that has not yet been developed.

pixelzoom commented 1 year ago

My recommendation is to remove this hack/bandaid from Carousel, and live with "holes" in the CCK release until we have resources to properly implement this feature for Carousel and PageControl. If you want to proceed with the hack/bandaid, then I'd prefer not to be involved with Carousel and PageControl.

samreid commented 1 year ago

Partial progress towards a more long-term implementation for Carousel:

```diff Subject: [PATCH] Add SimVersion for use in MigrationEngine, see https://github.com/phetsims/phet-io/issues/1899 --- Index: js/Carousel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/Carousel.ts b/js/Carousel.ts --- a/js/Carousel.ts (revision 0ef58b768ccbe524b85b53b8e8602ce1c04805fd) +++ b/js/Carousel.ts (date 1670969100903) @@ -211,7 +211,6 @@ const scrollingLength = ( items.length * ( maxItemLength + options.spacing ) + ( numberOfSeparators * options.spacing ) + options.spacing ); const scrollingWidth = isHorizontal ? scrollingLength : ( maxItemWidth + 2 * options.margin ); const scrollingHeight = isHorizontal ? ( maxItemHeight + 2 * options.margin ) : scrollingLength; - let itemCenter = options.spacing + ( maxItemLength / 2 ); // Options common to all separators const separatorOptions = { @@ -227,61 +226,38 @@ // All items, arranged in the proper orientation, with margins and spacing. // Horizontal carousel arrange items left-to-right, vertical is top-to-bottom. // Translation of this node will be animated to give the effect of scrolling through the items. - const scrollingNode = options.isScrollingNodeLayoutBox ? - ( isHorizontal ? new HBox( { - spacing: options.spacing, - yMargin: options.margin - } ) : new VBox( { - spacing: options.spacing, - xMargin: options.margin - } ) ) : - new Rectangle( 0, 0, scrollingWidth, scrollingHeight ); + const scrollingNode = ( isHorizontal ? new HBox( { + spacing: options.spacing, + yMargin: options.margin + } ) : new VBox( { + spacing: options.spacing, + xMargin: options.margin + } ) ); this.isScrollingNodeLayoutBox = options.isScrollingNodeLayoutBox; items.forEach( item => { - // add the item - if ( isHorizontal ) { - item.centerX = itemCenter; - item.centerY = options.margin + ( maxItemHeight / 2 ); - } - else { - item.centerX = options.margin + ( maxItemWidth / 2 ); - item.centerY = itemCenter; - } scrollingNode.addChild( item ); - // center for the next item - itemCenter += ( options.spacing + maxItemLength ); - // add optional separator if ( options.separatorsVisible ) { - let separator; if ( isHorizontal ) { // vertical separator, to the left of the item - separator = new VSeparator( combineOptions( { + scrollingNode.addChild( new VSeparator( combineOptions( { preferredHeight: scrollingHeight, centerX: item.centerX + ( maxItemLength / 2 ) + options.spacing, centerY: item.centerY - }, separatorOptions ) ); - scrollingNode.addChild( separator ); - - // center for the next item - itemCenter = separator.centerX + options.spacing + ( maxItemLength / 2 ); + }, separatorOptions ) ) ); } else { // horizontal separator, below the item - separator = new HSeparator( combineOptions( { + scrollingNode.addChild( new HSeparator( combineOptions( { preferredWidth: scrollingWidth, centerX: item.centerX, centerY: item.centerY + ( maxItemLength / 2 ) + options.spacing - }, separatorOptions ) ); - scrollingNode.addChild( separator ); - - // center for the next item - itemCenter = separator.centerY + options.spacing + ( maxItemLength / 2 ); + }, separatorOptions ) ) ); } } } ); ```

I'm continuing to work on this since even after hearing the 20+ hours time estimate, the designer said:

My personal preference would be to invest the time into carousel because it's not going away, but I cannot make this call. I added the sun issue to the PhET-iO project board for further discussion.

samreid commented 1 year ago

In https://github.com/phetsims/circuit-construction-kit-common/issues/630, we determined this feature is critical to one of this month's high priority projects, and @jbphet, @matthew-blackman and I worked on an implementation that follows @pixelzoom's recommendations in https://github.com/phetsims/sun/issues/814#issuecomment-1334463348. We have been working in patches, but we would like to move to branches to make it possible to commit progress and coordinate. We would also like to request review from @pixelzoom review once @jbphet @matthew-blackman and I are ready (hopefully in the next few business days). So I'll start creating branches.

samreid commented 1 year ago

After the above commits, we observed in the LanguageSelectionNode that the highlights change the size of items in the carousel and it makes the layout jump around. We recommend that the highlighted items should have the same size as regular items and will commit that in a moment. Heads up to @jessegreenberg if you want to review that commit (note it is in a branch).

samreid commented 1 year ago

In the next commit (coming up after this comment), we saw that number suite common was doing a lot of extra work to put the items in pages. But now that is handled by the Carousel, so I'll commit that too... Heads up to @chrisklus since he is responsible dev for that repo.

samreid commented 1 year ago

For reference, here are the repos with branches so far:

build-a-molecule
circuit-construction-kit-common
function-builder
joist
number-line-operations
number-suite-common
sun

samreid commented 1 year ago

I think the majority of the above commits are from merging master. I also added the list to perennial so we can use commands using for-each.sh carousel-dynamic.

samreid commented 1 year ago

Notes from discussion with @zepumph:

Use group item node options for carousel items, like we do for vertical aqua radio button group, etc. Then carousel can be in charge of the IndexedNodeIO part, like ComboBox is.
containerDepth is an implementation detail which will not appear in the API
Add caching
Should we add our own serialization for carousel, instead of relying on IndexedNodeIO. It can have its own move up/move down implementation without having to use IndexedNodeIO. This would allow use to use VSeparator and HSeparator, by just subtracting them out of the indices at serialization time.
After doing this, we won't need IndexedNodeIO to be parametric.... maybe?
SR: At what point should we involve @pixelzoom?

MK: Changes for carousel instrumentation should happen now since it is our first instrumented sim with carousels.

Should carousel items have metadata like this? We should be using GroupItemOptions plus more??

    // Describes one radio button
    export type RectangularRadioButtonGroupItem<T> = {
      value: T; // value associated with the button
      label?: Node; // optional label that appears outside the button
      phetioDocumentation?: string; // optional documentation for PhET-iO
      labelContent?: PDOMValueType; // optional label for a11y (description and voicing)
      voicingContextResponse?: VoicingResponse;
      descriptionContent?: PDOMValueType; // optional label for a11y
      options?: StrictOmit<RectangularRadioButtonOptions, 'tandem'>; // options passed to RectangularRadioButton constructor
    } & GroupItemOptions;

We think this is the right time for doing that.

PhET-iO Designed Features:

Hide an item, and the layout reflows (but still have support for some cases where a hole is supposed to be there)
Change the order of things by move up/move down.

samreid commented 1 year ago

@matthew-blackman and I would like to commit to the branches, and to bring in the work from https://github.com/phetsims/circuit-construction-kit-common/issues/923 into the branches (to avoid patches on top of branches or branches on top of branches). Some questions about this commit:

Spacing and margins are adjusted in this commit, but we aren't sure why it is necessary. Also, after implementing CarouselIO for move up/move down, it may free us up to use VSeparator/HSeparator and the spacing and margins may resolve naturally.
Also, after implementing CarouselIO for move up/move down, it may free us up to use VSeparator/HSeparator. That will also allow them to take up space in the layout (which they cannot in this commit).
Also, the Function Builder layout is off: (may be related to setTimeout?? probably not)

samreid commented 1 year ago

Those commits (all in branches) are working for about half of the repos that use Carousel. I didn't want to leave partial progress in patches.

@zepumph @matthew-blackman and I discussed about whether it is OK to commit improvements that may not run or may have lint/type errors to branches. @marlitas also advocated for this previously. We think we should be able to do so. We think it might interfere with our ability to bisect, but we aren't sure about that and think it could be worth it anyways.

samreid commented 1 year ago

I started working on CarouselIO and having it report the indices of its items. It's unclear whether the pattern:

autoselect an item
press "move up" and "move down" on the item

Is ideal, or whether that was just convenient and where we ended up. How difficult would it be to simulate that same UI if all the state is in the CarouselIO?

samreid commented 1 year ago

@zepumph @matthew-blackman and I are making good progress. Next steps we are aware of:

[x] Other sims need to use GroupItemOptions
[x] Carousel has TODOs
[x] ~How do we want to deal with separators? We considered the complexity of adding separators into the indexing order, and we think it will be too complex. Or can be added in the future if necessary.~
[x] Support for some cases where a hole is supposed to be there? (like the last item was dragged out?)
[x] Pixel polishing and testing all Carousel sims
[ ] Code review
[ ] Merge to master
[ ] Test if the "broken age" of code in the branches breaks the ability to bisect on master

Sims to check:

[x] area builder: Margins don't match exactly, but @zepumph and @samreid both think it looks good
[x] balancing-act: The button styling has changed since the very old lavender version.
[x] build a molecule
[x] circuit construction kit ac, ac-virtual-lab, dc, dc-virtual lab
[x] expression-exchange
[x] function-builder
[x] function-builder-basics
[x] interaction-dashboard (@zepumph )
[x] number-line-operations
[x] number-play
[x] number-compare
[x] sun

zepumph commented 1 year ago

Support for some cases where a hole is supposed to be there? (like the last item was dragged out?)

If you set the visibility of the alignBox with Carousel.setItemVisibility (implemented in https://github.com/phetsims/sun/commit/b2232c6a547088924e5aa6d1fdefeefb7fd90c8a), then the Carousel will reflow, and gaps will collapse, but if you set the visibility of the Node returned in the createNode function (this is on your own, no API support), then there will remain a hole in the Carousel. Yay!!!

zepumph commented 1 year ago

@samreid and I were able to update all usages of Carousel to the new pattern on the carousel-dynamic branches. We did some pixel polishing comparing to the published versions when possible. The only worrisome usage we decided to punt on is in expression exchange. There we now have a carousel that is clearly not centers. We didn't understand exactly why, but the offset in https://github.com/phetsims/expression-exchange/blob/798a1103cc249fba3b6e168f8a43c7288e46ce8f/js/common/view/CoinTermCreatorNode.js#L73-L74 seemed suspicious.

samreid commented 1 year ago

Oops, that greenhouse effect commit didn't have anything to do with this issue. Just a random lint fix though.

samreid commented 1 year ago

I addressed the expression exchange centering issue and spot checked that other sims seem ok.

samreid commented 1 year ago

Issues discovered during review with @arouinfar and @matthew-blackman:

[x] Carousel buttons do not come back when 2nd page is added back
[x] Increase margins for CCK
[x] Move forward/Move back should only respect visible indices. See if it can be done in less than 2.5-3 hours??
[x] The white empty page control dot is smaller than the filled in page control dots, so the layout readjusts when it goes down to a single dot.

zepumph commented 1 year ago

Increase margins for CCK

I believe increased margins were desired for every single Carousel we looked at in the last 24 hours. Should we change the default back to something larger like it was last week?

samreid commented 1 year ago

Here's a patch that skips invisible children for move forward/back,

```diff Subject: [PATCH] Make selected dot the same width as unselected dots, see https://github.com/phetsims/sun/issues/814 --- Index: js/nodes/Node.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/nodes/Node.ts b/js/nodes/Node.ts --- a/js/nodes/Node.ts (revision 1d3aceb20c9b7ad0dd1b5f872036237b50886533) +++ b/js/nodes/Node.ts (date 1673469543093) @@ -1228,6 +1228,14 @@ return this; // chaining } + /** + * Move this node one index forward in each of its parents. If the Node is already at the front, this is a no-op. + */ + public moveForwardVisible(): this { + this._parents.forEach( parent => parent.moveChildForwardVisible( this ) ); // TODO: Do we need slice like moveToFront has? + return this; // chaining + } + /** * Moves the specified child forward by one index. If the child is already at the front, this is a no-op. */ @@ -1239,6 +1247,25 @@ return this; // chaining } + /** + * Moves the specified child forward by one index. If the child is already at the front, this is a no-op. + */ + public moveChildForwardVisible( child: Node ): this { + const index = this.indexOfChild( child ); + + let targetIndex = index + 1; + + // skip invisible children + while ( targetIndex < this.children.length && !this.children[ targetIndex ].visible ) { + targetIndex++; + } + + if ( targetIndex < this.children.length ) { + this.moveChildToIndex( child, targetIndex ); + } + return this; // chaining + } + /** * Move this node one index backward in each of its parents. If the Node is already at the back, this is a no-op. */ @@ -1247,6 +1274,14 @@ return this; // chaining } + /** + * Move this node one index backward in each of its parents. If the Node is already at the back, this is a no-op. + */ + public moveBackwardVisible(): this { + this._parents.forEach( parent => parent.moveChildBackwardVisible( this ) ); // TODO: Do we need slice like moveToFront has? + return this; // chaining + } + /** * Moves the specified child forward by one index. If the child is already at the back, this is a no-op. */ @@ -1257,6 +1292,25 @@ } return this; // chaining } + + /** + * Moves the specified child forward by one index. If the child is already at the back, this is a no-op. + */ + public moveChildBackwardVisible( child: Node ): this { + const index = this.indexOfChild( child ); + + let targetIndex = index - 1; + + // skip invisible children + while ( targetIndex > 0 && !this.children[ targetIndex ].visible ) { + targetIndex--; + } + + if ( targetIndex >= 0 ) { + this.moveChildToIndex( child, targetIndex ); + } + return this; // chaining + } /** * Moves this Node to the back (front) of all of its parents children array. Index: js/nodes/IndexedNodeIO.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/nodes/IndexedNodeIO.ts b/js/nodes/IndexedNodeIO.ts --- a/js/nodes/IndexedNodeIO.ts (revision 1d3aceb20c9b7ad0dd1b5f872036237b50886533) +++ b/js/nodes/IndexedNodeIO.ts (date 1673468669296) @@ -98,7 +98,7 @@ returnType: VoidIO, parameterTypes: [], implementation: function( this: Node ) { - return this.moveForward(); + return this.moveForwardVisible(); }, documentation: 'Move this node one index forward in each of its parents. If the node is already at the front, this is a no-op.' }, @@ -107,7 +107,7 @@ returnType: VoidIO, parameterTypes: [], implementation: function( this: Node ) { - return this.moveBackward(); + return this.moveBackwardVisible(); }, documentation: 'Move this node one index backward in each of its parents. If the node is already at the back, this is a no-op.' } ```

samreid commented 1 year ago

That change does not belong in scenery Node.ts, I'll move it to IndexedNodeIO in a forthcoming commit.

samreid commented 1 year ago

Instructions for Carousel code review. Please note all code for the review is developed in branches, so you will not see the changes or new features until you check out the branches.

Major features

Carousel layout is dynamic -- if one item is removed via Studio, the layout adjusts instead of leaving a hole. This can be demonstrated by launching Circuit Construction Kit: DC in Studio, autoselecting a circuit element tool, and changing visibleProperty such as circuitConstructionKitDc.introScreen.view.circuitElementToolbox.carousel.items.resistorToolNode.visibleProperty to false. The layout should adjust. Pressing "Test" should preserve the visibilities and layout in the launched sim. Removing items can remove pages, and PageControl automatically updates. If the user is on the last page when its last item is removed, it automatically moves to the previous page. A Carousel always has at least one page, even if there are no items.
Carousel items can move up/move down in Studio. This can be demonstrated by launching Circuit Construction Kit: DC in Studio, autoselecting a circuit element tool such as circuitConstructionKitDc.introScreen.view.circuitElementToolbox.carousel.items.resistorToolNode and pressing "move up" or "move down" on it. Pressing "Test" should preserve the visibilities and layout in the launched sim.
Hiding the light bulb via circuitConstructionKitDc.introScreen.view.circuitElementToolbox.carousel.items.lightBulbToolNode.visibleProperty then moving the resistor circuitConstructionKitDc.introScreen.view.circuitElementToolbox.carousel.items.resistorToolNode up one time used to have no effect, because it was moving the resistor past the invisible light bulb. We changed it so that pressing "move up" or "move down" ignores invisible neighbors for purposes of indexing. That change is in IndexedNodeIO and should be cherry picked into ph-scale and ph-scale basics for https://github.com/phetsims/ph-scale/issues/266. The solution works for Carousel and ComboBox.
Carousel now enforces the same-sizedness of its elements via a nested AlignBox.
Separators are no longer part of the traversal order in the same parent as the item nodes so that IndexedNodeIO can do move up/move down. Separators are automatically created/destroyed when items are made visible/invisible.
Carousel now uses GroupItemOptions and createNode + tandemName. This led to a little work in Function Builder and other sims that need to reference the actual created Nodes. The Carousel has the 3 levels of detail:

  // Items hold the data to create the carouselItemNode
  private readonly items: CarouselItem[];

  // each AlignBox holds a carouselItemNode and ensures proper sizing in the Carousel
  private readonly alignBoxes: AlignBox[];

  // created from createNode() in CarouselItem
  public readonly carouselItemNodes: Node[];

Disposal. Carousels create their items via createNode, and hence can dispose the created nodes.
Some sims (like Area Builder) duplicated code to show a panel and a carousel. The carousel now hides the previous/next buttons if it is a one-page carousel. This allows us to avoid duplicated code and continue using Carousel in cases like that.
Removed the option to "hide carousel buttons when they are disabled" because it did not have a consistent look and feel across sims, and because where it was used, it felt unbalanced/asymmetrical. Note that this looks odd for carousel combo box that has a small number of items. Our understanding was that is OK--this is not yet a production component. That case looks like this:
In reviewing all of the sims that use Carousel, we checked that they looked good and reasonable, and we did not spend undue effort trying to make everything identical with what's in master or on the PhET website.

About the branches

The branch name is "carousel-dynamic"
All of the branched repos are listed in perennial/data/carousel-dynamic
You can check them out via:

cd {{directory where all sims are checked out}}/
for-each.sh carousel-dynamic git checkout carousel-dynamic
for-each.sh carousel-dynamic git pull

master has been merged to carousel-dynamic recently, to avoid too much conflict
Once you have checked out the code, you can run tests like above.
We decided to work in branches so that we can make commits and progress without disrupting master
There were a temporary times in the branches where some sims were not running or linting or type checking. We think this is OK and we want to see if it disrupts bisecting by date once merged to master. We do not want to squash any commits.
One of the overheads of this process was creating 14 branch issues. That did not feel like a valuable use of time, since each branch has a commit referencing this issue (so we had a reasonable paper trail), and the branches and issues are meant to be transient anyways. If we use this strategy again, perhaps we will prefer not to spend time creating branch issues.
I'm being somewhat verbose about the branching strategy used here in case we use it more in the project. It was a pretty good development experience, and we would not have been able to make this amount of progress in master.
After review recommendations are addressed, we would like to merge the changes into master, delete the branches, close the branch issues, and remove the perennial/data/carousel-dynamic list.

Scope of review

Basically, any file that has changed in the branches is in the scope of review. For each repo, you can compare the branch to master using WebStorm via: Git -> Branches -> {{choose a repo}} -> Right pullout arrow -> master -> Show diff with working tree. That will collapse all of the changes and show how it differs from master. If desired, you can also select master -> "Compare with carousel-dynamic" and you can see each commit in the branch. Specific files and areas to look out for:
Carousel.ts
IndexedNodeIO.ts
PageControl.ts
Function Builder and Expression Exchange had more complex usage sites for the carousel/items/nodes.
Note that Geometric Optics had a commit because the API of IndexedNodeIO changed, but then it was changed back. So if you show commits you will see that change and revert. But if you compare repo contents, you will see no changes.

Timeline

This simulation is one of the subteam's priorities for January. This is one of the last large issues for that milestone, but even if the review was done quickly, it looks like it would still be a while before QA would test the sim.
We have scheduled a kick-off meeting to review these instructions and to answer questions.
Please feel free to make commits in the branches. Please feel free to fix anything you want to.
I know there's a lot to review, so please feel free to invite other to assist, or to do any part of the review synchronously over zoom with some of the authors, so we can answer questions as they come up.
(as mentioned above in the branches section) After review recommendations are addressed, we would like to merge the changes into master, delete the branches, close the branch issues, and remove the perennial/data/carousel-dynamic list.

samreid commented 1 year ago

In discussion with me, @pixelzoom @zepumph and @matthew-blackman, we recommend scheduling a different reviewer. While it is true that @pixelzoom wrote the initial version of Carousel, this is mostly a rewrite (Carousel 2.0) with a similar API. Also @pixelzoom would do a good job reviewing it. But after seeing the scope, @pixelzoom advised that it would be about 2 full days and he would need to push everything else back (including higher priority things). Therefore we (all 4 of us) concluded we would like to request another reviewer for now. This will help us move out of the branches (prevent divergence) and move forward with Circuit Construction Kit. Once @pixelzoom's higher priorities have resolved, he can take a 2nd look if necessary. So we would like to reach out and request another developer for this review. Instructions are listed in https://github.com/phetsims/sun/issues/814#issuecomment-1379727179. If there is not an obvious reviewer from #pistachio that would be appropriate, we can schedule one within #boba-phet. Perhaps @jbphet once his publications are underway.

UPDATE: @kathy-phet said:

@jonathanolson would be a possible reviewer for this once the GA deployments are taken care of. @jbphet will have a11y work to focus on, so isn't as available.

And @jonathanolson agreed to review. I met with him to discuss the instructions above.

jonathanolson commented 1 year ago

Looks like things will need to be merged or otherwise to get TS happy, so I'm noting a patch here of my current review progress:

```diff Subject: [PATCH] Carousel review up to 2023-01-23, see https://github.com/phetsims/sun/issues/814 --- Index: build-a-molecule/js/common/view/KitPanel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/build-a-molecule/js/common/view/KitPanel.js b/build-a-molecule/js/common/view/KitPanel.js --- a/build-a-molecule/js/common/view/KitPanel.js (revision bf9e7161624a17f2c502028aceeff0f1277344b0) +++ b/build-a-molecule/js/common/view/KitPanel.js (date 1674503382025) @@ -60,7 +60,9 @@ fill: BAMConstants.KIT_BACKGROUND, stroke: BAMConstants.KIT_BORDER, itemsPerPage: 1, - buttonSoundPlayer: nullSoundPlayer + buttonOptions: { + soundPlayer: nullSoundPlayer + } } ); // When the page number changes update the current collection. Index: sun/js/demo/components/demoCarousel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/demo/components/demoCarousel.ts b/sun/js/demo/components/demoCarousel.ts --- a/sun/js/demo/components/demoCarousel.ts (revision 361897928bfaff44a9ec2e80a7798417ab570436) +++ b/sun/js/demo/components/demoCarousel.ts (date 1674500346121) @@ -27,20 +27,24 @@ const vCarousel = new Carousel( vItems, { orientation: 'vertical', separatorsVisible: true, - buttonTouchAreaXDilation: 5, - buttonTouchAreaYDilation: 15, - buttonMouseAreaXDilation: 2, - buttonMouseAreaYDilation: 7 + buttonOptions: { + touchAreaXDilation: 5, + touchAreaYDilation: 15, + mouseAreaXDilation: 2, + mouseAreaYDilation: 7 + } } ); // horizontal carousel const hCarousel = new Carousel( hItems, { orientation: 'horizontal', separatorsVisible: true, - buttonTouchAreaXDilation: 15, - buttonTouchAreaYDilation: 5, - buttonMouseAreaXDilation: 7, - buttonMouseAreaYDilation: 2, + buttonOptions: { + touchAreaXDilation: 15, + touchAreaYDilation: 5, + mouseAreaXDilation: 7, + mouseAreaYDilation: 2 + }, centerX: vCarousel.centerX, top: vCarousel.bottom + 50 } ); Index: sun/js/CarouselComboBox.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/CarouselComboBox.ts b/sun/js/CarouselComboBox.ts --- a/sun/js/CarouselComboBox.ts (revision 361897928bfaff44a9ec2e80a7798417ab570436) +++ b/sun/js/CarouselComboBox.ts (date 1674503302762) @@ -68,7 +68,9 @@ }, carouselOptions: { - arrowSize: new Dimension2( 20, 4 ), + buttonOptions: { + arrowSize: new Dimension2( 20, 4 ), + }, // Like ComboBox, 'vertical' is the only orientation supported (verified below). orientation: 'vertical', Index: balancing-act/js/common/view/MassCarousel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/balancing-act/js/common/view/MassCarousel.js b/balancing-act/js/common/view/MassCarousel.js --- a/balancing-act/js/common/view/MassCarousel.js (revision f519bbd34ccaa46ea613010b08357099d86b3b22) +++ b/balancing-act/js/common/view/MassCarousel.js (date 1674505182465) @@ -44,9 +44,11 @@ itemsPerPage: 1, // lightweight look for the buttons since the user must drag items across the buttons - buttonColor: null, - buttonDisabledColor: null, - buttonStroke: null, + buttonOptions: { + baseColor: null, + stroke: null, + disabledColor: null + }, tandem: Tandem.REQUIRED }, options ); Index: sun/js/buttons/CarouselButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/buttons/CarouselButton.ts b/sun/js/buttons/CarouselButton.ts --- a/sun/js/buttons/CarouselButton.ts (revision 361897928bfaff44a9ec2e80a7798417ab570436) +++ b/sun/js/buttons/CarouselButton.ts (date 1674505256333) @@ -1,18 +1,17 @@ // Copyright 2015-2022, University of Colorado Boulder -//TODO sun#197 ideally, only 2 corners of the button should be rounded (the corners in the direction of the arrow) /** * Next/previous button in a Carousel. * * @author Chris Malley (PixelZoom, Inc.) */ -import Bounds2 from '../../../dot/js/Bounds2.js'; import Dimension2 from '../../../dot/js/Dimension2.js'; import Matrix3 from '../../../dot/js/Matrix3.js'; import { Shape } from '../../../kite/js/imports.js'; import optionize from '../../../phet-core/js/optionize.js'; -import { TColor, Path } from '../../../scenery/js/imports.js'; +import StrictOmit from '../../../phet-core/js/types/StrictOmit.js'; +import { PaintColorProperty, Path, PathOptions } from '../../../scenery/js/imports.js'; import sun from '../sun.js'; import ButtonNode from './ButtonNode.js'; import RectangularPushButton, { RectangularPushButtonOptions } from './RectangularPushButton.js'; @@ -26,30 +25,20 @@ }; type ArrowDirection = 'up' | 'down' | 'left' | 'right'; -type LineCap = 'round' | 'square' | 'butt'; type SelfOptions = { - - // arrow + arrowPathOptions?: PathOptions; arrowDirection?: ArrowDirection; // direction that the arrow points - arrowSize?: Dimension2; // size of the arrow, in 'up' directions - arrowStroke?: TColor; // {color used for the arrow icons - arrowLineWidth?: number; // line width used to stroke the arrow icons - arrowLineCap?: LineCap; - - // Convenience options for dilating pointer areas such that they do not overlap with Carousel content. - // See computePointerArea. - touchAreaXDilation?: number; - touchAreaYDilation?: number; - mouseAreaXDilation?: number; - mouseAreaYDilation?: number; + arrowSize?: Dimension2; // size of the arrow (width/height when it is pointing up) }; -export type CarouselButtonOptions = SelfOptions & RectangularPushButtonOptions; +export type CarouselButtonOptions = SelfOptions & StrictOmit; export default class CarouselButton extends RectangularPushButton { - public constructor( providedOptions?: RectangularPushButtonOptions ) { + private readonly customStrokeProperty: PaintColorProperty | null; + + public constructor( providedOptions?: CarouselButtonOptions ) { // see supertype for additional options const options = optionize()( { @@ -57,22 +46,29 @@ // CarouselButtonOptions arrowDirection: 'up', arrowSize: new Dimension2( 20, 7 ), - arrowStroke: 'black', - arrowLineWidth: 3, - arrowLineCap: 'round', - touchAreaXDilation: 0, - touchAreaYDilation: 0, - mouseAreaXDilation: 0, - mouseAreaYDilation: 0, + + arrowPathOptions: { + stroke: 'black', + lineWidth: 3, + lineCap: 'round' + }, // RectangularPushButtonOptions baseColor: 'rgba( 200, 200, 200, 0.5 )', - stroke: 'black', buttonAppearanceStrategy: ButtonNode.FlatAppearanceStrategy, cornerRadius: 4 }, providedOptions ); + let customStrokeProperty: PaintColorProperty | null = null; + + if ( options.stroke === undefined ) { + customStrokeProperty = new PaintColorProperty( options.baseColor, { + luminanceFactor: -0.8 + } ); + options.stroke = customStrokeProperty; + } + // validate options assert && assert( ANGLES.hasOwnProperty( options.arrowDirection ), `invalid direction: ${options.arrowDirection}` ); @@ -86,60 +82,47 @@ arrowShape = arrowShape.transformed( Matrix3.rotation2( ANGLES[ options.arrowDirection ] ) ); // Arrow node - options.content = new Path( arrowShape, { - stroke: options.arrowStroke, - lineWidth: options.arrowLineWidth, - lineCap: options.arrowLineCap - } ); + options.content = new Path( arrowShape, options.arrowPathOptions ); // set up the options such that the inner corners are square and outer ones are rounded - const arrowDirection = options.arrowDirection; // convenience var - const cornerRadius = options.cornerRadius; // convenience var + const arrowDirection = options.arrowDirection; + const cornerRadius = options.cornerRadius; options.leftTopCornerRadius = arrowDirection === 'up' || arrowDirection === 'left' ? cornerRadius : 0; options.rightTopCornerRadius = arrowDirection === 'up' || arrowDirection === 'right' ? cornerRadius : 0; options.leftBottomCornerRadius = arrowDirection === 'down' || arrowDirection === 'left' ? cornerRadius : 0; options.rightBottomCornerRadius = arrowDirection === 'down' || arrowDirection === 'right' ? cornerRadius : 0; + // Computes touch area dilations/shifts so that the pointer area will not overlap with the contents of a Carousel. + // We do this here so that it's set up to work with any dynamic layout + if ( arrowDirection === 'up' || arrowDirection === 'down' ) { + const mouseAreaYDilation = options.mouseAreaYDilation / 2 || 0; + const touchAreaYDilation = options.touchAreaYDilation / 2 || 0; + + options.mouseAreaYDilation = mouseAreaYDilation; + options.touchAreaYDilation = touchAreaYDilation; + options.mouseAreaYShift = arrowDirection === 'up' ? -mouseAreaYDilation : mouseAreaYDilation; + options.touchAreaYShift = arrowDirection === 'up' ? -touchAreaYDilation : touchAreaYDilation; + } + else { + const mouseAreaXDilation = options.mouseAreaXDilation / 2 || 0; + const touchAreaXDilation = options.touchAreaXDilation / 2 || 0; + + options.mouseAreaXDilation = mouseAreaXDilation; + options.touchAreaXDilation = touchAreaXDilation; + options.mouseAreaXShift = arrowDirection === 'left' ? -mouseAreaXDilation : mouseAreaXDilation; + options.touchAreaXShift = arrowDirection === 'left' ? -touchAreaXDilation : touchAreaXDilation; + } + super( options ); - // pointer areas - this.touchArea = computePointerArea( this, arrowDirection, options.touchAreaXDilation, options.touchAreaYDilation ); - this.mouseArea = computePointerArea( this, arrowDirection, options.mouseAreaXDilation, options.mouseAreaYDilation ); + this.customStrokeProperty = customStrokeProperty; } -} -/** - * Computes a pointer area based on dilation of a CarouselButton's local bounds. - * The button is not dilated in the direction that is opposite to the arrow's direction. - * This ensures that the pointer area will not overlap with the contents of a Carousel. - * - * @param button - * @param arrowDirection - direction that the arrow points - * @param x - horizontal dilation - * @param y - vertical dilation - * @returns the pointer area, null if no dilation is necessary, i.e. x === 0 && y === 0 - */ -function computePointerArea( button: CarouselButton, arrowDirection: ArrowDirection, x: number, y: number ): Bounds2 | null { - let pointerArea = null; - if ( x || y ) { - switch( arrowDirection ) { - case 'up': - pointerArea = button.localBounds.dilatedXY( x, y / 2 ).shiftedY( -y / 2 ); - break; - case 'down': - pointerArea = button.localBounds.dilatedXY( x, y / 2 ).shiftedY( y / 2 ); - break; - case 'left': - pointerArea = button.localBounds.dilatedXY( x / 2, y ).shiftedX( -x / 2 ); - break; - case 'right': - pointerArea = button.localBounds.dilatedXY( x / 2, y ).shiftedX( x / 2 ); - break; - default: - throw new Error( `unsupported arrowDirection: ${arrowDirection}` ); - } + public override dispose(): void { + this.customStrokeProperty && this.customStrokeProperty.dispose(); + + super.dispose(); } - return pointerArea; } sun.register( 'CarouselButton', CarouselButton ); \ No newline at end of file Index: number-suite-common/js/lab/view/NumberCardCreatorCarousel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-suite-common/js/lab/view/NumberCardCreatorCarousel.ts b/number-suite-common/js/lab/view/NumberCardCreatorCarousel.ts --- a/number-suite-common/js/lab/view/NumberCardCreatorCarousel.ts (revision 603efa1b63f46b27ab0c7c9dc3eca2ae98537037) +++ b/number-suite-common/js/lab/view/NumberCardCreatorCarousel.ts (date 1674490935414) @@ -57,7 +57,9 @@ itemsPerPage: 10, margin: 10, spacing: 10, - animationDuration: 0.4 + animationOptions: { + duration: 0.4 + } } ); this.screenView = screenView; Index: number-line-operations/js/common/view/OperationEntryCarousel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-line-operations/js/common/view/OperationEntryCarousel.js b/number-line-operations/js/common/view/OperationEntryCarousel.js --- a/number-line-operations/js/common/view/OperationEntryCarousel.js (revision 087e65e6f4cdfbcf383c221a59fe9acc06e458ed) +++ b/number-line-operations/js/common/view/OperationEntryCarousel.js (date 1674505195334) @@ -61,9 +61,11 @@ margin: 10, fill: new Color( 255, 255, 255, 0.5 ), stroke: options.themeColor, - buttonColor: options.themeColor, - buttonStroke: new Color( 255, 255, 255, 0.1 ), - buttonDisabledColor: new Color( 255, 255, 255, 0.1 ) + buttonOptions: { + baseColor: options.themeColor, + stroke: new Color( 255, 255, 255, 0.1 ), + disabledColor: new Color( 255, 255, 255, 0.1 ) + } } ); // page indicator Index: function-builder/js/common/view/SceneNode.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/function-builder/js/common/view/SceneNode.js b/function-builder/js/common/view/SceneNode.js --- a/function-builder/js/common/view/SceneNode.js (revision d8bc005f808dc18ccf483fae7ac1be29bd8837ce) +++ b/function-builder/js/common/view/SceneNode.js (date 1674502060134) @@ -100,8 +100,10 @@ separatorsVisible: true, itemsPerPage: options.cardsPerPage, defaultPageNumber: options.cardCarouselDefaultPageNumber, - buttonTouchAreaXDilation: 5, - buttonTouchAreaYDilation: 15, + buttonOptions: { + touchAreaXDilation: 5, + touchAreaYDilation: 15 + }, spacing: 20, margin: 10, left: layoutBounds.left + 30, Index: sun/js/Carousel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/Carousel.ts b/sun/js/Carousel.ts --- a/sun/js/Carousel.ts (revision 361897928bfaff44a9ec2e80a7798417ab570436) +++ b/sun/js/Carousel.ts (date 1674505292227) @@ -17,16 +17,14 @@ import StrictOmit from '../../phet-core/js/types/StrictOmit.js'; import Property from '../../axon/js/Property.js'; import stepTimer from '../../axon/js/stepTimer.js'; -import Timer from '../../axon/js/Timer.js'; import Dimension2 from '../../dot/js/Dimension2.js'; import { Shape } from '../../kite/js/imports.js'; import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js'; import optionize, { combineOptions } from '../../phet-core/js/optionize.js'; -import { AlignBox, AlignGroup, HBox, IndexedNodeIO, Line, Node, NodeOptions, Rectangle, Separator, TColor, VBox } from '../../scenery/js/imports.js'; -import TSoundPlayer from '../../tambo/js/TSoundPlayer.js'; +import { AlignBox, AlignGroup, FlowBox, HBox, IndexedNodeIO, LayoutOrientation, Line, Node, NodeOptions, Rectangle, Separator, SeparatorOptions, TPaint, VBox } from '../../scenery/js/imports.js'; import pushButtonSoundPlayer from '../../tambo/js/shared-sound-players/pushButtonSoundPlayer.js'; import Tandem from '../../tandem/js/Tandem.js'; -import Animation from '../../twixt/js/Animation.js'; +import Animation, { AnimationOptions } from '../../twixt/js/Animation.js'; import Easing from '../../twixt/js/Easing.js'; import CarouselButton, { CarouselButtonOptions } from './buttons/CarouselButton.js'; import ColorConstants from './ColorConstants.js'; @@ -42,9 +40,9 @@ type SelfOptions = { // container - orientation?: 'horizontal' | 'vertical'; - fill?: TColor; // background color of the carousel - stroke?: TColor; // color used to stroke the border of the carousel + orientation?: LayoutOrientation; + fill?: TPaint; // background color of the carousel + stroke?: TPaint; // color used to stroke the border of the carousel lineWidth?: number; // width of the border around the carousel cornerRadius?: number; // radius applied to the carousel and next/previous buttons defaultPageNumber?: number; // page that is initially visible @@ -54,31 +52,16 @@ spacing?: number; // spacing between items, between items and optional separators, and between items and buttons margin?: number; // margin between items and the edges of the carousel - // next/previous buttons - buttonColor?: TColor; // base color for the buttons - buttonStroke?: TColor | 'derived'; // stroke around the buttons, 'derived' derives a stroke from buttonColor - buttonDisabledColor?: TColor; // same default as from ButtonNode.js - buttonLineWidth?: number; // lineWidth of borders on buttons - arrowSize?: Dimension2; // size of the arrow, in 'up' directions - arrowStroke?: TColor; // color used for the arrow icons - arrowLineWidth?: number; // line width used to stroke the arrow icons - buttonSoundPlayer?: TSoundPlayer; // sound played when carousel button is pressed + // next/previous button options + buttonOptions?: CarouselButtonOptions; - // for dilating pointer areas of next/previous buttons such that they do not overlap with Carousel content - buttonTouchAreaXDilation?: number; // horizontal touchArea dilation - buttonTouchAreaYDilation?: number; // vertical touchArea dilation - buttonMouseAreaXDilation?: number; // horizontal mouseArea dilation - buttonMouseAreaYDilation?: number; // vertical mouseArea dilation - - // item separators + // item separator options separatorsVisible?: boolean; // whether to put separators between items - separatorColor?: TColor; // color for separators - separatorLineWidth?: number; // lineWidth for separators + separatorOptions?: SeparatorOptions; // animation, scrolling between pages animationEnabled?: boolean; // is animation enabled when scrolling between pages? - animationDuration?: number; // seconds - stepEmitter?: Timer; // see Animation options.stepEmitter + animationOptions?: StrictOmit, 'to' | 'setValue' | 'getValue'>; // We override to/setValue/getValue }; export type CarouselOptions = SelfOptions & StrictOmit; @@ -110,12 +93,8 @@ private readonly backgroundHeight: number; private readonly disposeCarousel: () => void; - private readonly scrollingNode: HBox | VBox; + private readonly scrollingNode: FlowBox; - /** - * @param items - Nodes shown in the carousel - * @param providedOptions - */ public constructor( items: CarouselItem[], providedOptions?: CarouselOptions ) { // Don't animate during initialization @@ -138,30 +117,44 @@ margin: 6, // next/previous buttons - buttonColor: 'rgba( 200, 200, 200, 0.5 )', - buttonStroke: 'derived', - buttonDisabledColor: ColorConstants.LIGHT_GRAY, - buttonLineWidth: 1, - arrowSize: DEFAULT_ARROW_SIZE, - arrowStroke: 'black', - arrowLineWidth: 3, - buttonSoundPlayer: pushButtonSoundPlayer, + buttonOptions: { + xMargin: 5, + yMargin: 5, + + // for dilating pointer areas of next/previous buttons such that they do not overlap with Carousel content + touchAreaXDilation: 0, + touchAreaYDilation: 0, + mouseAreaXDilation: 0, + mouseAreaYDilation: 0, + + baseColor: 'rgba( 200, 200, 200, 0.5 )', + disabledColor: ColorConstants.LIGHT_GRAY, + lineWidth: 1, + + arrowPathOptions: { + stroke: 'black', + lineWidth: 3 + }, + arrowSize: DEFAULT_ARROW_SIZE, - // for dilating pointer areas of next/previous buttons such that they do not overlap with Carousel content - buttonTouchAreaXDilation: 0, - buttonTouchAreaYDilation: 0, - buttonMouseAreaXDilation: 0, - buttonMouseAreaYDilation: 0, + soundPlayer: pushButtonSoundPlayer + }, // item separators separatorsVisible: false, - separatorColor: 'rgb( 180, 180, 180 )', - separatorLineWidth: 0.5, + separatorOptions: { + stroke: 'rgb( 180, 180, 180 )', + lineWidth: 0.5, + pickable: false + }, // animation, scrolling between pages animationEnabled: true, - animationDuration: 0.4, - stepEmitter: stepTimer, + animationOptions: { + duration: 0.4, + stepEmitter: stepTimer, + easing: Easing.CUBIC_IN_OUT + }, // phet-io tandem: Tandem.OPTIONAL, @@ -187,29 +180,15 @@ const maxItemHeight = _.maxBy( alignBoxes, ( item: Node ) => item.height )!.height; // Options common to both buttons - const buttonOptions = { - xMargin: 5, - yMargin: 5, + const buttonOptions = combineOptions( { cornerRadius: options.cornerRadius, - baseColor: options.buttonColor, - disabledColor: options.buttonDisabledColor, - stroke: ( options.buttonStroke === 'derived' ) ? undefined : options.buttonStroke, - lineWidth: options.buttonLineWidth, minWidth: isHorizontal ? 0 : maxItemWidth + ( 2 * options.margin ), // fill the width of a vertical carousel minHeight: isHorizontal ? maxItemHeight + ( 2 * options.margin ) : 0, // fill the height of a horizontal carousel - arrowSize: options.arrowSize, - arrowStroke: options.arrowStroke, - arrowLineWidth: options.arrowLineWidth, - touchAreaXDilation: options.buttonTouchAreaXDilation, - touchAreaYDilation: options.buttonTouchAreaYDilation, - mouseAreaXDilation: options.buttonMouseAreaXDilation, - mouseAreaYDilation: options.buttonMouseAreaYDilation, - soundPlayer: options.buttonSoundPlayer, enabledPropertyOptions: { phetioReadOnly: true, phetioFeatured: false } - } as const; + }, options.buttonOptions ); assert && assert( options.spacing >= options.margin, 'The spacing must be >= the margin, or you will see ' + 'page 2 items at the end of page 1' ); @@ -273,12 +252,9 @@ const x2 = isHorizontal ? x1 : scrollingNode.width; const y2 = isHorizontal ? scrollingNode.height : ( node1.bottom + node2.top ) / 2; - separators.push( new Separator( { - x1: x1, y1: y1, x2: x2, y2: y2, - stroke: options.separatorColor, - lineWidth: options.separatorLineWidth, - pickable: false - } ) ); + separators.push( new Separator( combineOptions( { + x1: x1, y1: y1, x2: x2, y2: y2 + }, options.separatorOptions ) ) ); } separatorLayer!.children = separators; @@ -394,18 +370,13 @@ if ( this.animationEnabled && !phet.joist.sim.isSettingPhetioStateProperty.value && isInitialized ) { // create and start the scroll animation - scrollAnimation = new Animation( { - - // options not specific to orientation - duration: options.animationDuration, - stepEmitter: options.stepEmitter, - easing: Easing.CUBIC_IN_OUT, + scrollAnimation = new Animation( combineOptions>( {}, options.animationOptions, { to: targetValue, // options that are specific to orientation getValue: () => isHorizontal ? scrollingNodeContainer.left : scrollingNodeContainer.top, setValue: ( value: number ) => { scrollingNodeContainer[ isHorizontal ? 'left' : 'top' ] = value; } - } ); + } ) ); scrollAnimation.start(); } else { @@ -514,7 +485,6 @@ itemAlignBox.visible = visible; } - // Return the created node given a CarouselItem public getCreatedNodeForItem( item: CarouselItem ): Node { const itemIndex = this.items.indexOf( item ); const node = this.carouselItemNodes[ itemIndex ]; @@ -522,9 +492,6 @@ return node; } - /** - * Converts an item index to a page number. - */ private itemIndexToPageNumber( itemIndex: number ): number { assert && assert( itemIndex >= 0 && itemIndex < this.items.length, `itemIndex out of range: ${itemIndex}` ); return Math.floor( itemIndex / this.itemsPerPage ); ```

matthew-blackman commented 1 year ago

@jonathanolson editing this to add some clarity. While completing the CCK tree review on 1/24/23, @arouinfar @samreid and I found that pageNumberProperty has a static set of valid values. If a page is hidden due to removed elements, the Studio interface still shows a radio button for the missing page.

@samreid and @matthew-blackman made dynamic range, and now there is a get/setValue interface. Valid values include the visible pages. Range property includes the total number of pages.

Ran into trouble regenerating the API, so placing updates in the following patch:

Subject: [PATCH] Replace validValues with isValidValue in pageNumberProperty to check if page number exists when setting via studio, add range to pageNumberProperty showing initial number of pages - see https://github.com/phetsims/sun/issues/814
---
Index: js/Carousel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/Carousel.ts b/js/Carousel.ts
--- a/js/Carousel.ts    (revision 361897928bfaff44a9ec2e80a7798417ab570436)
+++ b/js/Carousel.ts    (date 1674585128830)
@@ -19,6 +19,7 @@
 import stepTimer from '../../axon/js/stepTimer.js';
 import Timer from '../../axon/js/Timer.js';
 import Dimension2 from '../../dot/js/Dimension2.js';
+import Range from '../../dot/js/Range.js';
 import { Shape } from '../../kite/js/imports.js';
 import InstanceRegistry from '../../phet-core/js/documentation/InstanceRegistry.js';
 import optionize, { combineOptions } from '../../phet-core/js/optionize.js';
@@ -308,7 +309,8 @@
     const pageNumberProperty = new NumberProperty( options.defaultPageNumber, {
       tandem: options.tandem.createTandem( 'pageNumberProperty' ),
       numberType: 'Integer',
-      validValues: _.range( this.numberOfPagesProperty.value ),
+      isValidValue: ( value: number ) => value < this.numberOfPagesProperty.value && value >= 0,
+      range: new Range( 0, this.numberOfPagesProperty.value - 1 ),
       phetioFeatured: true
     } );

samreid commented 1 year ago

We had ComboBox errors when regenerating the API in the branches, but we need to commit that patch in the prior commit.

jonathanolson commented 1 year ago

Review isn't complete, but I've done significant amounts of work to Carousel / PageControl that's worth a "review inside a review". @samreid can you see the changes I've done above?

I'd also benefit from having someone more experienced with phet-io tag along for IndexedNodeIO review.

I'm to the point where I'll be scanning the other commits for changes (I'd consider the files Carousel/CarouselButton/PageControl to be fully reviewed now).

jonathanolson commented 1 year ago

Review complete, and I fixed up the ordering issues that showed up during today's meetings.

jonathanolson commented 1 year ago

Hey! @jbphet noticed the expression-exchange carousel is resizing on the 2nd screen when "All Coefficients" checkbox is toggled.

It looks like the content items are changing sizes. We'll probably want a good way to support minimum sizes for cells (to pass to the align boxes). Any thoughts on how to support this (should the sim just be wrapping it in things that don't change size?)

samreid commented 1 year ago

@zepumph and I made a cursory review a few days ago. @chrisklus said a minor disruption would be OK as long as major functionality in Number Play lab screen is preserved. @pixelzoom requested not to disrupt the Beer's Law Lab SHAs, but we have created those RCs. I'll take a look at the expression-exchange carousel behavior. I see in master it does not resize.

samreid commented 1 year ago

I reproduced the problem in expression exchange where carousel items change size and the carousel resizes. I agree it doesn't look so good.

samreid commented 1 year ago

The worst case looks like this:

samreid commented 1 year ago

Any thoughts on how to support this (should the sim just be wrapping it in things that don't change size?)

I followed that strategy in this patch:

```diff Subject: [PATCH] Fix default group, see https://github.com/phetsims/circuit-construction-kit-common/issues/937 --- Index: js/common/view/CoinTermCreatorBoxFactory.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/CoinTermCreatorBoxFactory.js b/js/common/view/CoinTermCreatorBoxFactory.js --- a/js/common/view/CoinTermCreatorBoxFactory.js (revision b679d04bd3d967c11c7e275126ccf71df04b3f79) +++ b/js/common/view/CoinTermCreatorBoxFactory.js (date 1675477369116) @@ -153,7 +153,7 @@ options = merge( { itemsPerCarouselPage: creatorSetID === CoinTermCreatorSetID.VARIABLES ? 4 : 3, - itemSpacing: creatorSetID === CoinTermCreatorSetID.VARIABLES ? 35 : 40 + itemSpacing: creatorSetID === CoinTermCreatorSetID.VARIABLES ? 5 : 10 }, options ); // create the list of creator nodes from the descriptor list @@ -180,7 +180,7 @@ */ createGameScreenCreatorBox( challengeDescriptor, model, view, options ) { options = merge( { - itemSpacing: 30, + itemSpacing: 5, align: 'top' }, options ); Index: js/common/view/CoinTermCreatorBox.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/CoinTermCreatorBox.js b/js/common/view/CoinTermCreatorBox.js --- a/js/common/view/CoinTermCreatorBox.js (revision b679d04bd3d967c11c7e275126ccf71df04b3f79) +++ b/js/common/view/CoinTermCreatorBox.js (date 1675477961308) @@ -10,8 +10,10 @@ */ import merge from '../../../../phet-core/js/merge.js'; -import { Node } from '../../../../scenery/js/imports.js'; +import { Node, Rectangle } from '../../../../scenery/js/imports.js'; import Carousel from '../../../../sun/js/Carousel.js'; +import { Shape } from '../../../../kite/js/imports.js'; +import Vector2 from '../../../../dot/js/Vector2.js'; import expressionExchange from '../../expressionExchange.js'; class CoinTermCreatorBox extends Node { @@ -26,7 +28,7 @@ options = merge( { itemsPerCarouselPage: 3, - itemSpacing: 20, // empirically determined to work for most cases in this sim + itemSpacing: 5, // empirically determined to work for most cases in this sim cornerRadius: 4, align: 'center' }, options ); @@ -40,11 +42,30 @@ // @private {Node} this.coinTermCreatorBox = new Carousel( creatorNodes.map( element => { - return { createNode: tandem => element }; + return { + createNode: tandem => { + + // Pick bounds that fit every single thing in the sim. + const H = 90; + const W = 140; + + // Could be a Node, but this helps with debugging if you want to see the bounds. + const panel = new Rectangle( 0, 0, W, H, { + fill: 'transparent', + children: [ element ] + } ); + element.center = new Vector2( W / 2, H / 2 ); + + // Prevent resizing when elements bounds want to go outside the rectangle + panel.clipArea = Shape.rect( 0, 0, W, H ); + + return panel; + } + }; } ), { itemsPerPage: options.itemsPerCarouselPage, - spacing: options.itemSpacing, - margin: 14, + spacing: 5, + margin: 5, cornerRadius: options.cornerRadius } ); this.addChild( this.coinTermCreatorBox ); ```

It looks reasonable enough in every circumstance I tried, but it also seems kind of brittle if some coins or terms change size and this doesn't accommodate. Luckily I didn't see any translateable strings in the coins or terms. If this is the last part, we may proceed and request help from @jbphet if pixel polishing is necessary.

samreid commented 1 year ago

I'm using these URLs for testing behavior vs published:

https://phet.colorado.edu/sims/html/area-builder/latest/area-builder_en.html http://localhost/main/area-builder/area-builder_en.html

https://phet.colorado.edu/sims/html/balancing-act/latest/balancing-act_en.html http://localhost/main/balancing-act/balancing-act_en.html

https://phet.colorado.edu/sims/html/build-a-molecule/latest/build-a-molecule_en.html http://localhost/main/build-a-molecule/build-a-molecule_en.html

https://phet.colorado.edu/sims/html/circuit-construction-kit-ac/latest/circuit-construction-kit-ac_en.html http://localhost/main/circuit-construction-kit-ac/circuit-construction-kit-ac_en.html

https://phet.colorado.edu/sims/html/circuit-construction-kit-common/latest/circuit-construction-kit-common_en.html http://localhost/main/circuit-construction-kit-common/circuit-construction-kit-common_en.html

https://phet.colorado.edu/sims/html/circuit-construction-kit-dc/latest/circuit-construction-kit-dc_en.html http://localhost/main/circuit-construction-kit-dc/circuit-construction-kit-dc_en.html

https://phet.colorado.edu/sims/html/expression-exchange/latest/expression-exchange_en.html http://localhost/main/expression-exchange/expression-exchange_en.html

https://phet.colorado.edu/sims/html/function-builder/latest/function-builder_en.html http://localhost/main/function-builder/function-builder_en.html

https://phet.colorado.edu/sims/html/function-builder-basics/latest/function-builder-basics_en.html http://localhost/main/function-builder-basics/function-builder-basics_en.html

https://phet.colorado.edu/sims/html/geometric-optics/latest/geometric-optics_en.html http://localhost/main/geometric-optics/geometric-optics_en.html

https://phet.colorado.edu/sims/html/joist/latest/joist_en.html http://localhost/main/joist/joist_en.html

https://phet.colorado.edu/sims/html/number-line-operations/latest/number-line-operations_en.html http://localhost/main/number-line-operations/number-line-operations_en.html

https://phet.colorado.edu/sims/html/number-suite-common/latest/number-suite-common_en.html http://localhost/main/number-suite-common/number-suite-common_en.html

https://phet.colorado.edu/sims/html/scenery/latest/scenery_en.html http://localhost/main/scenery/scenery_en.html

https://phet.colorado.edu/sims/html/sun/latest/sun_en.html http://localhost/main/sun/sun_en.html

samreid commented 1 year ago

Everything else seems to be working, so I'm going to go for it. I'll open an expression exchange issue for @jbphet to take a closer look once merged back to master.

samreid commented 1 year ago

Merge complete. Still to do:

[x] Issue for expression exchange
[ ] Delete branches
[x] Close branch issues
[x] Request spot check in number play
[x] Request mini-review in CCK
[x] Message the google group

samreid commented 1 year ago

OK I opened or commented in all appropriate side issues. I want to wait for some CT columns to complete and to have more confidence the branches are all merged to master and pushed. After that, we can delete the branches.

samreid commented 1 year ago

CT caught an error in number line operations, which I fixed above. It also caught a bug in build-a-molecule, which I could not figure out, so I opened a side issue and assigned to @jonathanolson.

samreid commented 1 year ago

All work merged to master, has been reviewed. Side issues opened as appropriate. Branches deleted. Closing.

phetsims / sun