Safemode UI #17916

OzoneH3 · Aug 4, 2016

This PR implements a autopickup like UI for white-/blacklisting safemode monsters.
If no rules are specified, the old safemode checks will be used.

Todo:

Check NPCs agains rules (rule name = human)
Added whitelistung with ~ (stolen from enable debug mode)
Option to blacklist monsters via the V menu monster list

The example below would:

Trigger when any hostile monster is withing 30 tiles
Exclude hostile zombie (exact word match) from the whitelist
Trigger when a neutral moose is within viewdistance

mugling · Aug 4, 2016

One possible addition would be to allow ignoring specific monsters from the interruption prompt?

OzoneH3 · Aug 4, 2016

Ignoring is already implemented in the current safemode.

Or do you mean something different?

mugling · Aug 4, 2016

The message that interrupts long activities (especially crafting)

mugling · Aug 4, 2016

@OzoneH3 could possible be implemented via the critter tracker?

OzoneH3 · Aug 5, 2016

I'll try to figure out the long activity interrution in another PR. It's not really related with the ui.
From what I've seen it shouldn't be too hard.

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

data/raw/keybindings.json

@@ -1710,6 +1862,7 @@
    { "type":"keybinding", "name": "Control Vehicle", "category":"DEFAULTMODE", "id":"control_vehicle", "bindings":[ { "input_method":"keyboard", "key":"^" } ] },
    { "type":"keybinding", "name": "Toggle Safemode", "category":"DEFAULTMODE", "id":"safemode", "bindings":[ { "input_method":"keyboard", "key":"!" } ] },
    { "type":"keybinding", "name": "Ignore Nearby Enemy", "category":"DEFAULTMODE", "id":"ignore_enemy", "bindings":[ { "input_method":"keyboard", "key":"'" } ] },
+    { "type":"keybinding", "name": "Whitelist enemy", "category":"DEFAULTMODE", "id":"whitelist_enemy", "bindings":[ { "input_method":"keyboard", "key":"~" } ] },


This should replace "ignore" bind.

Why? You can still ignore enemys without whitelisting them forever.

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/creature.cpp

@@ -1542,3 +1542,23 @@ void Creature::check_dead_state() {
        die( nullptr );
    }
 }
+
+std::pair<std::string, nc_color> const &Creature::get_creature_attitude( Attitude att )


This name is very misleading. Should be something like attitude_name or attitude_ui_data.

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/game.cpp

@@ -11556,14 +11623,21 @@ bool game::check_safe_mode_allowed( bool repeat_safe_mode_warnings )
    if( new_seen_mon.empty() ) {
        // naming consistent with code in game::mon_info
        spotted_creature_name = _( "a hostile survivor" );
+        get_safemode().whitelist = "human";


Would be useful if you could also trigger it on neutral survivors. They can become hostile, steal stuff etc.

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/safemode_ui.cpp

+    const int iContentHeight = FULL_SCREEN_HEIGHT - 2 - iHeaderHeight;
+
+    const int iOffsetX = ( TERMX > FULL_SCREEN_WIDTH ) ? ( TERMX - FULL_SCREEN_WIDTH ) / 2 : 0;
+    const int iOffsetY = ( TERMY > FULL_SCREEN_HEIGHT ) ? ( TERMY - FULL_SCREEN_HEIGHT ) / 2 : 0;


Are you copy+pasting the UI from somewhere?

It would be a good idea to clean it up. For example, make it cover the whole screen instead of just the middle.

It's a copy of the autopickup ui. Shouldn't it stay in line with the other option menus?

Both should cover the whole screen eventually. It doesn't need to be in this PR, though.

Well, all 4 of these are copies: options -> autopickup -> color manager -> safmode

Do they really need to fill the whole screen though?

They don't need to, but it can be useful when there are many entries. Options and color manager could sure use it, I don't use autopickup except for NPCs so I don't know how big should it be.

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/safemode_ui.cpp

+                           ( iLine == i && iColumn == col_IE ) ? hilite( cLineColor ) : cLineColor,
+                           "%s", ( ( vRules[iTab][i].bExclude ) ? rm_prefix( _( "Exclude" ) ).c_str() : rm_prefix(
+                                       _( "Include" ) ).c_str() )
+                         );


