Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More progress on broadcast feature #944

Merged
merged 105 commits into from
Nov 27, 2024

Conversation

julien4215
Copy link
Contributor

@julien4215 julien4215 commented Aug 26, 2024

screen-20240910-235044.2.mp4

@ijm8710
Copy link

ijm8710 commented Aug 26, 2024

It's minor but not sure tournament is the right word to switch between open and gm specific.
Maybe draw. Veloce may know the best word but I think tournament may falsely induce the user to think that's a dropdown to switch to a whole new tournament altogether when it's not.

@ornicar
Copy link
Contributor

ornicar commented Aug 26, 2024

Very impressive work Julien! 👍

@tom-anders
Copy link
Contributor

tom-anders commented Aug 27, 2024

Looks really cool!

In the analysis screen (final part of the video), would it make sense to display the players' names above/below the board? The mobile site also does this.

Screenshot_20240827-175849

@julien4215
Copy link
Contributor Author

julien4215 commented Aug 27, 2024

Looks really cool!

In the analysis screen (final part of the video), would it make sense to display the players' names above/below the board? The mobile site also does this.

Yes, I also think it would be better with the players' names and clock/result.

@veloce
Copy link
Contributor

veloce commented Aug 29, 2024

Yeah, awesome work @julien4215 . I think we can release like that indeed, even without the eval bar. I'll deal with it asap. Right now I'm focusing on releasing the new editor and opening explorer so I think it'll be part of the next release.

@veloce
Copy link
Contributor

veloce commented Sep 3, 2024

I'm focusing right now on the next release but I'll review asap. Can you please fix the test and solve the conflict in the meantime? Thanks!

@julien4215
Copy link
Contributor Author

I'll fix the tests and the conflict once the PR is fully ready. I have to fix some problems with the clock and the analysis screen.

@veloce
Copy link
Contributor

veloce commented Sep 5, 2024

I'll fix the tests and the conflict once the PR is fully ready. I have to fix some problems with the clock and the analysis screen.

In that case I prefer that the PR is a draft. Easier for me to choose PR to review. Thanks!

@julien4215 julien4215 marked this pull request as draft September 5, 2024 15:31
@veloce
Copy link
Contributor

veloce commented Nov 22, 2024

So it is ready to review? Awesome. There's just a couple of conflicts to solve and I'll review this.

@julien4215
Copy link
Contributor Author

Yes it is ready to review.

I fixed the conflicts and I modified the code to use the new AnalysisLayout widget in the broadcast game screen.

Copy link
Contributor

@veloce veloce left a comment

Choose a reason for hiding this comment

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

Excellent work! I think we're getting close to release this. Only have a couple of comments here.

I'll test this next week, and figure out something about the dropdown menu bug, since I don't really want to patch flutter.

this.enableDrawingShapes = true,
this.shouldReplaceChildOnUserMove = false,
});

final AnalysisOptions options;
final double boardSize;
final BorderRadiusGeometry? borderRadius;
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand that. Why changing to radius instead of borderRadius?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you talking only about the name of the variable or the type ?

Copy link
Contributor

Choose a reason for hiding this comment

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

both

Copy link
Contributor Author

@julien4215 julien4215 Nov 23, 2024

Choose a reason for hiding this comment

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

To add rounded corners to the players widgets instead of the board for broadcast like on the website

The change of the name is just to match the type

