Comments and name changes to clarify code for toolbar configurations

src/toolbars/ToolDock.cpp

@@ -64,7 +64,7 @@ auto ToolBarConfiguration::FindPlace(const ToolBar *bar) const
       });
 }
 
-auto ToolBarConfiguration::FindParent(const ToolBar *bar)
+auto ToolBarConfiguration::FindPeers(const ToolBar *bar)
    -> std::pair<Forest*, Forest::iterator>
 {
    auto findTree = [=](Forest &forest){
@@ -102,6 +102,7 @@ void ToolBarConfiguration::Insert(ToolBar *bar, Position position)
 {
    if (position == UnspecifiedPosition) {
       // Add at the "end" of the layout
+      // bottommost and rightmost
       Forest *pForest = &mForest;
       while (!pForest->empty())
          pForest = &pForest->back().children;
@@ -109,13 +110,19 @@ void ToolBarConfiguration::Insert(ToolBar *bar, Position position)
       pForest->back().pBar = bar;
    }
    else {
+      // Insert at what depth?
       auto pForest = &mForest;
       if (position.rightOf) {
          const auto parent = FindPlace(position.rightOf);
          if (parent != end())
+            // Insert among children of some node
             pForest = &parent->pTree->children;
       }
+      else {
+         // Insert a new root in the top forest
+      }
 
+      // Insert at what breadth?
       const auto begin = pForest->begin();
       auto iter = begin;
       const auto end = pForest->end();
@@ -135,16 +142,22 @@ void ToolBarConfiguration::Insert(ToolBar *bar, Position position)
             iter = begin;
       }
       else
+         // No previous sibling specified, so insert as first
          adopt = (iter != end);
 
-      // Adopt the child only if the insertion point specifies that
+      // Insert as a leaf, or as an internal node?
       if (adopt && position.adopt) {
-         // Make NEW node with one child
+         // Existing child of parent become its grandchild
+
+         // TODO:  is it ever correct to adopt more than one, depending on
+         // heights?  Could an inserted tall bar adopt two short ones?
+
+         // Make NEW node
          Tree tree;
          tree.pBar = bar;
-         tree.children.push_back(Tree{});
 
          // Do adoption
+         tree.children.push_back(Tree{});
          auto &child = tree.children.back();
          child.pBar = iter->pBar;
          child.children.swap(iter->children);
@@ -153,6 +166,7 @@ void ToolBarConfiguration::Insert(ToolBar *bar, Position position)
          (*iter).swap(tree);
       }
       else
+         // Insert as a leaf
          pForest->insert(iter, Tree {})->pBar = bar;
    }
 }
@@ -166,6 +180,9 @@ void ToolBarConfiguration::InsertAtPath
    // Guarantee the existence of nodes
    for (auto ii : path) {
       Forest::size_type uu = std::max(0, ii);
+      // This may make more than one default-constructed tree, which we
+      // will fill in with some other call to InsertAtPath, or else cleanup
+      // with RemoveNulls
       pForest->resize(std::max(uu + 1, pForest->size()));
       pTree = &(*pForest)[uu];
       pForest = &pTree->children;
@@ -177,6 +194,7 @@ void ToolBarConfiguration::InsertAtPath
 
 void ToolBarConfiguration::Remove(Forest &forest, Forest::iterator iter)
 {
+   // Reparent all of the children of the deleted node
    Tree tree;
    tree.swap(*iter);
    iter = forest.erase(iter);
@@ -191,7 +209,7 @@ void ToolBarConfiguration::Remove(Forest &forest, Forest::iterator iter)
 
 void ToolBarConfiguration::Remove(const ToolBar *bar)
 {
-   auto results = FindParent(bar);
+   auto results = FindPeers(bar);
    auto pForest = results.first;
    if (pForest) {
       // Reparent all of the children of the deleted node
@@ -225,7 +243,8 @@ bool ToolBarConfiguration::IsRightmost(const ToolBar *bar) const
       // Last of all
       return true;
    if (bar->GetRect().y != iter->pTree->pBar->GetRect().y)
-      // 	
+      // Next step in preorder traversal is not rightward to a child drawn at
+      // the same height
       return true;
    return false;
 }
@@ -292,6 +311,8 @@ void ToolBarConfiguration::PostRead(Legacy &legacy)
    // against the case of obsolete preferences, perhaps
    RemoveNulls(mForest);
 
+   // Interpret what was saved in old .cfg files under "order"
+   // which specified toolbar configuration simply as a sequence, not a tree
    ToolBar *prev {};
    for (auto pBar : legacy.bars) {
       if (!pBar)
@@ -307,7 +328,9 @@ void ToolBarConfiguration::PostRead(Legacy &legacy)
 void ToolBarConfiguration::Write
    (const ToolBarConfiguration *pConfiguration, const ToolBar *bar)
 {
+   // Assume a path has been set in gPrefs suitable for bar
    if (pConfiguration) {
+      // Write comma-separated list of numbers specifying position in the tree
       wxString strPath;
       const auto cIter = pConfiguration->FindPlace(bar);
       const auto path = cIter.GetPath();
@@ -320,6 +343,9 @@ void ToolBarConfiguration::Write
       gPrefs->Write(wxT("Path"), strPath);
 
       // Remove any legacy configuration info.
+      // Note:  this causes Audacity 2.1.2 and earlier to create toolbars
+      // always in default position when reading a .cfg saved by Audacity
+      // 2.1.3 or later
       gPrefs->DeleteEntry(wxT("Order"));
    }
    gPrefs->Write( wxT("Show"), bar->IsVisible() );
@@ -506,7 +532,7 @@ void ToolDock::VisitLayout(LayoutVisitor &visitor,
          prevSib = sib;
          sib = ct;
       }
-      ToolBarConfiguration::Position prevPosition = { parent, prevSib };
+      auto prevPosition = ToolBarConfiguration::Position{ parent, prevSib };
 
       // Determine the size of the toolbar to fit, with advice from
       // the visitor object

src/toolbars/ToolDock.h

@@ -45,6 +45,8 @@ enum
    DockCount = 2
 };
 
+// A description of a layout of toolbars, as a forest of trees that root
+// at the left edge of the tool dock and grow rightward
 class ToolBarConfiguration
 {
    struct Tree;
@@ -62,6 +64,10 @@ public:
       mForest.clear();
    }
 
+   // Describe one toolbar's position in terms of its parent and preceding
+   // sibling
+   // When specifying a place at which to insert, "adopt" means insertion of
+   // an internal node displacing other nodes deeper as its children
    struct Position {
       ToolBar *rightOf {};
       ToolBar *below {};
@@ -71,7 +77,7 @@ public:
       // Default constructor
       Position() {}
 
-      Position(
+      explicit Position(
          ToolBar *r,
          ToolBar *b = nullptr,
          bool shouldAdopt = true
@@ -99,13 +105,14 @@ public:
 
    static const Position UnspecifiedPosition;
 
+   // Point to a node in the forest and describe its position
    struct Place {
       Tree *pTree {};
       Position position;
    };
 
    // This iterator visits the nodes of the forest in pre-order, and at each
-   // stop, makes the parent, previous sibling, and children accessible.
+   // stop, reports its Place
    class Iterator
       : public std::iterator<std::forward_iterator_tag, Place>
    {
@@ -116,7 +123,7 @@ public:
       {
          // This is a feature:  advance position even at the end
          mPlace.position =
-            { mPlace.pTree ? mPlace.pTree->pBar : nullptr };
+            Position{ mPlace.pTree ? mPlace.pTree->pBar : nullptr };
 
          if (!mIters.empty())
          {
@@ -212,7 +219,7 @@ public:
             return
                // lhs.begin == rhs.begin &&
                lhs.current == rhs.current
-               // lhs.end == rhs.end
+               // && lhs.end == rhs.end
             ;
          }
       };
@@ -275,7 +282,7 @@ private:
    };
 
    Iterator FindPlace(const ToolBar *bar) const;
-   std::pair<Forest*, Forest::iterator> FindParent(const ToolBar *bar);
+   std::pair<Forest*, Forest::iterator> FindPeers(const ToolBar *bar);
 
    Forest mForest;
 };

src/toolbars/ToolManager.cpp

@@ -764,8 +764,8 @@ void ToolManager::ReadConfig()
          default:        d = nullptr; pLegacy = nullptr; break;
       }
 
-      bool ordered = ToolBarConfiguration::Read
+      bool ordered = ToolBarConfiguration::Read(
-         (d ? &d->GetConfiguration() : nullptr,
+         d ? &d->GetConfiguration() : nullptr,
          pLegacy,
          bar, show[ ndx ], bShownByDefault)
       && found;
@@ -922,7 +922,7 @@ void ToolManager::ReadConfig()
             const auto deviceToolBar = mBars[ DeviceBarID ].get();
             if (deviceToolBar->GetDock() == mTopDock) {
                deviceToolBar->GetDock()->Undock(deviceToolBar);
-               position = { t, nullptr };
+               position = ToolBarConfiguration::Position{ t, nullptr };
                mTopDock->Dock(deviceToolBar, false, position);
 
                // Remember not to place the device toolbar again