[PVR] Group Manager: Fix persistence issues.

9 files changed, 249 insertions, 68 deletions

diff --git a/xbmc/pvr/channels/PVRChannelGroup.cpp b/xbmc/pvr/channels/PVRChannelGroup.cpp
index af4a04d29c..cc4bd649fa 100644
--- a/xbmc/pvr/channels/PVRChannelGroup.cpp
+++ b/xbmc/pvr/channels/PVRChannelGroup.cpp
@@ -886,6 +886,11 @@ void CPVRChannelGroup::Delete()
   }
 }
 
+void CPVRChannelGroup::DeleteGroupMember(const std::shared_ptr<CPVRChannelGroupMember>& member)
+{
+  DeleteGroupMembersFromDb({member});
+}
+
 bool CPVRChannelGroup::Renumber(RenumberMode mode /* = NORMAL */)
 {
   bool bReturn(false);
@@ -1022,7 +1027,7 @@ std::string CPVRChannelGroup::GroupName() const
   return m_path.GetGroupName();
 }
 
-void CPVRChannelGroup::SetGroupName(const std::string& strGroupName,
+bool CPVRChannelGroup::SetGroupName(const std::string& strGroupName,
                                     bool isUserSetName /* = false */)
 {
   std::unique_lock<CCriticalSection> lock(m_critSection);
@@ -1038,11 +1043,10 @@ void CPVRChannelGroup::SetGroupName(const std::string& strGroupName,
     }
 
     if (m_bLoaded)
-    {
       m_bChanged = true;
-      Persist(); //! @todo why must we persist immediately?
-    }
+    return true;
   }
+  return false;
 }
 
 std::string CPVRChannelGroup::ClientGroupName() const
@@ -1140,9 +1144,9 @@ bool CPVRChannelGroup::SetHidden(bool bHidden)
     m_bHidden = bHidden;
     if (m_bLoaded)
       m_bChanged = true;
+    return true;
   }
-
-  return m_bChanged;
+  return false;
 }
 
 bool CPVRChannelGroup::IsHidden() const
@@ -1157,7 +1161,7 @@ int CPVRChannelGroup::GetPosition() const
   return m_iPosition;
 }
 
-void CPVRChannelGroup::SetPosition(int iPosition)
+bool CPVRChannelGroup::SetPosition(int iPosition)
 {
   std::unique_lock<CCriticalSection> lock(m_critSection);
 
@@ -1166,7 +1170,9 @@ void CPVRChannelGroup::SetPosition(int iPosition)
     m_iPosition = iPosition;
     if (m_bLoaded)
       m_bChanged = true;
+    return true;
   }
+  return false;
 }
 
 int CPVRChannelGroup::GetClientPosition() const
diff --git a/xbmc/pvr/channels/PVRChannelGroup.h b/xbmc/pvr/channels/PVRChannelGroup.h
index d660f14ec0..4582a273a3 100644
--- a/xbmc/pvr/channels/PVRChannelGroup.h
+++ b/xbmc/pvr/channels/PVRChannelGroup.h
@@ -156,8 +156,9 @@ public:
    * @brief Change the name of this group.
    * @param strGroupName The new group name.
    * @param isUserSetName Whether the name was set by the user.
+   * @return True if the group name was changed, false otherwise.
    */
-  void SetGroupName(const std::string& strGroupName, bool isUserSetName = false);
+  bool SetGroupName(const std::string& strGroupName, bool isUserSetName = false);
 
   /*!
    * @brief Set the name this group has on the client.
@@ -381,7 +382,17 @@ public:
    */
   std::shared_ptr<CPVRChannelGroupMember> GetByUniqueID(const std::pair<int, int>& id) const;
 
+  /*!
+   * @brief Set the hidden state of this group.
+   * @param bHidden True to set hidden state, false to unhide the group.
+   * @return True if hidden state was changed, false otherwise.
+   */
   bool SetHidden(bool bHidden);
