Handling problem of redundant key bindings

For example, @toolbar.ruler and edit_menu.modes.ruler was
redundant in a sense that they point to the same action,
hence, each entry in the key_binding config was overriding
the other one.

To solve this, there now is a primary entry, which is the
edit_menu one. Only that entry is effective and listed
in key_bindings.

src/lay/lay/layMacroController.cc

@@ -208,11 +208,6 @@ MacroController::uninitialize (lay::Dispatcher * /*root*/)
 bool
 MacroController::configure (const std::string &key, const std::string &value)
 {
-  if (key == cfg_key_bindings) {
-    m_key_bindings = unpack_key_binding (value);
-  } else if (key == cfg_menu_items_hidden) {
-    m_menu_items_hidden = unpack_menu_items_hidden (value);
-  }
   return false;
 }
 
@@ -810,21 +805,9 @@ MacroController::do_update_menu_with_macros ()
   add_macro_items_to_menu (m_temp_macros, used_names, groups, tech);
   add_macro_items_to_menu (lym::MacroCollection::root (), used_names, groups, tech);
 
-  //  apply the custom keyboard shortcuts
+  //  apply the custom keyboard shortcuts and hidden flags
-  for (std::vector<std::pair<std::string, std::string> >::const_iterator kb = m_key_bindings.begin (); kb != m_key_bindings.end (); ++kb) {
+  mp_mw->apply_key_bindings ();
-    if (mp_mw->menu ()->is_valid (kb->first)) {
+  mp_mw->apply_hidden ();
-      lay::Action *a = mp_mw->menu ()->action (kb->first);
-      a->set_shortcut (kb->second);
-    }
-  }
-
-  //  apply the custom hidden flags
-  for (std::vector<std::pair<std::string, bool> >::const_iterator hf = m_menu_items_hidden.begin (); hf != m_menu_items_hidden.end (); ++hf) {
-    if (mp_mw->menu ()->is_valid (hf->first)) {
-      lay::Action *a = mp_mw->menu ()->action (hf->first);
-      a->set_hidden (hf->second);
-    }
-  }
 }
 
 void

src/lay/lay/layMacroController.h

@@ -249,8 +249,6 @@ private:
   tl::DeferredMethod<MacroController> dm_do_sync_with_external_sources;
   tl::DeferredMethod<MacroController> dm_sync_file_watcher;
   tl::DeferredMethod<MacroController> dm_sync_files;
-  std::vector<std::pair<std::string, std::string> > m_key_bindings;
-  std::vector<std::pair<std::string, bool> > m_menu_items_hidden;
 
   void sync_implicit_macros (bool ask_before_autorun);
   void add_macro_items_to_menu (lym::MacroCollection &collection, std::set<std::string> &used_names, std::set<std::string> &groups, const db::Technology *tech);

src/lay/lay/layMainWindow.cc

@@ -1192,8 +1192,8 @@ MainWindow::configure (const std::string &name, const std::string &value)
 
   } else if (name == cfg_menu_items_hidden) {
 
-    std::vector<std::pair<std::string, bool> > hidden = unpack_menu_items_hidden (value);
+    m_hidden = unpack_menu_items_hidden (value);
-    apply_hidden (hidden);
+    apply_hidden ();
     return true;
 
   } else if (name == cfg_initial_technology) {
@@ -1213,12 +1213,15 @@ MainWindow::configure (const std::string &name, const std::string &value)
 }
 
 void
