Fix UCI 'button' options

When a button fires UCIOption::operator=() is called and from
there the on_change() function. Now it happens that in case of
a button the on_change() function resets option's value to
"false" triggering again UCIOption::operator=() that calls again
on_change() and so on in an endless loop that is experienced
by the user as an application hang.

Rework the button logic to fix the issue and also be more clear
about how button works.

Reported by several people working with Scid and tracked down
to the "Clear Hash" UCI button by Steven Atkinson.

Bug recently introduced by 2ef5b4066e649.

No functional change.

Signed-off-by: Marco Costalba <mcostalba@gmail.com>

diff --git a/src/ucioption.cpp b/src/ucioption.cpp
index 51987d03c9d01c5ee05b4332d156de47fecf830a..75eb205313124669de33679a1e8434506e6438c7 100644 (file)
--- a/src/ucioption.cpp
+++ b/src/ucioption.cpp
@@ -33,10 +33,10 @@ OptionsMap Options; // Global object
 namespace {
 
 /// 'On change' actions, triggered by an option's value change
 namespace {
 
 /// 'On change' actions, triggered by an option's value change

-void on_eval(UCIOption&) { Eval::init(); }
-void on_threads(UCIOption&) { Threads.read_uci_options(); }
-void on_hash_size(UCIOption& o) { TT.set_size(o); }
-void on_clear_hash(UCIOption& o) { TT.clear(); o = false; } // UCI button
+void on_eval(const UCIOption&) { Eval::init(); }
+void on_threads(const UCIOption&) { Threads.read_uci_options(); }
+void on_hash_size(const UCIOption& o) { TT.set_size(o); }
+void on_clear_hash(const UCIOption&) { TT.clear(); }

 
 /// Our case insensitive less() function as required by UCI protocol
 bool ci_less(char c1, char c2) { return tolower(c1) < tolower(c2); }
 
 /// Our case insensitive less() function as required by UCI protocol
 bool ci_less(char c1, char c2) { return tolower(c1) < tolower(c2); }


@@ -74,7 +74,7 @@ OptionsMap::OptionsMap() {
   o["Threads"]                     = UCIOption(cpus, 1, MAX_THREADS);
   o["Use Sleeping Threads"]        = UCIOption(true, on_threads);
   o["Hash"]                        = UCIOption(32, 4, 8192, on_hash_size);
   o["Threads"]                     = UCIOption(cpus, 1, MAX_THREADS);
   o["Use Sleeping Threads"]        = UCIOption(true, on_threads);
   o["Hash"]                        = UCIOption(32, 4, 8192, on_hash_size);

-  o["Clear Hash"]                  = UCIOption(false, on_clear_hash);
+  o["Clear Hash"]                  = UCIOption(on_clear_hash);

   o["Ponder"]                      = UCIOption(true);
   o["OwnBook"]                     = UCIOption(true);
   o["MultiPV"]                     = UCIOption(1, 1, 500);
   o["Ponder"]                      = UCIOption(true);
   o["OwnBook"]                     = UCIOption(true);
   o["MultiPV"]                     = UCIOption(1, 1, 500);


@@ -121,6 +121,9 @@ UCIOption::UCIOption(const char* v, Fn* f) : type("string"), min(0), max(0), idx
 UCIOption::UCIOption(bool v, Fn* f) : type("check"), min(0), max(0), idx(Options.size()), on_change(f)
 { defaultValue = currentValue = (v ? "true" : "false"); }
 
 UCIOption::UCIOption(bool v, Fn* f) : type("check"), min(0), max(0), idx(Options.size()), on_change(f)
 { defaultValue = currentValue = (v ? "true" : "false"); }
 

+UCIOption::UCIOption(Fn* f) : type("button"), min(0), max(0), idx(Options.size()), on_change(f)
+{ defaultValue = currentValue = "false"; }
+

 UCIOption::UCIOption(int v, int minv, int maxv, Fn* f) : type("spin"), min(minv), max(maxv), idx(Options.size()), on_change(f)
 { std::ostringstream ss; ss << v; defaultValue = currentValue = ss.str(); }
 
 UCIOption::UCIOption(int v, int minv, int maxv, Fn* f) : type("spin"), min(minv), max(maxv), idx(Options.size()), on_change(f)
 { std::ostringstream ss; ss << v; defaultValue = currentValue = ss.str(); }
 


@@ -134,12 +137,15 @@ void UCIOption::operator=(const string& v) {
   assert(!type.empty());
 
   if (   !v.empty()
   assert(!type.empty());
 
   if (   !v.empty()

-      && (type == "check") == (v == "true" || v == "false")
+      && (type == "check" || type == "button") == (v == "true" || v == "false")

       && (type != "spin" || (atoi(v.c_str()) >= min && atoi(v.c_str()) <= max)))
   {
       currentValue = v;
 
       if (on_change)
           (*on_change)(*this);
       && (type != "spin" || (atoi(v.c_str()) >= min && atoi(v.c_str()) <= max)))
   {
       currentValue = v;
 
       if (on_change)
           (*on_change)(*this);

+
+      if (type == "button")
+          currentValue = "false";

   }
 }
   }
 }

diff --git a/src/ucioption.h b/src/ucioption.h
index cde832318ca740ffea9bdbb4c29bc7fe211883ea..7af54739ba437c7b908c922d7c7c158b07d5acd2 100644 (file)
--- a/src/ucioption.h
+++ b/src/ucioption.h
@@ -30,12 +30,12 @@ struct OptionsMap;
 /// UCIOption class implements an option as defined by UCI protocol
 class UCIOption {
 
 /// UCIOption class implements an option as defined by UCI protocol
 class UCIOption {
 

-  typedef void (Fn)(UCIOption&);
+  typedef void (Fn)(const UCIOption&);

 
 public:
 
 public:

-  UCIOption() {} // Required by std::map::operator[]
-  UCIOption(const char* v, Fn* = NULL);
+  UCIOption(Fn* = NULL);

   UCIOption(bool v, Fn* = NULL);
   UCIOption(bool v, Fn* = NULL);

+  UCIOption(const char* v, Fn* = NULL);

   UCIOption(int v, int min, int max, Fn* = NULL);
 
   void operator=(const std::string& v);
   UCIOption(int v, int min, int max, Fn* = NULL);
 
   void operator=(const std::string& v);