lib/src/view/broadcast/broadcast_game_screen.dart Outdated Show resolved Hide resolved
lib/src/view/broadcast/broadcast_game_tree_view.dart Outdated Show resolved Hide resolved
lib/src/view/broadcast/broadcast_screen.dart Outdated Show resolved Hide resolved
lib/src/widgets/evaluation_bar.dart Outdated Show resolved Hide resolved
lib/src/widgets/pgn.dart Show resolved Hide resolved
@@ -287,7 +287,8 @@ class _CountdownClockState extends State<CountdownClockBuilder> {
void didUpdateWidget(CountdownClockBuilder oldClock) {
super.didUpdateWidget(oldClock);

if (widget.clockUpdatedAt != oldClock.clockUpdatedAt) {
if (widget.timeLeft != oldClock.timeLeft ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that change is a good idea. The way to clock is designed is to rely only on clockUpdatedAt to know when the clock has changed. We want to force update clockUpdateAt, and here you'd broke that contract.

Copy link
Contributor Author

@julien4215 julien4215 Nov 23, 2024

Choose a reason for hiding this comment

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

Not sure to understand your comment. Do you mean that the only way to change the clock should be the clockUpdateAt field ?

Copy link
Contributor

@veloce veloce Nov 25, 2024

Choose a reason for hiding this comment

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

The only way to indicate the clock has changed is to update clockUpdatedAt. Meaning if you want to update timeLeft you have to update clockUpdatedAt. We want to support updating the clock with the same time left, so this is the only way to ensure it.
If you don't provide at all the clockUpdatedAt it means timeLeft will never change.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can read the tests in clock_test.dart to understand also, it contains the specification.

Copy link
Contributor Author

@julien4215 julien4215 Nov 26, 2024

Choose a reason for hiding this comment

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

I don't understand why you are saying that

We want to support updating the clock with the same time left, so this is the only way to ensure it.

With what I have done you can still update the clock with the same time left.

Copy link
Contributor Author

@julien4215 julien4215 Nov 26, 2024

Choose a reason for hiding this comment

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

The reason I needed to also update the clock when timeLeft change (which makes sense I think) is that on the broadcast game screen when going through the moves I need to display the clocks and the clocks are always inactive with no clockUpdatedAt value expect for the current broadcast path if the game is still ongoing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I could do instead is simply use a text widget but then why add an inactive field.

@@ -115,7 +115,7 @@ void main() {
expect(find.text('0:00.0'), findsOneWidget);
});

testWidgets('do not update if clockUpdatedAt is same',
testWidgets('do not update if timeLeft and clockUpdatedAt are same',
Copy link
Contributor

Choose a reason for hiding this comment

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

Cf. other comment, this change need to be removed.

@julien4215
Copy link
Contributor Author

julien4215 commented Nov 23, 2024

I'll test this next week, and figure out something about the dropdown menu bug, since I don't really want to patch flutter.

Flutter fixed the issue on the master branch but I don't know when it will land in the beta.

If you don't want to patch Flutter maybe I can just copy the fixed dropdown menu widget in the broadcast folder for now and we remove it once it lands on the beta channel ?

@veloce
Copy link
Contributor

veloce commented Nov 25, 2024

I'll test this next week, and figure out something about the dropdown menu bug, since I don't really want to patch flutter.

Flutter fixed the issue on the master branch but I don't know when it will land in the beta.

If you don't want to patch Flutter maybe I can just copy the fixed dropdown menu widget in the broadcast folder for now and we remove it once it lands on the beta channel ?

Not sure about that. The problem with the dropdown menu is that we can't show more detailed info it seems. I think it would be very nice to show the round date as in the website, it is an important information. And it looks like we can't do it with DropdownMenu.

I wonder what is the alternative? Maybe showMenu and PopupMenuItem? https://api.flutter.dev/flutter/material/PopupMenuItem-class.html

@julien4215
Copy link
Contributor Author

julien4215 commented Nov 25, 2024

The dropdown menu entries can be larger than the button so that should be possible to show more detailed information.

Maybe it can be done in a different pull request after merging this one ? This one is already bigger than what I planned to do.

@tom-anders
Copy link
Contributor

tom-anders commented Nov 25, 2024

The dropdown menu entries can be larger than the button so that should be possible to show more detailed information.

Maybe it can be done in a different pull request after merging this one ? This one is already bigger than what I planned to do.

Plus, people would probably be very excited if this feature is released before the WCC ends, even if some features are missing

@veloce
Copy link
Contributor

veloce commented Nov 26, 2024

You're right we'll see to improve the menu after. So you can put a copy of the widget in the meantime.

@julien4215
Copy link
Contributor Author

So you can put a copy of the widget in the meantime.

Done. I also merged main to get the new search time setting and the new ui for settings.

@veloce veloce merged commit f34c092 into lichess-org:main Nov 27, 2024
1 check passed
@veloce
Copy link
Contributor

veloce commented Nov 27, 2024

🎉 finally!

I must thank you again @julien4215 , this was a long journey but we finally made it! Thanks to your dedication and hard work. Congrats!

@julien4215 julien4215 deleted the broadcast-feature branch November 27, 2024 22:10
@julien4215
Copy link
Contributor Author

Happy to see it merged into main! And thanks for you thorough review.

I'll try to make smaller PR in the future. This one got a bit out of hand.

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.

5 participants