Uprating from P3 to P2 as it's a regression

The problem appears to be caused by the following if statement in void ToolManager::ReadConfig():

if (someFound &&
             t->GetType() == ScrubbingBarID) {
            // Special case code to put the NEW scrubbing toolbar where we
            // want it, when audacity.cfg is present from an older version

......
}

The scrubbing toolbar was introduced in 2.1.3.
Removing completely that if statement fixes the bug.
That would be my suggested fix - I have no inclination to investigate any further.

However, a developer may want to do something different.

Demoting this to P3 - there is a very simple workaround documented - plus we are gerring #diminishingly few reports of this on the Forum.

(In reply to David Bailes from comment #2)
> The problem appears to be caused by the following if statement in void
> ToolManager::ReadConfig():
> 
> if (someFound &&
>              t->GetType() == ScrubbingBarID) {
>             // Special case code to put the NEW scrubbing toolbar where we
>             // want it, when audacity.cfg is present from an older version
> 
> ......
> }
> 
> The scrubbing toolbar was introduced in 2.1.3.
> Removing completely that if statement fixes the bug.
> That would be my suggested fix - I have no inclination to investigate any
> further.
> 
> However, a developer may want to do something different.

Paul or James: do you have an opinion on this suggested fix? I am happy to commit it, if it's considered to be OK.

That code change (comment #4) doesn't look right.  The code in the 'if' is supposed to be just for scrubbing bar, so how is it right to make it apply to all toolbars?  Why does it 'work'?  I don't see how it relates to the commit by Paul that comment #0 says caused the problem.

Without an explanation for why the new changed code is better (which is not obvious) I don't want to take this change.  For all I know it causes other problems with toolbar layout.

WITH comments explaining what the corrected code is doing and why, this could be a valid fix.

I presume the following code would be deleted too:

         // Dock it
         d->Dock( t, false );

         // Show or hide the bar
         Expose( t->GetId(), show[ t->GetId() ] );

(In reply to James Crook from comment #5)
> That code change (comment #4) doesn't look right.  The code in the 'if' is
> supposed to be just for scrubbing bar, so how is it right to make it apply
> to all toolbars?  Why does it 'work'?

By removing the if statement, I meant removing all the following code, that is the entire if statement, and not just the condition:
         if (someFound &&
             t->GetType() == ScrubbingBarID) {
            // Special case code to put the NEW scrubbing toolbar where we
            // want it, when audacity.cfg is present from an older version
            ToolBar *lastRoot {};

            // Change from the ideal configuration to the constrained one,
            // just as when dragging and dropping
            ToolBarConfiguration dummy;
            mTopDock->WrapConfiguration(dummy);

            // Start a NEW row with just the scrubbing toolbar
            auto &configuration = mTopDock->GetConfiguration();
            for (const auto place : configuration)
               if (place.position.rightOf == nullptr)
                  lastRoot = place.pTree->pBar;
            ToolBarConfiguration::Position position {
               nullptr, lastRoot, false
            };
            mTopDock->Dock(t, false, position);

            // Reposition the device toolbar, if it was docked above,
            // right of scrubbing
            const auto deviceToolBar = mBars[ DeviceBarID ].get();
            if (deviceToolBar->GetDock() == mTopDock) {
               deviceToolBar->GetDock()->Undock(deviceToolBar);
               position = ToolBarConfiguration::Position{ t, nullptr };
               mTopDock->Dock(deviceToolBar, false, position);

               // Remember not to place the device toolbar again
               deviceWasPositioned = true;
            }
            Expose( t->GetId(), show[ t->GetId() ] );
            continue;
         }

and this if statement can also be removed:
           if (deviceWasPositioned &&
             t->GetType() == DeviceBarID)
            continue;


  I don't see how it relates to the
> commit by Paul that comment #0 says caused the problem.

I don't know why Paul's commit caused the problem. I've had a look at Paul's code for positioning the toolbars, and decided it would take a long time to be able to understand it.

The code I'm suggesting to remove relates only to the positioning of the scrubbing toolbar. I would assume that this positioning is now of very little importance.

I don't know what is going wrong in the code I'm suggesting removing because I'm not familiar enough with Paul's positioning code, but removing it does fix the problem.

> 
> Without an explanation for why the new changed code is better (which is not
> obvious) I don't want to take this change.  For all I know it causes other
> problems with toolbar layout.
> 
> WITH comments explaining what the corrected code is doing and why, this
> could be a valid fix.

As I've already said, I can't provide a complete explanation of the problem. Ideally, this bug should be fixed by someone who is more familiar with the code.

> 
> I presume the following code would be deleted too:
> 
>          // Dock it
>          d->Dock( t, false );
> 
>          // Show or hide the bar
>          Expose( t->GetId(), show[ t->GetId() ] );

No, that code would remain.

DEVEL - FIX MADE
https://github.com/audacity/audacity/commit/9e69be9f6fb8589dbced06391a8ac945a0a10db1

Thanks David.  I'm happy with that code vanishing.  The comments suggest it was only relevant to upgraders, and since it causes problems for upgraders it can go - making our code much more readable.  It is irrelevant once a user has saved a new layout.

I think there are other bugs with toolbar layout.  I got an issue where toolbars stopped flowing on resizing the window, but I could not reproduce it, so I did not log it.  Anyway, it is not THIS bug, and if it is seen again should be logged as a new bug.

Tested on W10 with audacity-2.3.3-alpha-338-9e69be9f6fb8589dbced06391a8ac945a0a10db1

This looks to be properly fixed ...


Tested against 2.3.0 and the alpha 2.3.3 build 337 (one immediately prior) and on both of those I get the jumbled toolbars when I "upgrade" from 2.1.2 audacity settings.

But when I run the latest 338 alpha afyet an "upgrade" from 2.1.2 the toolbars are absolutely fine.

Also tested adding in the  Scrubbing Toolbar  which docked nicely in the upper tooldock - and the Spectral Selection Toolbarwhich docked nicely in the lower tooldock.