Skip to content

Conversation

@Lonec-L
Copy link

@Lonec-L Lonec-L commented Jul 18, 2024

fixes #870

sounds should now be played correctly when user goes to previous or next move on analysis board. The issue was that only associated move of current node (after the requested change) was considered when determining which sound to play. Now the associated move of current node is used when user moves forward through the tree and associated move of previous node (before the requested change) is used when backtracking.

This also fixes move sound not playing when reverting first move of the game. Now the normal move sound is played whenever current node is not a Branch. I am not sure about this, so I am looking for some feedback if this is undesired behavior.

@tom-anders
Copy link
Collaborator

tom-anders commented Jul 18, 2024

I got curious what lichess.org and the old app do: Turns out they both play actually no sound at all when reverting a move in the analysis board.

Doesn't mean the new app has to do the same, I just thought I'd mention this.

@Lonec-L
Copy link
Author

Lonec-L commented Jul 21, 2024

To be honest, I did not pay attention to that. I just wanted to start contributing to the new app in some way, and this seemed like a good first issue to solve.

@veloce
Copy link
Contributor

veloce commented Jul 25, 2024

Thanks for your contribution @Lonec-L . I'm still not sure we should merge it because:

  1. it is not a bug but was intended
  2. it is the same behavior right now as in the website and the old app

@tom-anders
Copy link
Collaborator

tom-anders commented Jul 26, 2024

Thanks for your contribution @Lonec-L . I'm still not sure we should merge it because:

  1. it is not a bug but was intended
  2. it is the same behavior right now as in the website and the old app

I think you misunderstood the issue here:

  1. lichess.org and the old app do not play a sound when reverting a move in the analysis board.
  2. the new app does play a sound
  3. the new app sometimes plays the wrong sound
  4. this PR fixes that

So this PR improves things, but it's still inconsistent with the old app and lichess.org.

@veloce
Copy link
Contributor

veloce commented Jul 26, 2024

Oh my bad, sorry. I really thought that the new app would not play a sound when reverting a move.

@tom-anders
Copy link
Collaborator

Oh my bad, sorry. I really thought that the new app would not play a sound when reverting a move.

Yeah, so IMO we should be consistent with the old app and Web UI, by not playing a sound at all when reverting a move (instead of fixing the bug). Whats your opinion on this?

@Lonec-L Lonec-L closed this Jul 27, 2024
@Lonec-L Lonec-L force-pushed the bugfix/analysis-board-move-sounds branch from 73d4100 to 9de1059 Compare July 27, 2024 14:13
to match the old app and lichess website
@Lonec-L Lonec-L reopened this Jul 27, 2024
@Lonec-L
Copy link
Author

Lonec-L commented Jul 27, 2024

I have reverted previous changes and reopened this PR, now with sounds completely removed when reverting moves during analysis, matching the behavior of old app and lichess website.

@veloce
Copy link
Contributor

veloce commented Jul 29, 2024

Ok, thank you for that.

I'm still not sure whether no sound when replaying backwards is a good idea. It is like lichess website for sure, but we are allowed to deviate from the website for things like that. I'll think about it.

@veloce veloce merged commit 37bb233 into lichess-org:main Jul 29, 2024
@veloce
Copy link
Contributor

veloce commented Jul 29, 2024

In the meantime, I'm merging this since it fixes an inconsistency.

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.

Move sounds are wrong when you take back a move on anlaysis board

4 participants