+
+  /*!
+   * @brief Check whether this group is hidden.
+   * @return True if group is hidden, false otherwise.
+   */
   bool IsHidden() const;
 
   /*!
@@ -393,8 +404,9 @@ public:
   /*!
    * @brief Set the local position of this group.
    * @param iPosition The new local group position.
+   * @return True if position has changed, false otherwise.
    */
-  void SetPosition(int iPosition);
+  bool SetPosition(int iPosition);
 
   /*!
    * @brief Get the position of this group as supplied by the PVR client.
@@ -436,6 +448,12 @@ public:
   void Delete();
 
   /*!
+   * @brief Remove the given group member from the database.
+   * @param member The member to remove from the database.
+   */
+  void DeleteGroupMember(const std::shared_ptr<CPVRChannelGroupMember>& member);
+
+  /*!
    * @brief Whether this group is deleted.
    * @return True, if deleted, false otherwise.
    */
diff --git a/xbmc/pvr/channels/PVRChannelGroupAllChannels.cpp b/xbmc/pvr/channels/PVRChannelGroupAllChannels.cpp
index fceaf3f7dc..708577c2d9 100644
--- a/xbmc/pvr/channels/PVRChannelGroupAllChannels.cpp
+++ b/xbmc/pvr/channels/PVRChannelGroupAllChannels.cpp
@@ -49,7 +49,10 @@ void CPVRChannelGroupAllChannels::CheckGroupName()
 
   // Ensure the group name is still correct, or channels may fail to load after a locale change
   if (!IsUserSetName())