-MainWindow::apply_hidden (const std::vector<std::pair<std::string, bool> > &hidden)
+MainWindow::apply_hidden ()
 {
-  for (std::vector<std::pair<std::string, bool> >::const_iterator hf = hidden.begin (); hf != hidden.end (); ++hf) {
+  for (std::vector<std::pair<std::string, bool> >::const_iterator hf = m_hidden.begin (); hf != m_hidden.end (); ++hf) {
-    if (menu ()->is_valid (hf->first)) {
+    lay::AbstractMenuItem *item = menu ()->find_item_exact (hf->first);
-      lay::Action *a = menu ()->action (hf->first);
+    if (item && item->primary ()) {
-      a->set_hidden (hf->second);
+      lay::Action *a = item->action ();
+      if (a) {
+        a->set_hidden (hf->second);
+      }
     }
   }
 }
@@ -1227,9 +1230,12 @@ void
 MainWindow::apply_key_bindings ()
 {
   for (std::vector<std::pair<std::string, std::string> >::const_iterator kb = m_key_bindings.begin (); kb != m_key_bindings.end (); ++kb) {
-    if (menu ()->is_valid (kb->first)) {
+    lay::AbstractMenuItem *item = menu ()->find_item_exact (kb->first);
-      lay::Action *a = menu ()->action (kb->first);
+    if (item && item->primary ()) {
-      a->set_shortcut (kb->second);
+      lay::Action *a = item->action ();
+      if (a) {
+        a->set_shortcut (kb->second);
+      }
     }
   }
 }

src/lay/lay/layMainWindow.h

@@ -534,6 +534,16 @@ public:
    */
   static std::vector<std::string> menu_symbols ();
 
+  /**
+   *  @brief For internal use: apply current key bindings
+   */
+  void apply_key_bindings ();
+
+  /**
+   *  @brief For internal use: apply hidden menu flags
+   */
+  void apply_hidden ();
+
   /**
    *  @brief Open a new layout in mode 'mode'
    *
@@ -770,6 +780,7 @@ private:
   double m_default_grid;
   bool m_default_grids_updated;
   std::vector<std::pair<std::string, std::string> > m_key_bindings;
+  std::vector<std::pair<std::string, bool> > m_hidden;
   bool m_new_layout_current_panel;
   bool m_synchronized_views;
   bool m_synchronous;
@@ -864,8 +875,6 @@ private:
   void plugin_removed (lay::PluginDeclaration *cls);
 
   void libraries_changed ();
-  void apply_key_bindings ();
-  void apply_hidden (const std::vector<std::pair <std::string, bool> > &hidden);
 };
 
 }

src/laybasic/laybasic/layAbstractMenu.cc

@@ -220,20 +220,22 @@ parse_menu_title (const std::string &s, std::string &title, std::string &shortcu
 //  AbstractMenuItem implementation
 
 AbstractMenuItem::AbstractMenuItem (Dispatcher *dispatcher)
-  : mp_action (new Action ()), mp_dispatcher (dispatcher), m_has_submenu (false), m_remove_on_empty (false)
+  : mp_action (new Action ()), mp_dispatcher (dispatcher), m_has_submenu (false), m_remove_on_empty (false), m_primary (false)
 {
   //  ... nothing yet ..
 }
 
 AbstractMenuItem::AbstractMenuItem (const AbstractMenuItem &item)
-  : mp_action (new Action ()), mp_dispatcher (item.dispatcher ()), m_has_submenu (false), m_remove_on_empty (false)
+  : mp_action (new Action ()), mp_dispatcher (item.dispatcher ()), m_has_submenu (false), m_remove_on_empty (false), m_primary (false)
 {
   //  ... nothing yet ..
 }
 
 void
-AbstractMenuItem::setup_item (const std::string &pn, const std::string &s, Action *a)
+AbstractMenuItem::setup_item (const std::string &pn, const std::string &s, Action *a, bool primary)
 {
+  m_primary = primary;
+
   m_basename.clear ();
 
   tl::Extractor ex (s.c_str ());
@@ -1587,6 +1589,8 @@ AbstractMenu::items (const std::string &path) const
 void
 AbstractMenu::insert_item (const std::string &p, const std::string &name, Action *action)
 {
+  bool primary = true;
+
   tl::Extractor extr (p.c_str ());
   while (! extr.at_end ()) {
 
@@ -1601,7 +1605,8 @@ AbstractMenu::insert_item (const std::string &p, const std::string &name, Action
       parent->children.insert (iter, AbstractMenuItem (mp_dispatcher));
       --iter;
 
-      iter->setup_item (parent->name (), name, action);
+      iter->setup_item (parent->name (), name, action, primary);
+      primary = false;
 
       //  find any items with the same name and remove them
       for (std::list<AbstractMenuItem>::iterator existing = parent->children.begin (); existing != parent->children.end (); ) {
@@ -1635,7 +1640,7 @@ AbstractMenu::insert_separator (const std::string &p, const std::string &name)
     --iter;
     Action *action = new Action ();
     action->set_separator (true);
-    iter->setup_item (parent->name (), name, action);
+    iter->setup_item (parent->name (), name, action, true);
 
   }
 
@@ -1655,7 +1660,7 @@ AbstractMenu::insert_menu (const std::string &p, const std::string &name, Action
 
     parent->children.insert (iter, AbstractMenuItem (mp_dispatcher));
     --iter;
-    iter->setup_item (parent->name (), name, action);
+    iter->setup_item (parent->name (), name, action, true);
     iter->set_has_submenu ();
 
     //  find any items with the same name and remove them
@@ -1945,7 +1950,7 @@ AbstractMenu::find_item (tl::Extractor &extr)
           if (parent) {
             parent->children.insert (iter, AbstractMenuItem (mp_dispatcher));
             --iter;
-            iter->setup_item (parent->name (), n, new Action ());
+            iter->setup_item (parent->name (), n, new Action (), true);
             iter->set_has_submenu ();
             iter->set_remove_on_empty ();
             iter->set_action_title (ndesc.empty () ? n : ndesc);
@@ -2055,16 +2060,17 @@ AbstractMenu::get_shortcuts (const std::string &root, std::map<std::string, std:
   std::vector<std::string> items = this->items (root);
   for (std::vector<std::string>::const_iterator i = items.begin (); i != items.end (); ++i) {
     if (i->size () > 0) {
-      if (is_valid (*i) && action (*i)->is_visible ()) {
+      const AbstractMenuItem *item = find_item_exact (*i);
-        if (is_menu (*i)) {
+      if (item && item->action () && item->action ()->is_visible ()) {
+        if (item->has_submenu ()) {
           //  a menu must be listed (so it can be hidden), but does not have a shortcut
           //  but we don't include special menus
           if (i->at (0) != '@') {
             bindings.insert (std::make_pair (*i, std::string ()));
           }
           get_shortcuts (*i, bindings, with_defaults);
-        } else if (! is_separator (*i)) {
+        } else if (! item->action ()->is_separator () && item->primary ()) {
-          bindings.insert (std::make_pair (*i, with_defaults ? action (*i)->get_default_shortcut () : action (*i)->get_effective_shortcut ()));
+          bindings.insert (std::make_pair (*i, with_defaults ? item->action ()->get_default_shortcut () : item->action ()->get_effective_shortcut ()));
         }
       }
     }

src/laybasic/laybasic/layAbstractMenu.h

@@ -559,7 +559,7 @@ struct LAYBASIC_PUBLIC AbstractMenuItem
   /**
    *  @brief Internal method used to set up the item
    */
-  void setup_item (const std::string &pn, const std::string &n, Action *a);
+  void setup_item (const std::string &pn, const std::string &n, Action *a, bool primary);
 
   Dispatcher *dispatcher () const
   {
@@ -616,6 +616,11 @@ struct LAYBASIC_PUBLIC AbstractMenuItem
     return m_remove_on_empty;
   }
 
+  bool primary () const
+  {
+    return m_primary;
+  }
+
   std::list <AbstractMenuItem> children;
 
 private:
@@ -623,6 +628,7 @@ private:
   Dispatcher *mp_dispatcher;
   bool m_has_submenu;
   bool m_remove_on_empty;
+  bool m_primary;
   std::string m_name;
   std::string m_basename;
   std::set<std::string> m_groups;
@@ -906,6 +912,16 @@ public:
     return m_root;
   }
 
+  /**
+   *  @brief For internal use: gets the AbstractMenuItem for a given path or 0 if the path is not valid (const version)
+   */
+  const AbstractMenuItem *find_item_exact (const std::string &path) const;
+
+  /**
+   *  @brief For internal use: gets the AbstractMenuItem for a given path or 0 if the path is not valid
+   */
+  AbstractMenuItem *find_item_exact (const std::string &path);
+
 #if defined(HAVE_QT)
 signals:
   /**
@@ -918,8 +934,6 @@ private:
   friend class Action;
 
   std::vector<std::pair<AbstractMenuItem *, std::list<AbstractMenuItem>::iterator> > find_item (tl::Extractor &extr);
-  const AbstractMenuItem *find_item_exact (const std::string &path) const;
-  AbstractMenuItem *find_item_exact (const std::string &path);
   const AbstractMenuItem *find_item_for_action (const Action *action, const AbstractMenuItem *from = 0) const;
   AbstractMenuItem *find_item_for_action (const Action *action, AbstractMenuItem *from = 0);
 #if defined(HAVE_QT)