diff --git a/src/BloomExe/Publish/PublishHelper.cs b/src/BloomExe/Publish/PublishHelper.cs
index db874eb003be..0d63d4bf5716 100644
--- a/src/BloomExe/Publish/PublishHelper.cs
+++ b/src/BloomExe/Publish/PublishHelper.cs
@@ -48,6 +48,13 @@ public static void Cancel()
_latestInstance = null;
}
+ ///
+ /// A static mirror of WorkspaceTabSelection.ActiveTab == WorkspaceTab.publish, which exists
+ /// only because the code that stages books (and so hits the guard in the constructor above)
+ /// is static and cannot get at the one WorkspaceTabSelection. Its setter is the sole writer:
+ /// do not set this from anywhere else, or the two can drift apart and turn an ordinary
+ /// publish into the exception above (BL-16174).
+ ///
public static bool InPublishTab { get; set; }
private OffScreenBrowser _pageChecksBrowser;
diff --git a/src/BloomExe/Publish/PublishView.cs b/src/BloomExe/Publish/PublishView.cs
index 174fe826466c..68aa7c5def11 100644
--- a/src/BloomExe/Publish/PublishView.cs
+++ b/src/BloomExe/Publish/PublishView.cs
@@ -52,19 +52,20 @@ BloomWebSocketServer webSocketServer
//off the tab itself changing, either to us or away from us.
selectedTabChangedEvent.Subscribe(_ =>
{
- if (_tabSelection.ActiveTab == WorkspaceTab.publish)
- {
- if (!_isActive)
- {
- Activate();
- _isActive = true;
- }
- }
- else if (_isActive)
- {
+ var shouldBeActive = _tabSelection.ActiveTab == WorkspaceTab.publish;
+ if (shouldBeActive == _isActive)
+ return;
+ // Record where we are going before doing the work, not after. If some part of
+ // activating or deactivating throws, we must not be left believing we are still in
+ // the state we just left: that stale belief would silently skip the *next*
+ // Activate(), and with it the SetTabsEnabled(true) below that releases the Edit
+ // tab's save lock -- leaving the Collection and Edit tabs greyed out until the user
+ // restarts Bloom. That was half of BL-16174. The exception itself still propagates.
+ _isActive = shouldBeActive;
+ if (shouldBeActive)
+ Activate();
+ else
Deactivate();
- _isActive = false;
- }
});
//TODO: find a way to call this just once, at the right time:
@@ -80,7 +81,8 @@ private void Deactivate()
_publishToVideoApi.AbortMakingVideo();
// TODO-WV2: Can we clear the cache for WV2? Do we need to?
PublishHelper.Cancel();
- PublishHelper.InPublishTab = false;
+ // Note: PublishHelper.InPublishTab is not ours to clear; WorkspaceTabSelection.ActiveTab
+ // has already done it. See the comment there (BL-16174).
_webSocketServer.SendEvent("publish", "switchOutOfPublishTab");
}
@@ -104,7 +106,9 @@ private void Activate()
// Safety net: any Edit-tab save lock must be complete before we reach Publish,
// so ensure tab switching is enabled in case the re-enable callback was missed.
WorkspaceView?.SetTabsEnabled(true);
- PublishHelper.InPublishTab = true;
+ // Note: PublishHelper.InPublishTab is not ours to set; WorkspaceTabSelection.ActiveTab
+ // has already done it, before any of this event's subscribers ran. See the comment
+ // there (BL-16174).
var hostForm = GetHostControlForInvoke() as Form;
PublishEpubApi.ControlForInvoke = hostForm;
LibraryPublishApi.Model = new BloomLibraryPublishModel(
diff --git a/src/BloomExe/Workspace/WorkspaceTabSelection.cs b/src/BloomExe/Workspace/WorkspaceTabSelection.cs
index 75cdc35b97c2..a32db863568c 100644
--- a/src/BloomExe/Workspace/WorkspaceTabSelection.cs
+++ b/src/BloomExe/Workspace/WorkspaceTabSelection.cs
@@ -3,6 +3,7 @@
using System.Linq;
using System.Text;
using System.Threading.Tasks;
+using Bloom.Publish;
namespace Bloom.Workspace
{
@@ -20,6 +21,31 @@ public enum WorkspaceTab
///
public class WorkspaceTabSelection
{
- public WorkspaceTab ActiveTab;
+ private WorkspaceTab _activeTab;
+
+ ///
+ /// The tab the Workspace is currently showing. This is the authoritative answer to
+ /// "which tab are we on"; everything else that needs to know should either read this or
+ /// be kept in step by this setter.
+ ///
+ public WorkspaceTab ActiveTab
+ {
+ get => _activeTab;
+ set
+ {
+ _activeTab = value;
+ // PublishHelper refuses to stage a book while we are not in the Publish tab, but it
+ // is reached from static code that has no way to get hold of this object, so it needs
+ // a static mirror of this value. We update it here, as part of the same assignment,
+ // precisely so the two cannot disagree. PublishView used to own it instead, setting it
+ // from its SelectedTabChangedEvent subscriber -- which runs later, after ActiveTab has
+ // already changed and after however many other subscribers. Any gap or hiccup in
+ // between (e.g. an activation that got skipped) left ActiveTab saying "publish" while
+ // the flag still said "not publish", and then an ordinary switch-to-Publish-and-make-a-
+ // BloomPUB-preview died with "Should not be creating bloom book while not in publish
+ // tab". See BL-16174.
+ PublishHelper.InPublishTab = value == WorkspaceTab.publish;
+ }
+ }
}
}
diff --git a/src/BloomExe/web/controllers/PublishApi.cs b/src/BloomExe/web/controllers/PublishApi.cs
index 4405064159a9..a3cfbce7375b 100644
--- a/src/BloomExe/web/controllers/PublishApi.cs
+++ b/src/BloomExe/web/controllers/PublishApi.cs
@@ -778,6 +778,11 @@ public string StageBookForBloomPubPreviewForTest(Book.Book book)
// put Bloom into the publish tab first (POST workspace/selectTab {tab:"publish"}), which
// runs all the real publish-tab setup; otherwise we might miss setup that the tab does
// now or adds later. Fail Fast here (via the guard) if the caller forgot to do that.
+ // Note what the guard does and does not prove: since BL-16174 the flag is set by the
+ // ActiveTab setter, so it goes true when the tab becomes active, ahead of the
+ // SelectedTabChangedEvent subscribers that do the rest of the setup (PublishView.Activate).
+ // What makes the setup complete by the time we get here is that selectTab runs the whole
+ // switch synchronously inside its own API call, which the caller awaits -- not the flag.
if (!PublishHelper.InPublishTab)
throw new InvalidOperationException(
"makeBloomPubPreview requires the publish tab to be active. "
diff --git a/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs b/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs
new file mode 100644
index 000000000000..2ae36ddf8f56
--- /dev/null
+++ b/src/BloomTests/Workspace/WorkspaceTabSelectionTests.cs
@@ -0,0 +1,98 @@
+using Bloom.Publish;
+using Bloom.Workspace;
+using NUnit.Framework;
+
+namespace BloomTests.Workspace
+{
+ ///
+ /// PublishHelper.InPublishTab is a static mirror of ActiveTab, needed because the book-staging
+ /// code that consults it is static. These tests pin down that the mirror is updated by the
+ /// ActiveTab setter itself, so nothing can observe the two disagreeing. When they could
+ /// disagree, switching to the Publish tab and asking for a BloomPUB preview sometimes died with
+ /// "Should not be creating bloom book while not in publish tab" (BL-16174).
+ ///
+ [TestFixture]
+ // These tests read and write PublishHelper.InPublishTab, which is process-wide. Setup/TearDown
+ // put it back, which is enough while fixtures run one at a time, but say so explicitly so that
+ // turning parallel test execution on later cannot quietly let this fixture and the publish
+ // tests perturb each other.
+ [NonParallelizable]
+ public class WorkspaceTabSelectionTests
+ {
+ private bool _originalInPublishTab;
+
+ [SetUp]
+ public void Setup()
+ {
+ // It's a static, shared with the rest of the test run, so put it back afterwards.
+ _originalInPublishTab = PublishHelper.InPublishTab;
+ PublishHelper.InPublishTab = false;
+ }
+
+ [TearDown]
+ public void TearDown()
+ {
+ PublishHelper.InPublishTab = _originalInPublishTab;
+ }
+
+ [Test]
+ public void ActiveTab_SetToPublish_SetsInPublishTab()
+ {
+ var selection = new WorkspaceTabSelection();
+ Assert.That(
+ PublishHelper.InPublishTab,
+ Is.False,
+ "Sanity check: this test is meaningless unless the flag starts out false."
+ );
+
+ selection.ActiveTab = WorkspaceTab.publish;
+
+ Assert.That(selection.ActiveTab, Is.EqualTo(WorkspaceTab.publish));
+ Assert.That(PublishHelper.InPublishTab, Is.True);
+ }
+
+ [TestCase(WorkspaceTab.collection)]
+ [TestCase(WorkspaceTab.edit)]
+ public void ActiveTab_SetToOtherTab_ClearsInPublishTab(WorkspaceTab tab)
+ {
+ var selection = new WorkspaceTabSelection();
+ selection.ActiveTab = WorkspaceTab.publish;
+ Assert.That(
+ PublishHelper.InPublishTab,
+ Is.True,
+ "Sanity check: we should be starting from the publish tab."
+ );
+
+ selection.ActiveTab = tab;
+
+ Assert.That(selection.ActiveTab, Is.EqualTo(tab));
+ Assert.That(PublishHelper.InPublishTab, Is.False);
+ }
+
+ ///
+ /// Opening a different collection builds a whole new ProjectContext, and so a new
+ /// WorkspaceTabSelection, but InPublishTab is static and survives. Note that merely
+ /// constructing the new WorkspaceTabSelection does not clear it -- what clears it is the
+ /// new WorkspaceView constructor's `_tabSelection.ActiveTab = WorkspaceTab.collection`
+ /// (WorkspaceView.cs), which this test stands in for. So the invariant depends on that
+ /// line continuing to exist; without it we would carry "we are in the publish tab" over
+ /// into a collection that is sitting on its Collection tab.
+ ///
+ [Test]
+ public void ActiveTab_NewSelectionInitializedToCollection_ClearsStaleInPublishTab()
+ {
+ new WorkspaceTabSelection().ActiveTab = WorkspaceTab.publish;
+ Assert.That(
+ PublishHelper.InPublishTab,
+ Is.True,
+ "Sanity check: we should be starting from the publish tab."
+ );
+
+ // What WorkspaceView's constructor does for the new collection.
+ var newSelection = new WorkspaceTabSelection();
+ newSelection.ActiveTab = WorkspaceTab.collection;
+
+ Assert.That(PublishHelper.InPublishTab, Is.False);
+ }
+ }
+}