Merge pull request #21921 from enen92/cguiaction_cleanup

[GUI][CGUIAction] Remove friend class, create proper interface

3 files changed, 141 insertions, 35 deletions

diff --git a/xbmc/guilib/GUIAction.cpp b/xbmc/guilib/GUIAction.cpp
index a39c4cf027..345013976e 100644
--- a/xbmc/guilib/GUIAction.cpp
+++ b/xbmc/guilib/GUIAction.cpp
@@ -14,6 +14,36 @@
 #include "ServiceBroker.h"
 #include "utils/StringUtils.h"
 
+CGUIAction::CExecutableAction::CExecutableAction(const std::string& action) : m_action{action}
+{
+}
+
+CGUIAction::CExecutableAction::CExecutableAction(const std::string& condition,
+                                                 const std::string& action)
+  : m_condition{condition}, m_action{action}
+{
+}
+
+std::string CGUIAction::CExecutableAction::GetCondition() const
+{
+  return m_condition;
+}
+
+std::string CGUIAction::CExecutableAction::GetAction() const
+{
+  return m_action;
+}
+
+bool CGUIAction::CExecutableAction::HasCondition() const
+{
+  return !m_condition.empty();
+}
+
+void CGUIAction::CExecutableAction::SetAction(const std::string& action)
+{
+  m_action = action;
+}
+
 CGUIAction::CGUIAction(int controlID)
 {
   SetNavigation(controlID);
@@ -29,10 +59,10 @@ bool CGUIAction::ExecuteActions(int controlID, int parentID, const CGUIListItemP
   std::vector<std::string> actions;
   for (const auto &i : m_actions)
   {
-    if (i.condition.empty() || infoMgr.EvaluateBool(i.condition, 0, item))
+    if (!i.HasCondition() || infoMgr.EvaluateBool(i.GetCondition(), 0, item))
     {
-      if (!StringUtils::IsInteger(i.action))
-        actions.emplace_back(i.action);
+      if (!StringUtils::IsInteger(i.GetAction()))
+        actions.emplace_back(i.GetAction());
     }
   }
   // execute them
@@ -55,10 +85,10 @@ int CGUIAction::GetNavigation() const
   CGUIInfoManager& infoMgr = CServiceBroker::GetGUI()->GetInfoManager();
   for (const auto &i : m_actions)
   {
-    if (StringUtils::IsInteger(i.action))
+    if (StringUtils::IsInteger(i.GetAction()))
     {
-      if (i.condition.empty() || infoMgr.EvaluateBool(i.condition, INFO::DEFAULT_CONTEXT))
-        return atoi(i.action.c_str());
+      if (!i.HasCondition() || infoMgr.EvaluateBool(i.GetCondition(), INFO::DEFAULT_CONTEXT))
+        return std::stoi(i.GetAction());
     }
   }
   return 0;
@@ -69,17 +99,16 @@ void CGUIAction::SetNavigation(int id)
   if (id == 0)
     return;
 
-  std::string strId = std::to_string(id);
+  const std::string strId = std::to_string(id);
   for (auto &i : m_actions)
   {
-    if (StringUtils::IsInteger(i.action) && i.condition.empty())
+    if (StringUtils::IsInteger(i.GetAction()) && !i.HasCondition())
     {
-      i.action = std::move(strId);
+      i.SetAction(std::move(strId));
       return;
     }
   }
-  m_actions.emplace_back();
-  m_actions.back().action = std::move(strId);
+  m_actions.emplace_back(std::move(strId));
 }
 
 bool CGUIAction::HasActionsMeetingCondition() const
@@ -87,8 +116,28 @@ bool CGUIAction::HasActionsMeetingCondition() const
   CGUIInfoManager& infoMgr = CServiceBroker::GetGUI()->GetInfoManager();
   for (const auto &i : m_actions)
   {
-    if (i.condition.empty() || infoMgr.EvaluateBool(i.condition, INFO::DEFAULT_CONTEXT))
+    if (!i.HasCondition() || infoMgr.EvaluateBool(i.GetCondition(), INFO::DEFAULT_CONTEXT))
       return true;
   }
   return false;
 }
