One must be able to turn off the music in the options screen

[richelbilderbeek]

Depends

• [x] #664

Context

The options screen is a dialog in which the user can specify his/her ideal preferences for the game. These preferences/settings are encapsulated in game_options. It would make sense that the options dialog allows one to modify all game_options.

In this case, there should be some UI element (e.g. an on/off button) to turn on/off the sound.

This is the test:

  //#define FIX_ISSUE_630
  #ifdef FIX_ISSUE_630
  // (630) One must be able to turn off the music in the options screen
  {
    const game_options options;
    assert(options.is_playing_music());
    options_view view(options);
    assert(view.get_options().is_playing_music());
    view.click_play_music_button();
    assert(!view.get_options().is_playing_music());
  }
  #endif // FIX_ISSUE_630

After that, the button needs to be clickable for real. Add it and let a human test this:

• Start the options screen, music should be on/off
• Click twice on (e.g.) 'Music on/off' to make it turn off/on (or vice versa)

[TheoPannetier]

Hey @richelbilderbeek @ollyturner looks like you guys assigned yourselves after me to this issue and came up with an alternative solution to the one I had made already, causing a merge conflict https://github.com/tresinformal/game/commit/902848c18125e60d2280ce533a0c6ff970aa6155

I gave you my blessing last week to work on my branch, but I feel leaving it in a state of merge conflict is quite ongezellig. Also, I feel that assigning yourselves to work on an issue I already assigned myself too and work from scratch on it undermines the whole point of assignment (that is, indicating to people you're already working on it to avoid divergences and merge conflicts).

Apologies if I misunderstood what happened, this is what I'm reading from this issue thread and the commit network.

[richelbilderbeek]

Hi @TheoPannetier, you are right, thanks for calling me out on that: we indeed should have fixed your branch.

I do partially agree with overriding assignments: yes, it's good that team members can claim an Issue. OTOH, team members that are absent should not slow down the team and hence we should be flexible to some extent. As I feel that this Issue is among the top priorities, I feel it was justified to override the assignment to the Issue.

Still, we should have left the Issue in a better state than in which we found it. Sorry for that, and thanks for helping me remember that :-)

[TheoPannetier]

I do partially agree with overriding assignments: yes, it's good that team members can claim an Issue. OTOH, team members that are absent should not slow down the team and hence we should be flexible to some extent. As I feel that this Issue is among the top priorities, I feel it was justified to override the assignment to the Issue.

That's fair! I think it's great that members can take over to help moving forward with the issue. Now my issue is that you started tackling the problem from scratch (https://github.com/tresinformal/game/commit/ee1dde909e777ce3f7b5a5e0de722c9a4b314539), overriding the progress I had made earlier (https://github.com/tresinformal/game/commit/2b41be2a4b09cf024fd9296a34bdce0b691bd1f2), which caused the merge conflict. I understand this is a genuine mistake rather than a decision, so please just fix the merge conflict tomorrow and I'm happy to move on :)

[richelbilderbeek]

@TheoPannetier I'll do so now :-) . Thanks for your understanding!

[richelbilderbeek]

Blimey, indeed, quite some code was lost. I was completely unaware of that! It must have been some merging rule that did that automatically!

[richelbilderbeek]

@TheoPannetier could it be that the changes you did were not pushed before that event and hence, we got no merge conflict?

I checked the network; this seems likely to me:

Still, I'll happily fix it :-)

[richelbilderbeek]

@TheoPannetier: merge conflict is solved, using your earlier idea :+1:

[richelbilderbeek]

I have added a test, try to make it come true:

  //#define FIX_ISSUE_630
  #ifdef FIX_ISSUE_630
  // (630) One must be able to turn off the music in the options screen
  {
    const game_options options;
    assert(options.is_playing_music());
    options_view view(options);
    assert(view.get_options().is_playing_music());
    view.click_play_music_button();
    assert(!view.get_options().is_playing_music());
  }
  #endif // FIX_ISSUE_630

[TheoPannetier]

Hi @richelbilderbeek , I like the test! But AFAICS I don't think it can be solved, because you cannot create events and resolve them other than by actually interacting with the window (that is, with a human). Maybe I am wrong? do you know howone could mimic the effect of clicking on the screen?

[richelbilderbeek]

Hi @TheoPannetier, thanks for the fun question!

But AFAICS I don't think it can be solved, because you cannot create events and resolve them other than by actually interacting with the window (that is, with a human). Maybe I am wrong? do you know howone could mimic the effect of clicking on the screen?

Your reasoning is correct! The idea, however, is to be able to test as much as possible. The options_view::click_play_music_button is a simple regular member function. It will be called by some of the SFML events and by the tests.

I hope that clarifies things :-) . The test is the way to assess my correctness and I think it looks quite reasonable :sunglasses:

[TheoPannetier]

Fair enough :)

[TheoPannetier]

We've already made a lot of progress on this last time, I'll make sure to integrate this function in the options_view class

[TheoPannetier]

Turning music off won't work yet😢 Because the options in game_view and options_view are independent, disconnected objects. Maybe we should move game_options to live at the upper level, directly in the view_manager

[TheoPannetier]

@richelbilderbeek would it be a bad idea to declare a member options in view_manager and pass it by reference to all the different views that need to edit / use the options?

[richelbilderbeek]

@TheoPannetier it's usually a bad idea to duplicate data, but your idea -indeed!- prevent that. Good thinking, but I feel the extra options is not needed; it's in the right places already.

That the options in game_view and options_view are disconnected is A Good Thing:

• The options_view create an options
• The game_view asks the options_view for its current options.

To stress the difference: game_view could have had const options m_options. An options_view does not have such a member, not even a non-const one.

Hope that that helps :+1:

[TheoPannetier]

Solved! Still a minor issue, as a result of the music living in game_view: playing/stopping music only takes place when the game window is opened, not when the music button is clicked.