Skip to content

Conversation

@julien4215
Copy link
Collaborator

@julien4215 julien4215 commented Feb 13, 2025

Close #1408

The fix done in the PR #1428 could not work. This time it should really be fixed.

@veloce
Copy link
Contributor

veloce commented Feb 13, 2025

I'm not sure this fix would work better. UniqueKey shouldn't be created in build() method, it doesn't really make sense.

You need to store an unique key in a State object instead.

@julien4215 julien4215 force-pushed the fix-broadcast-clock-flip-board branch from 312b12a to b3fed1a Compare February 14, 2025 09:38

/// The side the board is displayed from.
///
/// It should only be used when you need the state of the board header or footer to be preserved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is misleading. You're not preserving the state, but you're forcing the destruction of the widgets (and recreation) when the ValueKey changes.

Since we always have that information available, I'd make that parameter mandatory. It would help to avoid further surprises.

Copy link
Collaborator Author

@julien4215 julien4215 Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly the documentation it will not recreate the widget if what you mean by widget is the underlying element. If I had kept the keys in the CountdownBuilder I agree that the underlying element would have been recreated which is why I moved it in the AnalysisLayout. I believe that thanks to the local key, it will simply call update on the element to update the widget configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we always have that information available, I'd make that parameter mandatory. It would help to avoid further surprises.

I didn't want to force this parameter as it is only useful when there is a board footer or header. Should I still make it mandatory and keep only the first line of the doc comment to not make it confusing ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got it. But there is no harm making it mandatory as we have the data ready and it's not used in a lot of places. Also it is future proof, in case we'll use stateful widgets in other screens than broadcast.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the pov parameter mandatory and added comments where we use a key to explain why it is needed

the board is flipped was incorrect, this commit corrects it.
@julien4215 julien4215 force-pushed the fix-broadcast-clock-flip-board branch from b3fed1a to 6d1263a Compare February 14, 2025 11:02
@veloce veloce merged commit f18fcb2 into lichess-org:main Feb 14, 2025
1 check passed
@julien4215 julien4215 deleted the fix-broadcast-clock-flip-board branch February 16, 2025 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broadcast bug for flip board

2 participants