-    SetGroupName(g_localizeStrings.Get(19287));
+  {
+    if (SetGroupName(g_localizeStrings.Get(19287)))
+      Persist();
+  }
 }
 
 bool CPVRChannelGroupAllChannels::UpdateFromClients(
diff --git a/xbmc/pvr/channels/PVRChannelGroups.cpp b/xbmc/pvr/channels/PVRChannelGroups.cpp
index add597cf10..79a5283eb9 100644
--- a/xbmc/pvr/channels/PVRChannelGroups.cpp
+++ b/xbmc/pvr/channels/PVRChannelGroups.cpp
@@ -605,9 +605,23 @@ std::shared_ptr<CPVRChannelGroup> CPVRChannelGroups::GetNextGroup(
   return GetFirstGroup();
 }
 
+void CPVRChannelGroups::GroupStateChanged(const std::shared_ptr<CPVRChannelGroup>& group,
+                                          GroupState state /* = GroupState::CHANGED */)
+{
+  if (state == GroupState::DELETED)
+  {
+    if (group->GroupID() > 0)
+      group->Delete(); // delete the group from the database
+  }
+  else
+    group->Persist();
+
+  CServiceBroker::GetPVRManager().PublishEvent(PVREvent::ChannelGroupsInvalidated);
+}
+
 std::shared_ptr<CPVRChannelGroup> CPVRChannelGroups::AddGroup(const std::string& strName)
 {
-  bool bPersist(false);
+  bool changed{false};
   std::shared_ptr<CPVRChannelGroup> group;
 
   {
@@ -615,22 +629,18 @@ std::shared_ptr<CPVRChannelGroup> CPVRChannelGroups::AddGroup(const std::string&
 
     // check if there's another local group with the same name already
     group = GetGroupByName(strName, PVR_GROUP_CLIENT_ID_LOCAL, Exclude::NONE);
-    if (!group)
+    if (!group || group->GetOrigin() != CPVRChannelGroup::Origin::USER)
     {
       // create a new local group
       group = GetGroupFactory()->CreateUserGroup(IsRadio(), strName, GetGroupAll());
 
       m_groups.emplace_back(group);
-      SortGroups();
-      bPersist = true;
-
-      CServiceBroker::GetPVRManager().PublishEvent(PVREvent::ChannelGroupsInvalidated);
+      changed = true;
     }
   }
 
-  // persist in the db if a new group was added
-  if (bPersist)
-    group->Persist();
+  if (changed)
+    GroupStateChanged(group);
 
   return group;
 }
@@ -643,7 +653,7 @@ bool CPVRChannelGroups::DeleteGroup(const std::shared_ptr<CPVRChannelGroup>& gro
     return false;
   }
 
-  bool bFound(false);
+  bool changed{false};
 
   // delete the group in this container
   {
@@ -653,35 +663,116 @@ bool CPVRChannelGroups::DeleteGroup(const std::shared_ptr<CPVRChannelGroup>& gro
       if (*it == group || (group->GroupID() > 0 && (*it)->GroupID() == group->GroupID()))
       {
         m_groups.erase(it);
-        bFound = true;
+        changed = true;
         break;
       }
     }
   }
 
-  if (bFound && group->GroupID() > 0)
+  if (changed)
+    GroupStateChanged(group, GroupState::DELETED);
+
+  return changed;
+}
+
+bool CPVRChannelGroups::HideGroup(const std::shared_ptr<CPVRChannelGroup>& group, bool bHide)
+{
+  if (group && group->SetHidden(bHide))
   {
-    // delete the group from the database
-    group->Delete();
-    CServiceBroker::GetPVRManager().PublishEvent(PVREvent::ChannelGroupsInvalidated);
+    GroupStateChanged(group);
+    return true;
   }
-  return bFound;
+  return false;
 }
 
-bool CPVRChannelGroups::HideGroup(const std::shared_ptr<CPVRChannelGroup>& group, bool bHide)
+bool CPVRChannelGroups::SetGroupName(const std::shared_ptr<CPVRChannelGroup>& group,
+                                     const std::string& newGroupName,
+                                     bool isUserSetName)
+{
+  if (group && group->SetGroupName(newGroupName, isUserSetName))
+  {
+    GroupStateChanged(group);
+    return true;
+  }
+  return false;
+}
+
+bool CPVRChannelGroups::AppendToGroup(
+    const std::shared_ptr<CPVRChannelGroup>& group,
+    const std::shared_ptr<const CPVRChannelGroupMember>& groupMember)
 {
-  bool bReturn = false;
+  if (group)
+  {
+    if (!group->SupportsMemberAdd())
+    {
+      CLog::LogF(LOGERROR, "Channel group {} does not support adding members", group->GroupName());
+      return false;
+    }
+
+    if (group->AppendToGroup(groupMember))
+    {
+      GroupStateChanged(group);
+      return true;
+    }
+  }
+  return false;
+}
 
+bool CPVRChannelGroups::RemoveFromGroup(const std::shared_ptr<CPVRChannelGroup>& group,
+                                        const std::shared_ptr<CPVRChannelGroupMember>& groupMember)
+{
   if (group)
   {
-    if (group->SetHidden(bHide))
+    if (!group->SupportsMemberRemove())
+    {
+      CLog::LogF(LOGERROR, "Channel group {} does not support removing members",
+                 group->GroupName());
+      return false;
+    }
+
+    if (group->RemoveFromGroup(groupMember))
+    {
+      group->DeleteGroupMember(groupMember);
+      GroupStateChanged(group);
+      return true;
+    }
+  }
+  return false;
+}
+
+bool CPVRChannelGroups::ResetGroupPositions(const std::vector<std::string>& sortedGroupPaths)
+{
+  static constexpr int START_POSITION{1};
+  int pos{START_POSITION};
+
+  bool success{true};
+  bool changed{false};
+
+  for (const auto& path : sortedGroupPaths)
+  {
+    const auto group{GetGroupByPath(path)};
+    if (!group)
+    {
+      CLog::LogFC(LOGERROR, LOGPVR, "Unable to obtain group with path '{}‘, Skipping it.", path);
+      success = false;
+      continue;
+    }
+
+    if (group->SetPosition(pos++))
     {
       // state changed
-      CServiceBroker::GetPVRManager().PublishEvent(PVREvent::ChannelGroupsInvalidated);
+      changed = true;
     }
-    bReturn = true;
   }
-  return bReturn;
+
+  if (changed)
+  {
+    PersistAll();
+    SortGroups();
+    CServiceBroker::GetPVRManager().PublishEvent(PVREvent::ChannelGroupsInvalidated);
+  }
+
+  return success;
 }
 
 int CPVRChannelGroups::CleanupCachedImages()
diff --git a/xbmc/pvr/channels/PVRChannelGroups.h b/xbmc/pvr/channels/PVRChannelGroups.h
index b787c62d6f..a3fc1fd754 100644
--- a/xbmc/pvr/channels/PVRChannelGroups.h
+++ b/xbmc/pvr/channels/PVRChannelGroups.h
@@ -192,6 +192,42 @@ public:
   bool HideGroup(const std::shared_ptr<CPVRChannelGroup>& group, bool bHide);
 
   /*!
+   * @brief Change the name of the given group.
+   * @param group The group.
+   * @param newGroupName The new group name.
+   * @param isUserSetName Whether the name was set by the user.
+   * @return True if the group name was changed, false otherwise.
+   */
+  bool SetGroupName(const std::shared_ptr<CPVRChannelGroup>& group,
+                    const std::string& newGroupName,
+                    bool isUserSetName);
+
+  /*!
+   * @brief Append a channel group member to the given group.
+   * @param group The group.
+   * @param groupMember The channel group member to append.
+   * @return True if the channel group member was appended, false otherwise.
+   */
+  bool AppendToGroup(const std::shared_ptr<CPVRChannelGroup>& group,
+                     const std::shared_ptr<const CPVRChannelGroupMember>& groupMember);
+
+  /*!
+   * @brief Remove a channel group member from the given group.
+   * @param group The group.
+   * @param groupMember The channel group member to remove.
+   * @return @return True if the channel group member was removed, false otherwise.
+   */
+  bool RemoveFromGroup(const std::shared_ptr<CPVRChannelGroup>& group,
+                       const std::shared_ptr<CPVRChannelGroupMember>& groupMember);
+
+  /*!
+   * @brief Reset the position of the given groups, then resort groups.
+   * @param sortedGroupPaths The paths of the groups to re-position.
+   * @return True if any group position was changed, false otherwise.
+   */
+  bool ResetGroupPositions(const std::vector<std::string>& sortedGroupPaths);
+
+  /*!
    * @brief Persist all changes in channel groups.
    * @return True if everything was persisted, false otherwise.
    */
@@ -223,12 +259,8 @@ public:
    */
   int CleanupCachedImages();
 
-  /*!
-   * @brief Sort the groups.
-   */
-  void SortGroups();
-
 private:
+  void SortGroups();
   void SortGroupsByBackendOrder();
   void SortGroupsByLocalOrder();
 
@@ -254,6 +286,14 @@ private:
                                                    Exclude exclude) const;
   std::shared_ptr<CPVRChannelGroup> GetGroupById(int groupId, Exclude exclude) const;
 