This code is hard to read. The non-standard naming scheme (hungarian notation itself, hungarian notation being mixed with non-hungarian notation) doesn't help.

Could be cleaned up by:

Extracting a lambda to cover all 3 mvwprintz without a lot of repetition (wouldn't be easy, but doable)

Extracting vRules[iTab][i] to a variable like current_tab

Extracting some of the inlined data to named variables, to show what is being displayed

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/safemode_ui.cpp

+    tmpx += shortcut_print( w_header, 0, tmpx, c_white, c_ltgreen, _( "<M>ove" ) ) + 2;
+    tmpx += shortcut_print( w_header, 0, tmpx, c_white, c_ltgreen, _( "<E>nable" ) ) + 2;
+    tmpx += shortcut_print( w_header, 0, tmpx, c_white, c_ltgreen, _( "<D>isable" ) ) + 2;
+    shortcut_print( w_header, 0, tmpx, c_white, c_ltgreen, _( "<T>est" ) );


A lot of nearly identical entries, consider a vector+loop (easier to maintain).

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/safemode_ui.cpp

+    ctxt.register_action( "MOVE_RULE_UP" );
+    ctxt.register_action( "MOVE_RULE_DOWN" );
+    ctxt.register_action( "TEST_RULE" );
+    ctxt.register_action( "HELP_KEYBINDINGS" );


Side note here:
We should probably have a register_action overload that takes a container argument.

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/safemode_ui.cpp

+                    wprintz( w, c_yellow, ">> " );
+                } else {
+                    wprintz( w, c_yellow, "   " );
+                }


Could be wprintz( w, c_yellow, iLine == i ? ">> " : " " );

Coolthulhu · Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu · Aug 5, 2016

src/safemode_ui.h

+                bool bActive;
+                bool bExclude;
+                int attCreature;
+                int iProxyDist;


