[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
This commit is contained in:
Lukas Brübach 2026-09-05 01:13:55 +02:00 committed by GitHub
parent ad6d33c1e2
commit 0bfa89b5d3
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
12 changed files with 103 additions and 53 deletions

View file

@ -1,11 +1,11 @@
#include "deck_color_identity.h"
#include <QSet>
#include <libcockatrice/card/database/card_database_manager.h>
#include <libcockatrice/card/database/card_database_querier.h>
#include <libcockatrice/deck_list/deck_list.h>
#include <libcockatrice/deck_list/tree/inner_deck_list_node.h>
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<QChar> 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) {

View file

@ -3,6 +3,7 @@
#include <QString>
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

View file

@ -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();

View file

@ -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();

View file

@ -10,6 +10,7 @@
#include <QPushButton>
#include <QTimeZone>
#include <QVBoxLayout>
#include <libcockatrice/card/database/card_database_manager.h>
#include <libcockatrice/deck_list/deck_list.h>
#include <libcockatrice/network/client/abstract/abstract_client.h>
#include <libcockatrice/protocol/pb/command_deck_share_create.pb.h>
@ -36,11 +37,14 @@ DlgShareDeck::DlgShareDeck(AbstractClient *_client, const QSharedPointer<DeckLis
buttonBox->button(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<int>(response.response_code()))));

View file

@ -13,6 +13,7 @@
class AbstractClient;
class CommandContainer;
class DeckList;
class QDialogButtonBox;
class QLineEdit;
class Response;
@ -36,6 +37,7 @@ private:
AbstractClient *client;
QSharedPointer<DeckList> deck;
QLineEdit *nameEdit;
QDialogButtonBox *buttonBox;
};
#endif // DLG_SHARE_DECK_H

View file

@ -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<DeckList> deck = deckStateManager->getDeckListShared();
if (deck->isBlankDeck()) {
return;

View file

@ -1,29 +1,24 @@
#include "tab_deck_storage.h"
#include "../../../client/settings/cache_settings.h"
#include "../../../main.h"
#include "../../deck_loader/deck_loader.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 <QAction>
#include <QApplication>
#include <QClipboard>
#include <QDateTime>
#include <QDebug>
#include <QDesktopServices>
#include <QFileSystemModel>
#include <QGroupBox>
#include <QGuiApplication>
#include <QHBoxLayout>
#include <QHeaderView>
#include <QInputDialog>
#include <QLineEdit>
#include <QMainWindow>
#include <QMessageBox>
#include <QStatusBar>
#include <QSystemTrayIcon>
#include <QTimeZone>
#include <QToolBar>
#include <QTreeView>
@ -248,12 +243,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();
}
}
@ -669,6 +666,7 @@ void TabDeckStorage::setShareModeEnabled(bool enabled)
{
shareBar->setVisible(enabled);
if (enabled) {
shareBar->setCreateEnabled(true);
shareBar->setName(tr("Shared decks"));
onServerSelectionChanged();
shareBar->focusName();
@ -692,6 +690,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);
@ -705,6 +714,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<const RemoteDeckList_TreeModel::DirectoryNode *>(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());
@ -712,19 +745,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<const RemoteDeckList_TreeModel::DirectoryNode *>(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<const RemoteDeckList_TreeModel::FileNode *>(node)) {
DeckShareItem *item = cmd.add_items();
@ -734,10 +757,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);
@ -745,29 +769,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<int>(response.response_code()))));
.arg(QString::number(static_cast<int>(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<QMainWindow *>(window())) {
mainWindow->statusBar()->showMessage(message, 10000);
}
QMessageBox box(warning ? QMessageBox::Warning : QMessageBox::Information, tr("Deck share"), message,
QMessageBox::Ok, this);
box.exec();
}

View file

@ -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);

View file

@ -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 <QMainWindow>
#include <QDateTime>
#include <QMessageBox>
#include <QStatusBar>
#include <QSystemTrayIcon>
#include <QVBoxLayout>
#include <libcockatrice/card/database/card_database_manager.h>
#include <libcockatrice/deck_list/deck_list.h>
#include <libcockatrice/network/client/abstract/abstract_client.h>
#include <libcockatrice/protocol/pb/command_deck_share_create.pb.h>
@ -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<int>(response.response_code()))));
exitShareMode();
.arg(QString::number(static_cast<int>(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<QMainWindow *>(window())) {
mainWindow->statusBar()->showMessage(message, 10000);
}
QMessageBox box(warning ? QMessageBox::Warning : QMessageBox::Information, tr("Deck share"), message,
QMessageBox::Ok, this);
box.exec();
}

View file

@ -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;

View file

@ -50,7 +50,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);