+  enum class GroupState
+  {
+    DELETED,
+    CHANGED,
+  };
+  void GroupStateChanged(const std::shared_ptr<CPVRChannelGroup>& group,
+                         GroupState state = GroupState::CHANGED);
+
   bool m_bRadio{false};
   std::vector<std::shared_ptr<CPVRChannelGroup>> m_groups;
   mutable CCriticalSection m_critSection;
diff --git a/xbmc/pvr/dialogs/GUIDialogPVRChannelManager.cpp b/xbmc/pvr/dialogs/GUIDialogPVRChannelManager.cpp
index f60ac12f23..b601c63985 100644
--- a/xbmc/pvr/dialogs/GUIDialogPVRChannelManager.cpp
+++ b/xbmc/pvr/dialogs/GUIDialogPVRChannelManager.cpp
@@ -979,12 +979,18 @@ bool CGUIDialogPVRChannelManager::UpdateChannelData(const std::shared_ptr<CFileI
     }
 
     // make sure the channel is part of the group
-    group->AppendToGroup(groupMember);
+    CServiceBroker::GetPVRManager()
+        .ChannelGroups()
+        ->Get(m_bIsRadio)
+        ->AppendToGroup(group, groupMember);
   }
   else
   {
     // remove the hidden channel from the group
-    group->RemoveFromGroup(groupMember);
+    CServiceBroker::GetPVRManager()
+        .ChannelGroups()
+        ->Get(m_bIsRadio)
+        ->RemoveFromGroup(group, groupMember);
   }
 
   const auto channel = groupMember->Channel();
