From 34e682d25bf3979dfd2d852062563f733750bd04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lukas=20Br=C3=BCbach?= Date: Sat, 5 Sep 2026 01:13:55 +0200 Subject: [PATCH] [DeckShare] Address review findings and harden the share flows Gate every share entry point on login, de-duplicate the share-link and color-identity logic behind DeckShareUtils and an injected querier, and replace the silent tray/status-bar notices with always-visible dialogs. - abstract_tab_deck_editor: explain that sharing requires a connection instead of silently doing nothing when logged out - tab_deck_storage: disable the share action on disconnect, reject folder/deck mixes and the root folder with clear warnings, re-enable Create on every entry/response so a dropped connection cannot leave the button disabled - tab_deck_storage_visual: same login gate for the context-menu entry, visible success/error dialogs, and a symmetric in-flight guard - getDeckColorIdentity now takes a CardDatabaseQuerier, dropping the CardDatabaseManager singleton access and enabling unit tests --- .../additional_info/deck_color_identity.cpp | 6 +- .../additional_info/deck_color_identity.h | 5 +- .../widgets/deck_share/share_bar_widget.cpp | 5 ++ .../widgets/deck_share/share_bar_widget.h | 3 + .../widgets/dialogs/dlg_share_deck.cpp | 7 +- .../widgets/dialogs/dlg_share_deck.h | 2 + .../widgets/tabs/abstract_tab_deck_editor.cpp | 5 ++ .../widgets/tabs/tab_deck_storage.cpp | 88 ++++++++++++------- .../interface/widgets/tabs/tab_deck_storage.h | 2 +- .../tab_deck_storage_visual.cpp | 29 +++--- .../tab_deck_storage_visual.h | 2 +- .../visual_deck_storage_widget.cpp | 2 +- 12 files changed, 103 insertions(+), 53 deletions(-) diff --git a/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.cpp b/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.cpp index dd6f4244b..62b01511e 100644 --- a/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.cpp +++ b/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.cpp @@ -1,11 +1,11 @@ #include "deck_color_identity.h" #include -#include +#include #include #include -QString getDeckColorIdentity(const DeckList &deck) +QString getDeckColorIdentity(const DeckList &deck, const CardDatabaseQuerier *db) { const QStringList cardList = deck.getCardList({DECK_ZONE_MAIN, DECK_ZONE_SIDE}); if (cardList.isEmpty()) { @@ -15,7 +15,7 @@ QString getDeckColorIdentity(const DeckList &deck) QSet colorSet; // A set to collect unique color symbols (e.g., W, U, B, R, G) for (const QString &cardName : cardList) { - CardInfoPtr currentCard = CardDatabaseManager::query()->getCardInfo(cardName); + CardInfoPtr currentCard = db->getCardInfo(cardName); if (currentCard) { const QString colors = currentCard->getColors(); // returns something like "WUB" for (const QChar &color : colors) { diff --git a/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.h b/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.h index 93d454c58..04294cb1c 100644 --- a/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.h +++ b/cockatrice/src/interface/widgets/cards/additional_info/deck_color_identity.h @@ -3,6 +3,7 @@ #include +class CardDatabaseQuerier; class DeckList; /** @@ -11,7 +12,9 @@ class DeckList; * * Shared as a free function so the deck storage previews and the deck share * dialog compute identities identically. + * + * @param db Card database used to look up card color symbols. */ -QString getDeckColorIdentity(const DeckList &deck); +QString getDeckColorIdentity(const DeckList &deck, const CardDatabaseQuerier *db); #endif // COCKATRICE_DECK_COLOR_IDENTITY_H diff --git a/cockatrice/src/interface/widgets/deck_share/share_bar_widget.cpp b/cockatrice/src/interface/widgets/deck_share/share_bar_widget.cpp index 584e5237f..3fe5917dc 100644 --- a/cockatrice/src/interface/widgets/deck_share/share_bar_widget.cpp +++ b/cockatrice/src/interface/widgets/deck_share/share_bar_widget.cpp @@ -67,6 +67,11 @@ void ShareBarWidget::setHintText(const QString &text, bool visible) hintLabel->setVisible(visible); } +void ShareBarWidget::setCreateEnabled(bool enabled) +{ + createButton->setEnabled(enabled); +} + void ShareBarWidget::focusName() { nameEdit->setFocus(); diff --git a/cockatrice/src/interface/widgets/deck_share/share_bar_widget.h b/cockatrice/src/interface/widgets/deck_share/share_bar_widget.h index 828755fa3..f3b902da8 100644 --- a/cockatrice/src/interface/widgets/deck_share/share_bar_widget.h +++ b/cockatrice/src/interface/widgets/deck_share/share_bar_widget.h @@ -42,6 +42,9 @@ public: /** @brief Sets the explainer hint text, showing it when @p visible is true. */ void setHintText(const QString &text, bool visible); + /** @brief Enables or disables the create-share-link button (guards double submission). */ + void setCreateEnabled(bool enabled); + /** @brief Moves keyboard focus to the name field. */ void focusName(); diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.cpp b/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.cpp index fdef84673..f5f7f295c 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.cpp +++ b/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -36,11 +37,14 @@ DlgShareDeck::DlgShareDeck(AbstractClient *_client, const QSharedPointerbutton(QDialogButtonBox::Cancel)->setText(tr("Cancel")); connect(buttonBox, &QDialogButtonBox::accepted, this, &DlgShareDeck::actShare); connect(buttonBox, &QDialogButtonBox::rejected, this, &DlgShareDeck::reject); + this->buttonBox = buttonBox; layout->addWidget(buttonBox); } void DlgShareDeck::actShare() { + buttonBox->button(QDialogButtonBox::Ok)->setEnabled(false); + Command_DeckShareCreate cmd; cmd.set_name(nameEdit->text().trimmed().toStdString()); if (cmd.name().empty()) { @@ -49,7 +53,7 @@ void DlgShareDeck::actShare() DeckShareItem *item = cmd.add_items(); item->set_deck_list(deck->writeToString_Native().toStdString()); - item->set_color_identity(getDeckColorIdentity(*deck).toStdString()); + item->set_color_identity(getDeckColorIdentity(*deck, CardDatabaseManager::query()).toStdString()); PendingCommand *pend = client->prepareSessionCommand(cmd); connect(pend, &PendingCommand::finished, this, &DlgShareDeck::shareFinished); @@ -59,6 +63,7 @@ void DlgShareDeck::actShare() void DlgShareDeck::shareFinished(const Response &response, const CommandContainer & /*commandContainer*/) { if (response.response_code() != Response::RespOk) { + buttonBox->button(QDialogButtonBox::Ok)->setEnabled(true); QMessageBox::critical(this, tr("Share deck"), tr("Failed to create the share link (server response code %1).") .arg(QString::number(static_cast(response.response_code())))); diff --git a/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.h b/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.h index 30c28fb73..0e8046bcd 100644 --- a/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.h +++ b/cockatrice/src/interface/widgets/dialogs/dlg_share_deck.h @@ -13,6 +13,7 @@ class AbstractClient; class CommandContainer; class DeckList; +class QDialogButtonBox; class QLineEdit; class Response; @@ -36,6 +37,7 @@ private: AbstractClient *client; QSharedPointer deck; QLineEdit *nameEdit; + QDialogButtonBox *buttonBox; }; #endif // DLG_SHARE_DECK_H \ No newline at end of file diff --git a/cockatrice/src/interface/widgets/tabs/abstract_tab_deck_editor.cpp b/cockatrice/src/interface/widgets/tabs/abstract_tab_deck_editor.cpp index 0b830c32d..3bd60ad32 100644 --- a/cockatrice/src/interface/widgets/tabs/abstract_tab_deck_editor.cpp +++ b/cockatrice/src/interface/widgets/tabs/abstract_tab_deck_editor.cpp @@ -388,6 +388,11 @@ bool AbstractTabDeckEditor::actSaveDeckAs() */ void AbstractTabDeckEditor::actShareDeck() { + if (tabSupervisor->getClient()->getStatus() != StatusLoggedIn) { + QMessageBox::information(this, tr("Share deck"), tr("You must be connected to the server to share a deck.")); + return; + } + const QSharedPointer deck = deckStateManager->getDeckListShared(); if (deck->isBlankDeck()) { return; diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp index c7a0b69d8..0794ce709 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.cpp @@ -1,30 +1,25 @@ #include "tab_deck_storage.h" #include "../../../client/settings/cache_settings.h" -#include "../../../main.h" #include "../../deck_loader/deck_loader.h" #include "../../pixel_map_generator.h" +#include "../deck_share/deck_share_utils.h" #include "../deck_share/share_bar_widget.h" #include "../interface/widgets/server/remote/remote_decklist_tree_widget.h" #include "../interface/widgets/utility/get_text_with_max.h" #include #include -#include #include #include #include #include #include -#include #include #include #include #include -#include #include -#include -#include #include #include #include @@ -249,12 +244,14 @@ void TabDeckStorage::setRemoteEnabled(bool enabled) aUpload->setEnabled(enabled); aOpenRemoteDeck->setEnabled(enabled); aDownload->setEnabled(enabled); + aShareDecks->setEnabled(enabled); aNewFolder->setEnabled(enabled); aDeleteRemoteDeck->setEnabled(enabled); if (enabled) { serverDirView->refreshTree(); } else { + setShareModeEnabled(false); serverDirView->clearTree(); } } @@ -670,6 +667,7 @@ void TabDeckStorage::setShareModeEnabled(bool enabled) { shareBar->setVisible(enabled); if (enabled) { + shareBar->setCreateEnabled(true); shareBar->setName(tr("Shared decks")); onServerSelectionChanged(); shareBar->focusName(); @@ -693,6 +691,17 @@ void TabDeckStorage::onServerSelectionChanged() ++files; } } + + QString hint; + if (folders > 1) { + hint = tr("Only one folder can be shared at a time."); + } else if (folders > 0 && files > 0) { + hint = tr("Share either a folder or decks, not both."); + } else if (folders == 0 && files == 0) { + hint = tr("Select folders or decks in the tree to share."); + } + shareBar->setHintText(hint, !hint.isEmpty()); + QStringList parts; if (folders > 0) { parts << tr("%n folder(s)", "", folders); @@ -706,6 +715,30 @@ void TabDeckStorage::onServerSelectionChanged() void TabDeckStorage::actShareSelection() { const auto selection = serverDirView->getCurrentSelection(); + QString sharedFolder; + bool hasFile = false; + bool hasFolder = false; + for (const auto *node : selection) { + if (const auto *dirNode = dynamic_cast(node)) { + hasFolder = true; + if (!sharedFolder.isEmpty()) { + showShareNotice(tr("Only one folder can be shared at a time."), true); + return; + } + sharedFolder = dirNode->getPath(); + } else { + hasFile = true; + } + } + + if (hasFile && hasFolder) { + showShareNotice(tr("Share either a folder or decks, not both."), true); + return; + } + if (hasFolder && sharedFolder.isEmpty()) { + showShareNotice(tr("The root folder cannot be shared."), true); + return; + } Command_DeckShareCreate cmd; cmd.set_name(shareBar->name().toStdString()); @@ -713,19 +746,9 @@ void TabDeckStorage::actShareSelection() cmd.set_name(tr("Shared decks").toStdString()); } - // Sharing a folder is exclusive with sharing individual decks (matches the picker's rule). - for (const auto *node : selection) { - if (const auto *dirNode = dynamic_cast(node)) { - const QString path = dirNode->getPath(); - if (path.isEmpty()) { - continue; // the root folder cannot be shared - } - cmd.set_folder_path(path.toStdString()); - break; - } - } - - if (cmd.folder_path().empty()) { + if (!sharedFolder.isEmpty()) { + cmd.set_folder_path(sharedFolder.toStdString()); + } else { for (const auto *node : selection) { if (const auto *fileNode = dynamic_cast(node)) { DeckShareItem *item = cmd.add_items(); @@ -735,10 +758,11 @@ void TabDeckStorage::actShareSelection() } if (cmd.items_size() == 0 && cmd.folder_path().empty()) { - showShareNotice(tr("Select decks to share.")); + showShareNotice(tr("Select decks to share."), true); return; } + shareBar->setCreateEnabled(false); PendingCommand *pend = client->prepareSessionCommand(cmd); connect(pend, &PendingCommand::finished, this, &TabDeckStorage::shareFromTreeFinished); client->sendCommand(pend); @@ -746,29 +770,29 @@ void TabDeckStorage::actShareSelection() void TabDeckStorage::shareFromTreeFinished(const Response &response, const CommandContainer & /*commandContainer*/) { + shareBar->setCreateEnabled(true); if (response.response_code() != Response::RespOk) { qWarning() << "failed to create deck share:" << response.response_code(); showShareNotice(tr("Failed to create the share link (server response code %1).") - .arg(QString::number(static_cast(response.response_code())))); + .arg(QString::number(static_cast(response.response_code()))), + true); return; } const Response_DeckShareCreate &resp = response.GetExtension(Response_DeckShareCreate::ext); const QString token = QString::fromStdString(resp.token()); - const QDateTime expiry = QDateTime::fromSecsSinceEpoch(resp.expires_at(), QTimeZone::UTC).toLocalTime(); + const QDateTime expiry = QDateTime::fromSecsSinceEpoch(resp.expires_at(), QTimeZone::UTC); - const QString link = QString("cockatrice://opendeck?share=%1&hostname=%2&port=%3") - .arg(token, client->serverName(), QString::number(client->serverPort())); - QGuiApplication::clipboard()->setText(link); + const QString link = DeckShareUtils::buildShareLink(client, token); + DeckShareUtils::copyShareLinkToClipboard(link); - showShareNotice(tr("Share link copied to the clipboard.\nExpires on %1.").arg(expiry.toString())); + showShareNotice( + tr("Share link copied to the clipboard.\nExpires on %1.").arg(DeckShareUtils::formatShareExpiry(expiry))); setShareModeEnabled(false); } -void TabDeckStorage::showShareNotice(const QString &message) +void TabDeckStorage::showShareNotice(const QString &message, bool warning) { - if (trayIcon && trayIcon->isVisible()) { - trayIcon->showMessage(tr("Deck share"), message); - } else if (auto *mainWindow = qobject_cast(window())) { - mainWindow->statusBar()->showMessage(message, 10000); - } + QMessageBox box(warning ? QMessageBox::Warning : QMessageBox::Information, tr("Deck share"), message, + QMessageBox::Ok, this); + box.exec(); } diff --git a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h index 5f28f7d55..201506789 100644 --- a/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h +++ b/cockatrice/src/interface/widgets/tabs/tab_deck_storage.h @@ -45,7 +45,7 @@ private: void setRemoteEnabled(bool enabled); - void showShareNotice(const QString &message); + void showShareNotice(const QString &message, bool warning = false); void setShareModeEnabled(bool enabled); diff --git a/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.cpp b/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.cpp index 558c080c2..fca9c9f0c 100644 --- a/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.cpp +++ b/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.cpp @@ -1,17 +1,15 @@ #include "tab_deck_storage_visual.h" -#include "../../../../main.h" #include "../../../deck_loader/deck_loader.h" #include "../../cards/additional_info/deck_color_identity.h" #include "../../deck_share/deck_share_utils.h" #include "../../interface/widgets/visual_deck_storage/visual_deck_storage_widget.h" #include "../tab_supervisor.h" -#include +#include #include -#include -#include #include +#include #include #include #include @@ -82,6 +80,7 @@ void TabDeckStorageVisual::enterShareMode(const QStringList &preselectFiles) if (!shareDeckAvailable) { return; // sharing is gated on being logged in } + shareBar->setCreateEnabled(true); visualDeckStorageWidget->setShareSelectable(true); visualDeckStorageWidget->setShareSelectedFiles(preselectFiles); shareBar->setName(tr("Shared decks")); @@ -99,6 +98,10 @@ void TabDeckStorageVisual::exitShareMode() void TabDeckStorageVisual::actShareDeck(const QString &filePath) { + if (!shareDeckAvailable) { + QMessageBox::information(this, tr("Share deck"), tr("You must be connected to the server to share a deck.")); + return; + } enterShareMode({filePath}); } @@ -145,9 +148,10 @@ void TabDeckStorageVisual::actShareSelected() } DeckShareItem *item = cmd.add_items(); item->set_deck_list(deckOpt->deckList.writeToString_Native().toStdString()); - item->set_color_identity(getDeckColorIdentity(deckOpt->deckList).toStdString()); + item->set_color_identity(getDeckColorIdentity(deckOpt->deckList, CardDatabaseManager::query()).toStdString()); } + shareBar->setCreateEnabled(false); PendingCommand *pend = tabSupervisor->getClient()->prepareSessionCommand(cmd); connect(pend, &PendingCommand::finished, this, &TabDeckStorageVisual::shareFinished); tabSupervisor->getClient()->sendCommand(pend); @@ -155,10 +159,11 @@ void TabDeckStorageVisual::actShareSelected() void TabDeckStorageVisual::shareFinished(const Response &response, const CommandContainer & /*commandContainer*/) { + shareBar->setCreateEnabled(true); if (response.response_code() != Response::RespOk) { showShareNotice(tr("Failed to create the share link (server response code %1).") - .arg(QString::number(static_cast(response.response_code())))); - exitShareMode(); + .arg(QString::number(static_cast(response.response_code()))), + true); return; } @@ -183,11 +188,9 @@ void TabDeckStorageVisual::handleConnectionChanged(ClientStatus status) } } -void TabDeckStorageVisual::showShareNotice(const QString &message) +void TabDeckStorageVisual::showShareNotice(const QString &message, bool warning) { - if (trayIcon && trayIcon->isVisible()) { - trayIcon->showMessage(tr("Deck share"), message); - } else if (auto *mainWindow = qobject_cast(window())) { - mainWindow->statusBar()->showMessage(message, 10000); - } + QMessageBox box(warning ? QMessageBox::Warning : QMessageBox::Information, tr("Deck share"), message, + QMessageBox::Ok, this); + box.exec(); } \ No newline at end of file diff --git a/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.h b/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.h index c91977ad4..da9f037b4 100644 --- a/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.h +++ b/cockatrice/src/interface/widgets/tabs/visual_deck_storage/tab_deck_storage_visual.h @@ -67,7 +67,7 @@ private slots: void handleConnectionChanged(ClientStatus status); private: - void showShareNotice(const QString &message); + void showShareNotice(const QString &message, bool warning = false); void updateShareHint(); VisualDeckStorageWidget *visualDeckStorageWidget; diff --git a/cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_widget.cpp b/cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_widget.cpp index 691b4691b..7a786773d 100644 --- a/cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_widget.cpp +++ b/cockatrice/src/interface/widgets/visual_deck_storage/visual_deck_storage_widget.cpp @@ -51,7 +51,7 @@ VisualDeckStorageWidget::VisualDeckStorageWidget(QWidget *parent) : QWidget(pare shareButton = new QToolButton(this); shareButton->setIcon(QPixmap("theme:icons/share")); shareButton->setFixedSize(32, 32); - shareButton->setToolTip(tr("Share selected decks")); + shareButton->setToolTip(tr("Select decks to share")); shareButton->setVisible(false); connect(shareButton, &QPushButton::clicked, this, &VisualDeckStorageWidget::shareRequested);