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

Update to sleep menu 2 #38768

Merged
merged 20 commits into from
Apr 7, 2020
Merged

Conversation

Pupsi-Mupsi
Copy link
Contributor

@Pupsi-Mupsi Pupsi-Mupsi commented Mar 14, 2020

Summary

SUMMARY: None

Purpose of change

  • As @akirashirosawa suggested in More obvious hint before going to sleep (UPDATED) #38722, the default sleep option is now only NO if there is an active element in the player inventory.
  • And I've also changed the phrase to match cigars and the like because you can't turn off cigars.
  • Gas masks, musical instruments and glowsticks are ignored.
  • New FLAG SLEEP IGNORE added . Items can now be easily excluded.
  • New hint "You may want to extinguish or turn off:" (Not shown in screenshots.)

This fixes #38785.

Hope you like it.

Screenshot(s)

grafik

grafik

grafik

@BevapDin
Copy link
Contributor

Instead of moving the code blocks around (that add the menu entries), you can set the initially selected item of the menu via uilist::selected. That way you don't have to duplicate all those entries and the menu has a consistent appearance (entries always in the same order).

Copy link
Contributor

@akirashirosawa akirashirosawa left a comment

Choose a reason for hiding this comment

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

Just fix/add comments

src/handle_action.cpp Outdated Show resolved Hide resolved
src/handle_action.cpp Outdated Show resolved Hide resolved
Change comment

Co-Authored-By: akirashirosawa <[email protected]>
@Dacendeth
Copy link
Contributor

Can you fix it flaring up with gas masks? you cant actually turn those off either.

@Pupsi-Mupsi
Copy link
Contributor Author

Instead of moving the code blocks around (that add the menu entries), you can set the initially selected item of the menu via uilist::selected. That way you don't have to duplicate all those entries and the menu has a consistent appearance (entries always in the same order).

Sounds good. Can you help me with that?

@Dacendeth
Copy link
Contributor

This also just makes where yes is inconsistent, did we really need to babyproof falling asleep with a flashlight on in the first place?

@Pupsi-Mupsi
Copy link
Contributor Author

This also just makes where yes is inconsistent, did we really need to babyproof falling asleep with a flashlight on in the first place?

I feel like it might help new players. Don't you?

@akirashirosawa
Copy link
Contributor

Instead of moving the code blocks around (that add the menu entries), you can set the initially selected item of the menu via uilist::selected. That way you don't have to duplicate all those entries and the menu has a consistent appearance (entries always in the same order).

Sounds good. Can you help me with that?

2

1

diff --git a/src/handle_action.cpp b/src/handle_action.cpp
--- a/src/handle_action.cpp
+++ b/src/handle_action.cpp
@@ -903,15 +903,16 @@ static void sleep()
     uilist as_m;
     as_m.text = _( "<color_white>Are you sure you want to sleep?</color>" );
     // (Y)es/(S)ave before sleeping/(N)o
-    as_m.entries.emplace_back( 2, true,
-                               get_option<bool>( "FORCE_CAPITAL_YN" ) ? 'N' : 'n',
-                               _( "No." ) );
-    as_m.entries.emplace_back( 1, g->get_moves_since_last_save(),
-                               get_option<bool>( "FORCE_CAPITAL_YN" ) ? 'S' : 's',
-                               _( "Yes, and save game before sleeping." ) );
     as_m.entries.emplace_back( 0, true,
                                get_option<bool>( "FORCE_CAPITAL_YN" ) ? 'Y' : 'y',
                                _( "Yes." ) );
+    as_m.entries.emplace_back( 1, g->get_moves_since_last_save(),
+                               get_option<bool>( "FORCE_CAPITAL_YN" ) ? 'S' : 's',
+                               _( "Yes, and save game before sleeping." ) );
+    as_m.entries.emplace_back( 2, true,
+                               get_option<bool>( "FORCE_CAPITAL_YN" ) ? 'N' : 'n',
+                               _( "No." ) );
+
 
     // List all active items, bionics or mutations so player can deactivate them
     std::vector<std::string> active;