diff --git a/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.cpp b/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.cpp
index b5b9da79c0..f04c7502aa 100644
--- a/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.cpp
+++ b/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.cpp
@@ -85,11 +85,6 @@ void CGUIDialogPVRGroupManager::SetRadio(bool bIsRadio)
   SetProperty("IsRadio", m_bIsRadio ? "true" : "");
 }
 
-bool CGUIDialogPVRGroupManager::PersistChanges()
-{
-  return CServiceBroker::GetPVRManager().ChannelGroups()->Get(m_bIsRadio)->PersistAll();
-}
-
 bool CGUIDialogPVRGroupManager::OnPopupMenu(int itemNumber)
 {
   // Currently, the only context menu item is "move".
@@ -144,7 +139,6 @@ bool CGUIDialogPVRGroupManager::ActionButtonOk(const CGUIMessage& message)
 
   if (iControl == BUTTON_OK)
   {
-    PersistChanges();
     Close();
     bReturn = true;
   }
@@ -211,7 +205,9 @@ bool CGUIDialogPVRGroupManager::ActionButtonDeleteGroup(const CGUIMessage& messa
               .ChannelGroups()
               ->Get(m_bIsRadio)
               ->DeleteGroup(m_selectedGroup))
+      {
         Update();
+      }
     }
 
     bReturn = true;
@@ -242,8 +238,14 @@ bool CGUIDialogPVRGroupManager::ActionButtonRenameGroup(const CGUIMessage& messa
       if (!strGroupName.empty())
       {
         ClearSelectedGroupsThumbnail();
-        m_selectedGroup->SetGroupName(strGroupName, !resetName);
-        Update();
+        if (CServiceBroker::GetPVRManager()
+                .ChannelGroups()
+                ->Get(m_bIsRadio)
+                ->SetGroupName(m_selectedGroup, strGroupName, !resetName))
+        {
+          m_iSelectedChannelGroup = -1; // recalc index in Update()
+          Update();
+        }
       }
     }
 
@@ -275,9 +277,13 @@ bool CGUIDialogPVRGroupManager::ActionButtonUngroupedChannels(const CGUIMessage&
         {
           const auto itemChannel = m_ungroupedChannels->Get(m_iSelectedUngroupedChannel);
 
-          if (m_selectedGroup->AppendToGroup(itemChannel->GetPVRChannelGroupMemberInfoTag()))
+          if (CServiceBroker::GetPVRManager()
+                  .ChannelGroups()
+                  ->Get(m_bIsRadio)
+                  ->AppendToGroup(m_selectedGroup, itemChannel->GetPVRChannelGroupMemberInfoTag()))
           {
             ClearSelectedGroupsThumbnail();
+            m_iSelectedChannelGroup = -1; // recalc index in Update()
             Update();
           }
         }
@@ -306,9 +312,16 @@ bool CGUIDialogPVRGroupManager::ActionButtonGroupMembers(const CGUIMessage& mess
         if (m_selectedGroup && m_groupMembers->GetFileCount() > 0)
         {
           const auto itemChannel = m_groupMembers->Get(m_iSelectedGroupMember);
-          m_selectedGroup->RemoveFromGroup(itemChannel->GetPVRChannelGroupMemberInfoTag());
           ClearSelectedGroupsThumbnail();
-          Update();
+          if (CServiceBroker::GetPVRManager()
+                  .ChannelGroups()
+                  ->Get(m_bIsRadio)
+                  ->RemoveFromGroup(m_selectedGroup,
+                                    itemChannel->GetPVRChannelGroupMemberInfoTag()))
+          {
+            m_iSelectedChannelGroup = -1; // recalc index in Update()
+            Update();
+          }
         }
       }
     }