Is this variable supposed to be proximity distance? proxy and proximity aren't highly related words, it would be better to use just proximity or distance.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+        // display safemode
+        for( int i = start_pos; i < ( int )creature_list.size(); i++ ) {
+            if( i >= start_pos &&
+                i < start_pos + ( ( content_height > ( int )creature_list.size() ) ?


Looks like this is supposed to get the content_height or creature_list.size(), whatever is smaller. If so, it should be std::min(static_cast<int>(creature_list.size()), content_height) as that's easier to reason about.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+    int nmatch = creature_list.size();
+    std::string buf = string_format( ngettext( "%1$d monster matches: %2$s",
+                                     "%1$d monsters match: %2$s",
+                                     nmatch ), nmatch, temp_rules[row_in].rule.c_str() );


Let mvprintz do the formatting.

This is needed for utf8_width(...) to work in the next line.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+void safemode::test_pattern( const int tab_in, const int row_in )
+{
+    std::vector<std::string> creature_list;
+    std::string creature_name = "";


I'm wondering why people write this explicit initialization for strings, but use the default constructor for the vector (above). The = "" is entirely redundant here.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+                auto &temp_rules_from = ( tab == GLOBAL_TAB ) ? global_rules : character_rules;
+                auto &temp_rules_to = ( tab == GLOBAL_TAB ) ? character_rules : global_rules;
+
+                temp_rules_to.push_back( rules_class(


Looks like you're copying the rules_class object. Why not use the copy constructor for that?

temp_rules_to.push_back(temp_rules_from[...]);

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/output.h

+int ci_find_substr( const charT &str1, const charT &str2, const std::locale &loc = std::locale() );
+// templated version of ci_equal so it could work with both char and wchar_t
+template<typename charT>
+struct ci_equal {


We really don't need the generic templated version of that function. charT will never be anything but char. Same with the function above.

In fact, this will not currently even work with anything else but char. Try to call ci_find_substr<int>(...) (with suitable dummy arguments) from a source file other than output.cpp. The linker will complain that can not find the function ci_find_substr<int>: only the function ci_find_substr<char> is (implicitly) instantiated in output.cpp (as it's called from another functions).

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/output.cpp

+            }
+        } else { //inbetween: vPat[i]
+            if (*it != "") {
+                if ((iPos = (int)ci_find_substr(sText, *it)) == -1) {


ci_find_substr returns an int - why is there a cast to it?

Also in the same line: assignment in if is bad practice and saves only one line.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/output.cpp

+    }
+
+    for (auto it = vPattern.begin(); it != vPattern.end(); ++it) {
+        if (it == vPattern.begin() && *it != "") { //beginning: ^vPat[i]


In the comment: what's vPat[i]? Makes no sense.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/output.cpp

+
+    //case insenitive search
+
+    /* Possible patterns


This kind of comment belongs into the function documentation in the header. None of those functions are currently documented at all.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/game.cpp

+
+    std::string whitelist = "";
+    if ( !get_safemode().empty() ) {
+        whitelist = string_format( " or %s to whitelist the monster", press_x( ACTION_WHITELIST_ENEMY ).c_str() );


Missing translation macro.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/creature.cpp

+        }
+    };
+
+    if( ( int ) att < 0 || ( int ) att > 4 ) {


Use >= strings.size() and either strings.back() or strings[strings.size() - 1] instead of the magic number.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/game.cpp

+        case ACTION_WHITELIST_ENEMY:
+            if ( safe_mode == SAFE_MODE_STOP && !get_safemode().empty() ) {
+                get_safemode().add_rule( get_safemode().whitelist, Creature::A_ANY, 0, RULE_WHITELISTED );
+                add_msg( m_info, string_format( _( "Creature whitelisted: %s" ), get_safemode().whitelist.c_str() ).c_str() );


Let add_msg do the formatting.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/game.cpp

-                if (rl_dist( u.pos(), p->pos() ) <= iProxyDist) {
+            //Safemode NPC check
+
+            const int mondist = rl_dist( u.pos(), p->pos() );


Looks like a repetition of the code above for the monster (only obvious difference is "human" instead of critter.name()), even the variable mondist (should be npcdist, shouldn't it?) - this figuratively screams to be put into a function/lambda.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/game.cpp

-                            sText = npc_attitude_name( p->attitude );
-                            color = p->symbol_color();
+                        std::string sSafemode = _("<A>dd do safemode Blacklist");
+                        const std::string monName = (bTypeNPC) ? "human" : m->name();


A small helper function (probably in the safemode ui class) that translates a const Creature & into this string (name() for monsters and "human" for NPCs) would make this a bit simpler and safer.

Coolthulhu · Aug 12, 2016

Coolthulhu reviewed Aug 12, 2016

View changes

Coolthulhu · Aug 12, 2016

src/output.cpp

+                return false;
+            }
+        } else { //inbetween: vPat[i]
+            if (*it != "") {


!it->empty()

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+            }
+        } else if( action == "QUIT" ) {
+            break;
+        } else if( tab == CHARACTER_TAB && g->u.name.empty() ) {


Now I'm wondering why this tests for empty(), while other code in this PR uses == "". Have the same behavior, but using empty() is generic and states the intend clearer.

Anyway, this is inconsistent. (Through the problem is not just here.)

Fixed all of those.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+        }
+
+        auto &current_tab = ( tab == GLOBAL_TAB ) ? global_rules : character_rules;
+        const bool current_tab_non_empty = !current_tab.empty();


What's the advantages of using this variable instead of !current_tab.empty() directly? The later is always up to date, while this variable can easily be wrong:

const bool is_non_empty = !foo.empty(); ... foo.clear(); ... if(is_non_empty) { foo[0]; // boom }

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+        for( int i = start_pos; i < ( int )current_tab.size(); i++ ) {
+            if( i >= start_pos &&
+                i < start_pos + ( ( content_height > ( int )current_tab.size() ) ?
+                                  ( int )current_tab.size() : content_height ) ) {


Looks like another place for using std::min.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+                        attitude = Creature::A_ANY;
+                        break;
+                    case Creature::A_ANY:
+                    default:


The default case should be avoided when switching over an enumeration value. That allows the compiler to warn you when an enumeration value has not been handled (e.g. when somebody added a new value to the enumeration without updating the switch). The default case suppresses those warnings.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+                } else {
+                    //Let the options class handle the validity of the new value
+                    auto temp_option = get_options().get_option( "SAFEMODEPROXIMITY" );
+                    temp_option.setValue( text.c_str() );


setValue accepts a std::string, no need to call c_str() here.

BevapDin · Aug 12, 2016

BevapDin reviewed Aug 12, 2016

View changes

BevapDin · Aug 12, 2016

src/safemode_ui.cpp

+
+                wprintz( w_test_rule_content, c_yellow, ( line == i ) ? ">> " : "   " );
+
+                wprintz( w_test_rule_content, ( line == i ) ? hilite( line_color ) : line_color,


Should be "%s", creature_list[i].c_str()); - just in case the monster name contains a '%'.

OzoneH3 · Aug 12, 2016

No time to fix this in the forseeable future.

OzoneH3 force-pushed the OzoneH3:safemode_ui branch to db5fc7d Aug 5, 2016

Fix whitelist trigger.

8f3aa39

OzoneH3 changed the title ~~[WiP][CR] Safemode UI~~ Safemode UI Aug 5, 2016

Only allow/show white/blacklisting if there are rules present.

Loading status checks…

eacc599

fix misleading function name

f24f4df

Fix misleading text.

Loading status checks…

2f01f06

OzoneH3 closed this Aug 12, 2016

Fixes

Loading status checks…

7deed01

OzoneH3 reopened this Aug 22, 2016

OzoneH3 added some commits Aug 22, 2016

Renamed include/exclude to white/blacklist

Loading status checks…

5e9c11d

row display hotfix

Loading status checks…

8ff866e

Rivet-the-Zombie merged commit 47575b0 into CleverRaven:master Aug 27, 2016
1 check passed

1 check passed

default This has been rescheduled for testing as the 'master' branch has been updated.

OzoneH3 referenced this pull request Aug 29, 2016
Closed
'Safemode' or 'Safe mode' #18148

OzoneH3 deleted the OzoneH3:safemode_ui branch Sep 6, 2016

CleverRaven/Cataclysm-DDA

Join GitHub today

Safemode UI #17916

Conversation

OzoneH3 commented Aug 4, 2016 • edited

This comment has been minimized.

mugling commented Aug 4, 2016

This comment has been minimized.

OzoneH3 commented Aug 4, 2016

This comment has been minimized.

mugling commented Aug 4, 2016

This comment has been minimized.

mugling commented Aug 4, 2016

OzoneH3 added some commits Aug 2, 2016

OzoneH3 force-pushed the OzoneH3:safemode_ui branch to db5fc7d Aug 5, 2016

OzoneH3 changed the title [WiP][CR] Safemode UI Safemode UI Aug 5, 2016

This comment has been minimized.

OzoneH3 commented Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Aug 5, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

Coolthulhu reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

BevapDin reviewed Aug 12, 2016 View changes

This comment has been minimized.

OzoneH3 commented Aug 4, 2016 •

edited

OzoneH3 force-pushed the OzoneH3:safemode_ui branch to `db5fc7d` Aug 5, 2016

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

Coolthulhu reviewed Aug 5, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

Coolthulhu reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

BevapDin reviewed Aug 12, 2016

View changes

Rivet-the-Zombie merged commit `47575b0` into CleverRaven:master Aug 27, 2016
1 check passed