@@ -958,8 +959,9 @@ static void sleep()
     // ask for deactivation
     std::stringstream data;
     if( !active.empty() ) {
+        as_m.selected = 2;
         data << as_m.text << std::endl;
-        data << _( "You may want to turn off:" ) << std::endl;
+        data << _( "You may want to take care of:" ) << std::endl;
         data << " " << std::endl;
         for( auto &a : active ) {
             data << "<color_red>" << a << "</color>" << std::endl;

@Pupsi-Mupsi
Copy link
Contributor Author

Thank you akirashirosawa!

@akirashirosawa
Copy link
Contributor

Can you fix it flaring up with gas masks? you cant actually turn those off either.

It would be nice to filter things like gas masks, glowstick, but I don't know what flag we can use to do it.

@Pupsi-Mupsi
Copy link
Contributor Author

Can you fix it flaring up with gas masks? you cant actually turn those off either.

It would be nice to filter things like gas masks, glowstick, but I don't know what flag we can use to do it.

Maybe -> "use_action": "GASMASK".
As for the glow stick, I still don't have a good idea.

@Dacendeth
Copy link
Contributor

I feel new players should learn from mistakes instead of having their hand held.

@akirashirosawa
Copy link
Contributor

Thank you akirashirosawa!

You are welcome) I don’t know how to make such big suggestion in review, sorry, but, I just copied diff again.

@Pupsi-Mupsi
Copy link
Contributor Author

I feel new players should learn from mistakes instead of having their hand held.

I understand your point.
You can still go to sleep and ignore the new hint.
Or would you prefer to remove the changes?

@akirashirosawa
Copy link
Contributor

I feel new players should learn from mistakes instead of having their hand held.

I feel that I went to sleep with mp3 player on like 1000 times. And I am very happy about this change! It just makes the game more convenient, not more easy.

@akirashirosawa
Copy link
Contributor

Can you fix it flaring up with gas masks? you cant actually turn those off either.

It would be nice to filter things like gas masks, glowstick, but I don't know what flag we can use to do it.

Maybe -> "use_action": "GASMASK".
As for the glow stick, I still don't have a good idea.

We cannot hardcode such things. A gas mask and a glowstick are what came to mind now. There may be more items, may appear more items. Need to find or add a flag. But I think ignoring the warning is also a good solution too. After all, it is written, that You may want to take care of, not You should.

@Pupsi-Mupsi
Copy link
Contributor Author

Pupsi-Mupsi commented Mar 14, 2020

Can you fix it flaring up with gas masks? you cant actually turn those off either.

@Dacendeth
@akirashirosawa

Just added this to ignore GASKMASK.
grafik

@Pupsi-Mupsi
Copy link
Contributor Author

We cannot hardcode such things. A gas mask and a glowstick are what came to mind now. There may be more items, may appear more items. Need to find or add a flag. But I think ignoring the warning is also a good solution too. After all, it is written, that You may want to take care of, not You should.

You're right. We should consider adding a FLAG to filter those things.
I would prefer to move this to another PR and wait for more feedback on that matter.

@akirashirosawa
Copy link
Contributor

For note: musical instruments not a problem to sleep too.
1

And don't forget run make astyle before commit. Last commit have astyle regressions found error.

@Pupsi-Mupsi
Copy link
Contributor Author

For note: musical instruments not a problem to sleep too.
1

And don't forget run make astyle before commit. Last commit have astyle regressions found error.

You are right on both counts.
The game takes care of the instruments, but they shouldn't be part of the hint, so I'll exclude them too.

grafik

@akirashirosawa
Copy link
Contributor

Just added this to ignore GASKMASK.

Already created issue with gas mask 🙂
I think you can add fix #38785 to PR description

@ZhilkinSerg ZhilkinSerg added 0.E Content Freeze <Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc. labels Mar 15, 2020
@ZhilkinSerg ZhilkinSerg changed the base branch from master to dev March 15, 2020 11:13
@ZhilkinSerg ZhilkinSerg force-pushed the dev branch 2 times, most recently from 924f105 to 39e00e3 Compare March 18, 2020 07:00
@Pupsi-Mupsi
Copy link
Contributor Author

So, should I change anything else?
Or are we good?

I will post three more comments. Please vote with 👍and 👎. So we could find a way to deal with it. By the way - I will vote well. OK?

And please let me know if I missed anything else.

@Pupsi-Mupsi
Copy link
Contributor Author

Pupsi-Mupsi commented Mar 18, 2020

I could offer to remove the whole sentence ("You may want to take care of:").
There is already a question of whether you are really sure you want to sleep, followed by a list of items.

_I would remove the sentence.

@Pupsi-Mupsi
Copy link
Contributor Author

Pupsi-Mupsi commented Mar 18, 2020

I could create a "WARN_ON_SLEEP" flag and mark all the items that should be on the waring list.

I don't mind. So I don't vote.

@Pupsi-Mupsi
Copy link
Contributor Author

Pupsi-Mupsi commented Mar 18, 2020

The third is what should happen with the option order. Leave it as is (this PR) or change it?

grafik

I would leave it as it would be after this PR gets merged. So I vote YES,

@umamaistempo
Copy link

About the flag, I would say that even if we consider doing it, it does not have to be shipped along with this PR, so don't worry.

About the message, having any message is better than none; so between removing it and keeping it, keeping seems to me like the safest choice.

I think that we could have a more explicit message on what are the problems and solutions (ie: "turn off electronics to avoid wasting power and extinguish fires to avoid ... fires ?").

@Pupsi-Mupsi
Copy link
Contributor Author

Thanks.

@ZhilkinSerg ZhilkinSerg force-pushed the dev branch 3 times, most recently from ad63e77 to 8e68539 Compare April 1, 2020 12:18
@kevingranade kevingranade force-pushed the dev branch 4 times, most recently from 621a68e to b7106d0 Compare April 2, 2020 07:42
@ZhilkinSerg ZhilkinSerg force-pushed the dev branch 3 times, most recently from 0f30a43 to d432807 Compare April 2, 2020 12:55
@Pupsi-Mupsi
Copy link
Contributor Author

Need help resolving conflicting files src/cata_string_consts.h.

Is it enough to move
static const std::string flag_SLEEP_IGNORE( "SLEEP_IGNORE" );

to

src\handle_action.cpp

cause there is no "src/cata_string_consts.h" anymore.

@anothersimulacrum
Copy link
Member

Yes.

@ZhilkinSerg ZhilkinSerg changed the base branch from dev to master April 7, 2020 23:20
@ZhilkinSerg ZhilkinSerg merged commit 3d01ae9 into CleverRaven:master Apr 7, 2020
@Pupsi-Mupsi Pupsi-Mupsi deleted the PM-sleep-update branch April 7, 2020 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
<Bugfix> This is a fix for a bug (or closes open issue) Info / User Interface Game - player communication, menus, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Useless before-sleep warning
9 participants