@@ -351,15 +364,20 @@ bool CGUIDialogPVRGroupManager::ActionButtonChannelGroups(const CGUIMessage& mes
         m_movingItem = false;
 
         // reset group positions
-        auto* groups = CServiceBroker::GetPVRManager().ChannelGroups()->Get(m_bIsRadio);
-        int pos = 1;
-        for (auto& groupItem : *m_channelGroups)
+        std::vector<std::string> paths;
+        for (const auto& groupItem : *m_channelGroups)
+        {
+          paths.emplace_back(groupItem->GetPath());
+        }
+
+        if (CServiceBroker::GetPVRManager()
+                .ChannelGroups()
+                ->Get(m_bIsRadio)
+                ->ResetGroupPositions(paths))
         {
-          const auto group = groups->GetGroupByPath(groupItem->GetPath());
-          if (group)
-            group->SetPosition(pos++);
+          Update();
         }
-        groups->SortGroups();
+
         bReturn = true;
       }
     }
@@ -378,11 +396,13 @@ bool CGUIDialogPVRGroupManager::ActionButtonHideGroup(const CGUIMessage& message
         static_cast<CGUIRadioButtonControl*>(GetControl(message.GetSenderId()));
     if (button)
     {
-      CServiceBroker::GetPVRManager()
-          .ChannelGroups()
-          ->Get(m_bIsRadio)
-          ->HideGroup(m_selectedGroup, button->IsSelected());
-      Update();
+      if (CServiceBroker::GetPVRManager()
+              .ChannelGroups()
+              ->Get(m_bIsRadio)
+              ->HideGroup(m_selectedGroup, button->IsSelected()))
+      {
+        Update();
+      }
     }
 
     bReturn = true;
@@ -397,7 +417,6 @@ bool CGUIDialogPVRGroupManager::ActionButtonToggleRadioTV(const CGUIMessage& mes
 
   if (message.GetSenderId() == BUTTON_TOGGLE_RADIO_TV)
   {
-    PersistChanges();
     SetRadio(!m_bIsRadio);
     Update();
     bReturn = true;
diff --git a/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.h b/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.h
index 2c8ef4fc82..b5a8aae14f 100644
--- a/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.h
+++ b/xbmc/pvr/dialogs/GUIDialogPVRGroupManager.h
@@ -41,7 +41,6 @@ private:
   void Clear();
   void ClearSelectedGroupsThumbnail();
   void Update();
-  bool PersistChanges();
   bool ActionButtonOk(const CGUIMessage& message);
   bool ActionButtonNewGroup(const CGUIMessage& message);
   bool ActionButtonDeleteGroup(const CGUIMessage& message);
diff --git a/xbmc/pvr/guilib/PVRGUIActionsChannels.cpp b/xbmc/pvr/guilib/PVRGUIActionsChannels.cpp
index f5b9831c8f..262841be9a 100644
--- a/xbmc/pvr/guilib/PVRGUIActionsChannels.cpp
+++ b/xbmc/pvr/guilib/PVRGUIActionsChannels.cpp
@@ -215,10 +215,9 @@ bool CPVRGUIActionsChannels::HideChannel(const CFileItem& item) const
           CVariant{""}, CVariant{channel->ChannelName()}))
     return false;
 
-  if (!CServiceBroker::GetPVRManager()
-           .ChannelGroups()
-           ->GetGroupAll(channel->IsRadio())
-           ->RemoveFromGroup(groupMember))
+  const auto groups{CServiceBroker::GetPVRManager().ChannelGroups()};
+  if (!groups->Get(channel->IsRadio())
+           ->RemoveFromGroup(groups->GetGroupAll(channel->IsRadio()), groupMember))
     return false;
 
   CGUIWindowPVRBase* pvrWindow =