Add keybaord shortcut for creating related cards #2426
Conversation
|
I'm not sure if this is the best/right approach. |
cockatrice/src/player.cpp
Outdated
|
|
||
| QStringList relatedCards = * new QStringList(); | ||
| relatedCards.append(sourceCard->getInfo()->getRelatedCards()); | ||
| relatedCards.append(sourceCard->getInfo()->getReverseRelatedCards2Me()); |
There was a problem hiding this comment.
While creating a new QStringList makes the code more readable, the second "append" forces Qt to make a deep copy of the list, with all the overhead involved in memory allocation, copy, etc..
This is not a big deal since the "related cards" list would only contain a few hundreds of bytes at max..
cockatrice/src/player.cpp
Outdated
| relatedCards.append(sourceCard->getInfo()->getRelatedCards()); | ||
| relatedCards.append(sourceCard->getInfo()->getReverseRelatedCards2Me()); | ||
|
|
||
| for (int i = 0; i < relatedCards.size(); i++) |
There was a problem hiding this comment.
You can use foreach with QStringList:
foreach (const QString &tokenName, relatedCards)
{
createCard(sourceCard, dbNameFromTokenDisplayName(tokenName));
}
| // token names take the form of "<Descriptors> <Power>/<Toughness> <Card Name> " or "<Card Name> ". | ||
| // dbName for tokens should take the form of "<Card Name> ". | ||
| // trailing whitespace is significant; it is hacked on at the end as an additional identifier in our single key database | ||
| QString Player::dbNameFromTokenDisplayName(const QString &tokenName) { |
There was a problem hiding this comment.
👍 Nice refactoring, it makes clear the meaning of this code.
| @@ -2425,6 +2424,44 @@ void Player::updateCardMenu(CardItem *card) | |||
| } | |||
| } | |||
|
|
|||
| void Player::addRelatedCardActions(const CardItem *card, QMenu *cardMenu) { | |||
There was a problem hiding this comment.
👍 Another nice refactoring, less duplicated code
There was a problem hiding this comment.
To be safe I think we should have nullptr checks on both arguments.
cockatrice/src/player.h
Outdated
| @@ -4,6 +4,7 @@ | |||
| #include <QInputDialog> | |||
| #include <QPoint> | |||
| #include <QMap> | |||
| #include <QRegExp> | |||
There was a problem hiding this comment.
You can move this into the .cpp file, since it's not used inside the .h code.
It will avoid the inclusion of <QRegExp> in all the other files #including player.h but not using it.
There was a problem hiding this comment.
The approach and refactoring look great.
Thanks for the contribution!
@ctrlaltca go ahead and merge when you feel it's ready
cockatrice/src/player.cpp
Outdated
| setShortcutsActive(); | ||
|
|
||
| for (int i = 0; i < table->getCards().size(); ++i) { |
There was a problem hiding this comment.
Prefer using foreach here also as ctrl said
| @@ -2425,6 +2424,44 @@ void Player::updateCardMenu(CardItem *card) | |||
| } | |||
| } | |||
|
|
|||
| void Player::addRelatedCardActions(const CardItem *card, QMenu *cardMenu) { | |||
There was a problem hiding this comment.
To be safe I think we should have nullptr checks on both arguments.
|
Thanks for the feedback - I think I handled everything except for the deep copy part. To me the deeper issue is that the this is the only place in the code base outside of importing from oracle that we refer to either of |
|
I'm happy with the change. nice job! |
|
Merged, thank you! |
You're right - only the card db should care about the difference. The rest of the code would rather just one list. I'd happily accept a PR with such a refactor. Thanks for helping improve the health of the codebase! It's grown really organically over time and needs some aggressive pruning and decoupling |
Finishes #2425.
Once again, c++ isn't my forte, so let me know if I can clean anything up.