refactors don't have to be messy

part of being a good developer involves thinking backwards from the users' and the reviewers' perspective for the ux and the code respectively. there's a really easy thing you can do to make commits less messy.

if you are refactoring a function i.e. creating a new function out of it's body, then you should place the new function "physically away" from the old function in the file. that way git diffs won't be messy. take the following diffs, both do the same thing with the difference being the physical location of the new function in the file.

easy to review

  1diff --git a/sc/source/ui/cctrl/checklistmenu.cxx b/sc/source/ui/cctrl/checklistmenu.cxx
  2index 39afd0963606..1e48e6e04afa 100644
  3--- a/sc/source/ui/cctrl/checklistmenu.cxx
  4+++ b/sc/source/ui/cctrl/checklistmenu.cxx
  5@@ -912,52 +912,12 @@ void ScCheckListMenuControl::MarkCheckedMembers()
  6 
  7 IMPL_LINK_NOARG(ScCheckListMenuControl, LockCheckedHdl, weld::Toggleable&, void)
  8 {
  9-    bool bLockCheckedEntries = mxChkLockChecked->get_active();
 10     MarkCheckedMembers();
 11+    UpdateVisibleMembers(false);
 12 
 13-    if (mbHasDates)
 14-    {
 15-        // TODO: flesh this out
 16-    }
 17-    else
 18-    {
 19-        mpChecks->freeze();
 20-        mpChecks->clear();
 21-        mpChecks->thaw();
 22-
 23-        OUString aSearchText = mxEdSearch->get_text();
 24-        aSearchText = ScGlobal::getCharClass().lowercase(aSearchText);
 25-        if (aSearchText.isEmpty())
 26-        {
 27-            /*
 28-             * when we click on lock, all the checked entries are marked and
 29-             * this `true` tells `initMembers` to check only the currently
 30-             * checked entries.
 31-             *
 32-             * when lock is unchecked we want that the entries which were locked
 33-             * and checked now become unlocked and checked so that we can select
 34-             * or deselect more entries, still we want only the marked (selected)
 35-             * entries to remain selected, thus this `true` is valid even in the
 36-             * uncheck case.
 37-             */
 38-            initMembers(-1, true);
 39-        }
 40-        else
 41-        {
 42-            std::vector<int> aShownIndexes;
 43-            loadSearchedMembers(aShownIndexes, maMembers, aSearchText, true);
 44-            std::vector<int> aFixedWidths { mnCheckWidthReq };
 45-
 46-            // insert the members, remember whether checked or unchecked.
 47-            mpChecks->bulk_insert_for_each(aShownIndexes.size(), [this, &aShownIndexes, &bLockCheckedEntries](weld::TreeIter& rIter, int i) {
 48-                size_t nIndex = aShownIndexes[i];
 49-                insertMember(*mpChecks, rIter, maMembers[nIndex], maMembers[nIndex].mbMarked, bLockCheckedEntries);
 50-            }, nullptr, &aFixedWidths);
 51-        }
 52-    }
 53-
 54-    // unmarking should happen after the members are inserted
 55-    if (!bLockCheckedEntries)
 56+    // unmarking should happen after the members are inserted or updated
 57+    // (in case there's a hierarchy, in which case we don't clear everything)
 58+    if (!mxChkLockChecked->get_active())
 59         for (auto& aMember : maMembers)
 60             aMember.mbMarked = false;
 61 }
 62@@ -1119,6 +1079,54 @@ IMPL_LINK_NOARG(ScCheckListMenuControl, SearchEditTimeoutHdl, Timer*, void)
 63     }
 64 }
 65 
 66+void ScCheckListMenuControl::UpdateVisibleMembers(bool bSearchEditTimeout)
 67+{
 68+    // TODO: change it later based on the `SearchEditTimeoutHdl` code
 69+    bool bLockCheckedEntries = !bSearchEditTimeout && mxChkLockChecked->get_active();
 70+    OUString aSearchText = mxEdSearch->get_text();
 71+    aSearchText = ScGlobal::getCharClass().lowercase(aSearchText);
 72+
 73+    if (mbHasDates)
 74+    {
 75+        // TODO: flesh it out later
 76+    }
 77+    else
 78+    {
 79+        mpChecks->freeze();
 80+        mpChecks->clear();
 81+        mpChecks->thaw();
 82+
 83+        if (aSearchText.isEmpty())
 84+        {
 85+            /*
 86+             * when we click on lock, all the checked entries are marked and
 87+             * this `true` tells `initMembers` to check only the currently
 88+             * checked entries.
 89+             *
 90+             * when lock is unchecked we want that the entries which were locked
 91+             * and checked now become unlocked and checked so that we can select
 92+             * or deselect more entries, still we want only the marked (selected)
 93+             * entries to remain selected, thus this `true` is valid even in the
 94+             * uncheck case.
 95+             */
 96+            initMembers(-1, true);
 97+        }
 98+        else
 99+        {
100+            std::vector<int> aShownIndexes;
101+            loadSearchedMembers(aShownIndexes, maMembers, aSearchText, true);
102+            std::vector<int> aFixedWidths { mnCheckWidthReq };
103+
104+            // insert the members, remember whether checked or unchecked.
105+            mpChecks->bulk_insert_for_each(aShownIndexes.size(), [this, &aShownIndexes, &bLockCheckedEntries](weld::TreeIter& rIter, int i) {
106+                size_t nIndex = aShownIndexes[i];
107+                insertMember(*mpChecks, rIter, maMembers[nIndex], maMembers[nIndex].mbMarked, bLockCheckedEntries);
108+            }, nullptr, &aFixedWidths);
109+        }
110+    }
111+}
112+
113+
114 IMPL_LINK_NOARG(ScCheckListMenuControl, EdModifyHdl, weld::Entry&, void)
115 {
116     maSearchEditTimer.Start();

harder to review

  1diff --git a/sc/source/ui/cctrl/checklistmenu.cxx b/sc/source/ui/cctrl/checklistmenu.cxx
  2index 39afd0963606..a38226336130 100644
  3--- a/sc/source/ui/cctrl/checklistmenu.cxx
  4+++ b/sc/source/ui/cctrl/checklistmenu.cxx
  5@@ -875,49 +875,16 @@ namespace
  6     }
  7 }
  8 
  9-void ScCheckListMenuControl::MarkCheckedMembers()
 10-{
 11-    if (mbHasDates)
 12-    {
 13-        // TODO: flesh it out later
 14-    }
 15-    else
 16-    {
 17-        // go over the members visible in the popup, and remember which one is
 18-        // checked, and which one is not by setting `mbMarked` to `true`; by default
 19-        // `mbMarked` is `false`, we clear all the marks when lock is unchecked, see
 20-        // at the end of this callback.
 21-        mpChecks->all_foreach([this](weld::TreeIter& rEntry){
 22-            if (mpChecks->get_toggle(rEntry) == TRISTATE_TRUE)
 23-            {
 24-                for (auto& aMember : maMembers)
 25-                {
 26-                    if (aMember.maName == mpChecks->get_text(rEntry))
 27-                    {
 28-                        aMember.mbMarked = true;
 29-                        /*
 30-                         * if there are multiple entries with the same
 31-                         * name in the range, they all show up as a single
 32-                         * entry in the autofilter, so we can break
 33-                         */
 34-                        break;
 35-                    }
 36-                }
 37-            }
 38-
 39-            return false;
 40-        });
 41-    }
 42-}
 43-
 44-IMPL_LINK_NOARG(ScCheckListMenuControl, LockCheckedHdl, weld::Toggleable&, void)
 45+void ScCheckListMenuControl::UpdateVisibleMembers(bool bSearchEditTimeout)
 46 {
 47-    bool bLockCheckedEntries = mxChkLockChecked->get_active();
 48-    MarkCheckedMembers();
 49+    // TODO: change it later based on the `SearchEditTimeoutHdl` code
 50+    bool bLockCheckedEntries = !bSearchEditTimeout && mxChkLockChecked->get_active();
 51+    OUString aSearchText = mxEdSearch->get_text();
 52+    aSearchText = ScGlobal::getCharClass().lowercase(aSearchText);
 53 
 54     if (mbHasDates)
 55     {
 56-        // TODO: flesh this out
 57+        // TODO: flesh it out later
 58     }
 59     else
 60     {
 61@@ -925,8 +892,6 @@ IMPL_LINK_NOARG(ScCheckListMenuControl, LockCheckedHdl, weld::Toggleable&, void)
 62         mpChecks->clear();
 63         mpChecks->thaw();
 64 
 65-        OUString aSearchText = mxEdSearch->get_text();
 66-        aSearchText = ScGlobal::getCharClass().lowercase(aSearchText);
 67         if (aSearchText.isEmpty())
 68         {
 69             /*
 70@@ -955,9 +920,51 @@ IMPL_LINK_NOARG(ScCheckListMenuControl, LockCheckedHdl, weld::Toggleable&, void)
 71             }, nullptr, &aFixedWidths);
 72         }
 73     }
 74+}
 75+
 76+void ScCheckListMenuControl::MarkCheckedMembers()
 77+{
 78+    if (mbHasDates)
 79+    {
 80+        // TODO: flesh it out later
 81+    }
 82+    else
 83+    {
 84+        // go over the members visible in the popup, and remember which one is
 85+        // checked, and which one is not by setting `mbMarked` to `true`; by default
 86+        // `mbMarked` is `false`, we clear all the marks when lock is unchecked, see
 87+        // at the end of this callback.
 88+        mpChecks->all_foreach([this](weld::TreeIter& rEntry){
 89+            if (mpChecks->get_toggle(rEntry) == TRISTATE_TRUE)
 90+            {
 91+                for (auto& aMember : maMembers)
 92+                {
 93+                    if (aMember.maName == mpChecks->get_text(rEntry))
 94+                    {
 95+                        aMember.mbMarked = true;
 96+                        /*
 97+                         * if there are multiple entries with the same
 98+                         * name in the range, they all show up as a single
 99+                         * entry in the autofilter, so we can break
100+                         */
101+                        break;
102+                    }
103+                }
104+            }
105+
106+            return false;
107+        });
108+    }
109+}
110+
111+IMPL_LINK_NOARG(ScCheckListMenuControl, LockCheckedHdl, weld::Toggleable&, void)
112+{
113+    MarkCheckedMembers();
114+    UpdateVisibleMembers(false);
115 
116-    // unmarking should happen after the members are inserted
117-    if (!bLockCheckedEntries)
118+    // unmarking should happen after the members are inserted or updated
119+    // (in case there's a hierarchy, in which case we don't clear everything)
120+    if (!mxChkLockChecked->get_active())
121         for (auto& aMember : maMembers)
122             aMember.mbMarked = false;
123 }