+
+bool CGUIAction::HasAnyActions() const
+{
+  return m_actions.size() > 0;
+}
+
+void CGUIAction::Append(const CExecutableAction& action)
+{
+  m_actions.emplace_back(action);
+}
+
+void CGUIAction::EnableSendThreadMessageMode()
+{
+  m_sendThreadMessages = true;
+}
+
+void CGUIAction::Reset()
+{
+  m_actions.clear();
+}
diff --git a/xbmc/guilib/GUIAction.h b/xbmc/guilib/GUIAction.h
index 07ca302512..1e53038b7a 100644
--- a/xbmc/guilib/GUIAction.h
+++ b/xbmc/guilib/GUIAction.h
@@ -16,18 +16,71 @@ class CGUIControl;
 class CGUIListItem; typedef std::shared_ptr<CGUIListItem> CGUIListItemPtr;
 
 /**
- * Class containing vector of condition->(action/navigation route) pairs and handling its execution.
+ * Class containing vector of condition->(action/navigation route) and handling its execution.
  */
 class CGUIAction
 {
 public:
+  /**
+   * Class which defines an executable action
+   */
+  class CExecutableAction
+  {
+  public:
+    /**
+    * Executable action constructor (without conditional execution)
+    * @param action - The action to be executed
+    */
+    explicit CExecutableAction(const std::string& action);
+    /**
+    * Executable action constructor (providing the condition and the action)
+    * @param condition - The condition that dictates the action execution
+    * @param action - The actual action
+    */
+    CExecutableAction(const std::string& condition, const std::string& action);
+
+    /**
+    * Get the condition of this executable action (may be empty)
+    * @return condition - The condition that dictates the action execution (or an empty string)
+    */
+    std::string GetCondition() const;
+
+    /**
+    * Checks if the executable action has any condition
+    * @return true if the executable action has any condition, else false
+    */
+    bool HasCondition() const;
+
+    /**
+    * Get the action string of this executable action
+    * @return action - The action string
+    */
+    std::string GetAction() const;
+
+    /**
+    * Sets/Replaces the action string of this executable action
+    * @param action - The action string
+    */
+    void SetAction(const std::string& action);
+
+  private:
+    /**
+    * Executable action default constructor
+    */
+    CExecutableAction() = delete;
+    /* The condition that dictates the action execution */
+    std::string m_condition;
+    /* The actual action */
+    std::string m_action;
+  };
+
   CGUIAction() = default;
   explicit CGUIAction(int controlID);
 
   /**
-   * Execute actions (no navigation paths), if action is paired with condition - evaluate condition first
+   * Execute actions (no navigation paths); if action is paired with condition - evaluate condition first
    */
-  bool ExecuteActions(int controlID, int parentID, const CGUIListItemPtr &item = NULL) const;
+  bool ExecuteActions(int controlID, int parentID, const CGUIListItemPtr& item = nullptr) const;
   /**
    * Check if there is any action that meet its condition
    */
@@ -35,7 +88,7 @@ public:
   /**
    * Check if there is any action
    */
-  bool HasAnyActions() const { return m_actions.size() > 0; }
+  bool HasAnyActions() const;
   /**
    * Get navigation route that meet its conditions first
    */
@@ -44,17 +97,20 @@ public:
    * Set navigation route
    */
   void SetNavigation(int id);
-private:
-  struct cond_action_pair
-  {
-    std::string condition;
-    std::string action;
-  };
+  /**
+   * Configure CGUIAction to send threaded messages
+   */
+  void EnableSendThreadMessageMode();
+  /**
+   * Add an executable action to the CGUIAction container
+   */
+  void Append(const CExecutableAction& action);
+  /**
+   * Prune any executable actions stored in the CGUIAction
+   */
+  void Reset();
 
-  std::vector<cond_action_pair> m_actions;
+private:
+  std::vector<CExecutableAction> m_actions;
   bool m_sendThreadMessages = false;
-
-  typedef std::vector<cond_action_pair>::const_iterator ciActions;
-  typedef std::vector<cond_action_pair>::iterator iActions;
-  friend class CGUIControlFactory; // no need for setters / adders
 };
diff --git a/xbmc/guilib/GUIControlFactory.cpp b/xbmc/guilib/GUIControlFactory.cpp
index 7a3a165157..eeac91207a 100644
--- a/xbmc/guilib/GUIControlFactory.cpp
+++ b/xbmc/guilib/GUIControlFactory.cpp
@@ -490,22 +490,22 @@ bool CGUIControlFactory::GetAnimations(TiXmlNode *control, const CRect &rect, in
   return ret;
 }
 
-bool CGUIControlFactory::GetActions(const TiXmlNode* pRootNode, const char* strTag, CGUIAction& action)
+bool CGUIControlFactory::GetActions(const TiXmlNode* pRootNode,
+                                    const char* strTag,
+                                    CGUIAction& actions)
 {
-  action.m_actions.clear();
+  actions.Reset();
   const TiXmlElement* pElement = pRootNode->FirstChildElement(strTag);
   while (pElement)
   {
     if (pElement->FirstChild())
     {
-      CGUIAction::cond_action_pair pair;
-      pair.condition = XMLUtils::GetAttribute(pElement, "condition");
-      pair.action = pElement->FirstChild()->Value();
-      action.m_actions.push_back(pair);
+      actions.Append(
+          {XMLUtils::GetAttribute(pElement, "condition"), pElement->FirstChild()->Value()});
     }
     pElement = pElement->NextSiblingElement(strTag);
   }
-  return action.m_actions.size() > 0;
+  return actions.HasAnyActions();
 }
 
 bool CGUIControlFactory::GetHitRect(const TiXmlNode *control, CRect &rect, const CRect &parentRect)
@@ -922,7 +922,8 @@ CGUIControl* CGUIControlFactory::Create(int parentID, const CRect &rect, TiXmlEl
   GetActions(pControlNode, "ontextchange", textChangeActions);
   GetActions(pControlNode, "onfocus", focusActions);
   GetActions(pControlNode, "onunfocus", unfocusActions);
-  focusActions.m_sendThreadMessages = unfocusActions.m_sendThreadMessages = true;
+  focusActions.EnableSendThreadMessageMode();
+  unfocusActions.EnableSendThreadMessageMode();
   GetActions(pControlNode, "altclick", altclickActions);
 
   std::string infoString;