mirror of
https://github.com/Cockatrice/Cockatrice.git
synced 2026-11-05 09:33:25 -08:00
[CardDB] Reload after the first run enables all sets without freezing startup (#7470)
* [CardDB] Reload when the first run enables all sets checkUnknownSets() enabled every set when none was enabled, but the database had already been parsed under that empty enablement, so the parsers had stripped every printing. It only notified the models and never reloaded, leaving the deck editor's card database display empty until the next launch. Route the all-sets-enabled case through the same reload the new-sets path uses, and skip caching a snapshot whose sets are all disabled. * [CardDB] Own checkUnknownSets after startup and fix the cache skip The loader's initialLoadComplete guard skipped checkUnknownSets() on the first successful load of the session, so a download that landed after startupConfigCheck() and the onboarding wizard had already run (e.g. skipping the wizard download and fetching later from the menu) left every set disabled and the deck editor blank until the next launch. The guard also let the check run on the worker before the queued snapshot swap, and alongside reloadCardDatabasesAndNotify()'s own check, double-emitting cardDatabaseNewSetsFound on manual reloads. Move the check to the callers that load after startup: the post-update, custom-import, and spoiler paths run it via a QFutureWatcher once the finished snapshot has been swapped in, and the loader's synchronous path-change reload runs it directly. Judge the cache skip by the parsed sets' own enablement, what the parsers actually saw, instead of getEnabledSetNames(), which reports empty for the noop controller even though it enables every set. Wait for the first-run reparse before opening Manage Sets so the dialog cannot bind to the pre-reload set instances. * [CardDB] Queue post-reload check and unblock first-run Manage Sets reloadCardDatabasesAndNotify() no longer emits cardDatabaseEnabledSetsChanged after the swap's Reset already rebuilt the models (second 36k rebuild froze startup), and its follow-up checkUnknownSets() is queued to the GUI thread so it sees swapped-in data. cardDatabaseAllNewSetsEnabled() chains Welcome/Manage Sets off a QFutureWatcher instead of waitForFinished() blocking the event loop during the reparse. * [CardDB] Batch the first-run enable-all into a single settings flush checkUnknownSets() enabled every set with ~3 QSettings rewrite cycles per set (sort key, known, enabled), and each setValue re-opens and re-syncs the whole cardDatabase.ini. With a full database (874 sets) that froze the GUI thread for ~9s after the database download (LagMonitor: event loop stalled for 9129 ms, measured offscreen with a real 59MB cards.xml), reading as a total application hang that recovers on its own. Persist the enable-all through the controller's batched saveSets() in a single flush instead, mirroring what SetsModel::save() already does: CardSetList::enableAllBatched() assigns fresh sort keys and marks every set known+enabled in memory, then writes all records at once. SetSaveData gains the isKnown flag so the batch covers all three options, and SetsModel::save() passes the sets' known state through (unchanged behavior there). Also drop the cardDatabaseEnabledSetsChanged emission from the all-sets-enabled branch: the reload chained off cardDatabaseAllNewSetsEnabled rebuilds the models from the re-parsed snapshot via cardDatabaseReset, so emitting both rebuilt the 36k-card models twice on the GUI thread. Adds a cache-invalidation test driving the real checkUnknownSets() first-run path. * [CardDB] Batch the new-sets-found enable-all the same way The first-run batching fixed the 874-set case, but the new-sets-found dialog path (enableAllUnknownSets/markAllSetsAsKnown) still wrote per set and per option, each setValue re-opening and re-syncing the whole cardDatabase.ini. Users with hundreds of custom sets hit the same multi-second GUI stall when answering the dialog. Persist both through the controller's batched saveSets() in a single flush, writing only changed sets (so the common no-op check skips the flush entirely). Removes the now-unused write-through CardSetList methods (enableAll, enableAllUnknown, markAllAsKnown, guessSortKeys) so the batched flush is the only way to persist bulk set changes. Extends the test controller mock with per-write/batched-flush counters and adds tests asserting both dialog paths persist with exactly one flush and zero individual writes. --------- Co-authored-by: Lukas Brübach <Bruebach.Lukas@bdosecurity.de>
This commit is contained in:
BruebachL
GitHub
parent
bac2d285e3
commit
a712553c81
14 changed files with 364 additions and 112 deletions
|
|
@ -159,16 +159,19 @@ public:
|
|||
|
||||
void setSortKey(QString shortName, unsigned int sortKey) override
|
||||
{
|
||||
++individualWrites;
|
||||
setOptions[shortName].sortKey = sortKey;
|
||||
}
|
||||
|
||||
void setEnabled(QString shortName, bool enabled) override
|
||||
{
|
||||
++individualWrites;
|
||||
setOptions[shortName].enabled = enabled;
|
||||
}
|
||||
|
||||
void setIsKnown(QString shortName, bool isknown) override
|
||||
{
|
||||
++individualWrites;
|
||||
setOptions[shortName].isKnown = isknown;
|
||||
}
|
||||
|
||||
|
|
@ -206,12 +209,17 @@ public:
|
|||
|
||||
void saveSets(const QVector<SetSaveData> &data) override
|
||||
{
|
||||
++batchedWrites;
|
||||
for (const SetSaveData &entry : data) {
|
||||
setOptions[entry.shortName].sortKey = entry.sortKey;
|
||||
setOptions[entry.shortName].enabled = entry.enabled;
|
||||
setOptions[entry.shortName].isKnown = entry.isKnown;
|
||||
}
|
||||
}
|
||||
|
||||
int individualWrites = 0; ///< Per-set/per-option controller writes (the slow path).
|
||||
int batchedWrites = 0; ///< Batched saveSets() flushes (the fast path).
|
||||
|
||||
private:
|
||||
QHash<QString, SetOptions> setOptions;
|
||||
};
|
||||
|
|
@ -375,6 +383,131 @@ TEST_F(CardDatabaseCacheInvalidationTest, ExplicitReloadBypassesCache)
|
|||
EXPECT_FALSE(db.getCardList().value("Cat")->getSets().contains("CAT"));
|
||||
EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("DOG"));
|
||||
}
|
||||
|
||||
TEST_F(CardDatabaseCacheInvalidationTest, FirstRunWithNoSetsEnabledSkipsCacheWrite)
|
||||
{
|
||||
// A blank config: the parser discovers every set, but none of them has an
|
||||
// ini entry, so all of them default to disabled (first run).
|
||||
SeededSetPriorityController blankController;
|
||||
NoopCardPreferenceProvider prefs;
|
||||
SandboxPathProvider pathProvider(root);
|
||||
CardDatabase db(nullptr, &prefs, &pathProvider, &blankController);
|
||||
|
||||
db.loadCardDatabases();
|
||||
ASSERT_FALSE(db.getCardList().value("Cat").isNull()) << "cards are parsed even while their sets are disabled";
|
||||
EXPECT_TRUE(db.getCardList().value("Cat")->getSets().isEmpty())
|
||||
<< "no printings survive an all-sets-disabled parse";
|
||||
EXPECT_TRUE(cacheContents().isEmpty()) << "a stripped first-run snapshot must not be cached";
|
||||
}
|
||||
|
||||
TEST_F(CardDatabaseCacheInvalidationTest, EnablingAllSetsAfterFirstRunRestoresPrintings)
|
||||
{
|
||||
SeededSetPriorityController blankController;
|
||||
NoopCardPreferenceProvider prefs;
|
||||
SandboxPathProvider pathProvider(root);
|
||||
CardDatabase db(nullptr, &prefs, &pathProvider, &blankController);
|
||||
|
||||
db.loadCardDatabases();
|
||||
ASSERT_TRUE(cacheContents().isEmpty());
|
||||
|
||||
// This mirrors what checkUnknownSets() does on a first run: enable every
|
||||
// set, then reload so the parser keeps the printings and the complete
|
||||
// snapshot gets cached under the enabled-set hash.
|
||||
for (const CardSetPtr &set : db.getSetList()) {
|
||||
blankController.setEnabled(set->getShortName(), true);
|
||||
}
|
||||
db.reloadCardDatabasesAndNotify();
|
||||
|
||||
ASSERT_FALSE(db.getCardList().value("Cat").isNull());
|
||||
EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("CAT"));
|
||||
EXPECT_FALSE(cacheContents().isEmpty()) << "the enabled-set snapshot must be cached";
|
||||
}
|
||||
|
||||
TEST_F(CardDatabaseCacheInvalidationTest, CheckUnknownSetsBatchesFirstRunEnableAll)
|
||||
{
|
||||
// Same first-run starting point as above, but driven through the real
|
||||
// checkUnknownSets() path, which must persist the enable-all with a single
|
||||
// batched saveSets() instead of per-set/per-option controller writes.
|
||||
SeededSetPriorityController blankController;
|
||||
NoopCardPreferenceProvider prefs;
|
||||
SandboxPathProvider pathProvider(root);
|
||||
CardDatabase db(nullptr, &prefs, &pathProvider, &blankController);
|
||||
|
||||
db.loadCardDatabases();
|
||||
ASSERT_TRUE(db.getSetList().getEnabledSetsNum() == 0);
|
||||
|
||||
db.checkUnknownSets();
|
||||
|
||||
EXPECT_EQ(blankController.individualWrites, 0) << "first-run enable-all must not write per set";
|
||||
EXPECT_EQ(blankController.batchedWrites, 1) << "first-run enable-all must persist with a single flush";
|
||||
for (const CardSetPtr &set : db.getSetList()) {
|
||||
EXPECT_TRUE(set->getEnabled()) << set->getShortName().toStdString();
|
||||
EXPECT_TRUE(blankController.isEnabled(set->getShortName())) << set->getShortName().toStdString();
|
||||
if (!set->getIsKnownIgnored()) {
|
||||
EXPECT_TRUE(set->getIsKnown()) << set->getShortName().toStdString();
|
||||
EXPECT_TRUE(blankController.isKnown(set->getShortName())) << set->getShortName().toStdString();
|
||||
}
|
||||
}
|
||||
|
||||
// The follow-up reload re-parses with the sets enabled, restoring printings.
|
||||
db.reloadCardDatabasesAndNotify();
|
||||
ASSERT_FALSE(db.getCardList().value("Cat").isNull());
|
||||
EXPECT_TRUE(db.getCardList().value("Cat")->getSets().contains("CAT"));
|
||||
}
|
||||
|
||||
TEST_F(CardDatabaseCacheInvalidationTest, EnableAllUnknownSetsIsBatched)
|
||||
{
|
||||
// The "new sets found" dialog path: enabling the unknown delta must persist
|
||||
// with a single flush no matter how many sets the database holds, so users
|
||||
// with hundreds of custom sets don't stall the GUI thread. Built directly
|
||||
// on CardSet objects because the fixture sets carry no metadata (and would
|
||||
// all count as ignored).
|
||||
SeededSetPriorityController controller;
|
||||
CardSetList sets;
|
||||
sets.append(
|
||||
CardSet::newInstance(&controller, "AAA", "Alpha Set", "core", QDate(2020, 1, 1), CardSet::PriorityPrimary));
|
||||
sets.append(
|
||||
CardSet::newInstance(&controller, "BBB", "Beta Set", "expansion", QDate(2021, 6, 1), CardSet::PriorityPrimary));
|
||||
sets.append(CardSet::newInstance(&controller, "XXX")); // no metadata: ignored
|
||||
|
||||
sets.enableAllUnknownBatched(&controller);
|
||||
|
||||
EXPECT_EQ(controller.individualWrites, 0) << "unknown-set enable must not write per set";
|
||||
EXPECT_EQ(controller.batchedWrites, 1) << "unknown-set enable must persist with a single flush";
|
||||
for (const CardSetPtr &set : sets) {
|
||||
EXPECT_TRUE(set->getEnabled()) << set->getShortName().toStdString();
|
||||
EXPECT_TRUE(controller.isEnabled(set->getShortName())) << set->getShortName().toStdString();
|
||||
if (set->getIsKnownIgnored()) {
|
||||
EXPECT_FALSE(set->getIsKnown()) << set->getShortName().toStdString();
|
||||
} else {
|
||||
EXPECT_TRUE(set->getIsKnown()) << set->getShortName().toStdString();
|
||||
EXPECT_TRUE(controller.isKnown(set->getShortName())) << set->getShortName().toStdString();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
TEST_F(CardDatabaseCacheInvalidationTest, MarkAllSetsAsKnownIsBatched)
|
||||
{
|
||||
// The dialog's "No" path: same single-flush requirement as enabling.
|
||||
SeededSetPriorityController controller;
|
||||
CardSetList sets;
|
||||
sets.append(
|
||||
CardSet::newInstance(&controller, "AAA", "Alpha Set", "core", QDate(2020, 1, 1), CardSet::PriorityPrimary));
|
||||
sets.append(
|
||||
CardSet::newInstance(&controller, "BBB", "Beta Set", "expansion", QDate(2021, 6, 1), CardSet::PriorityPrimary));
|
||||
sets.append(CardSet::newInstance(&controller, "XXX")); // no metadata: ignored
|
||||
|
||||
sets.markAllAsKnownBatched(&controller);
|
||||
|
||||
EXPECT_EQ(controller.individualWrites, 0) << "mark-known must not write per set";
|
||||
EXPECT_EQ(controller.batchedWrites, 1) << "mark-known must persist with a single flush";
|
||||
EXPECT_TRUE(controller.isKnown("AAA"));
|
||||
EXPECT_TRUE(controller.isKnown("BBB"));
|
||||
EXPECT_FALSE(controller.isEnabled("AAA"));
|
||||
EXPECT_FALSE(controller.isEnabled("BBB"));
|
||||
// Ignored sets keep their known state but are enabled.
|
||||
EXPECT_TRUE(controller.isEnabled("XXX"));
|
||||
}
|
||||
} // namespace
|
||||
|
||||
int main(int argc, char **argv)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue