Multiple bg images zone #4005
Multiple bg images zone #4005
Conversation
There was a problem hiding this comment.
Your pull request is not synced with master, you should have the remote for the main project in your git repo and do a git merge --strategy-option=ours [main master] to make sure no conflicts arise on master while you are working on this or a future pull request.
I see this fixes the issues in the current version, though I wonder if it could also be possible to have the pictures be layed out in a fashion that is dependent on their position instead of the player id.
I added some comments, be sure to use const & wherever possible for optimization.
cockatrice/src/player.cpp
Outdated
| @@ -104,6 +104,8 @@ Player::Player(const ServerInfo_User &info, int _id, bool _local, bool _judge, T | |||
| connect(settingsCache, SIGNAL(horizontalHandChanged()), this, SLOT(rearrangeZones())); | |||
| connect(settingsCache, SIGNAL(handJustificationChanged()), this, SLOT(rearrangeZones())); | |||
|
|
|||
| zoneId = 0; | |||
There was a problem hiding this comment.
initialize this together with the rest 5 lines up please
cockatrice/src/tab_game.cpp
Outdated
| @@ -802,6 +802,27 @@ Player *TabGame::addPlayer(int playerId, const ServerInfo_User &info) | |||
| gameMenu->insertMenu(playersSeparator, newPlayer->getPlayerMenu()); | |||
|
|
|||
| players.insert(playerId, newPlayer); | |||
|
|
|||
| int i = 1; | |||
There was a problem hiding this comment.
I use it a few lines below for this newPlayer->setZoneId(i);, maybe should I give the variable a more significant name?
There was a problem hiding this comment.
no you should move that inside the loop
cockatrice/src/tab_game.cpp
Outdated
| for (i = 1; i <= players.count(); ++i) { | ||
| bool aPlayerHasThisZone = false; | ||
| QMapIterator<int, Player *> playerIterator(players); | ||
| while (playerIterator.hasNext()) { |
There was a problem hiding this comment.
we prefer for (auto &item : array) loops over java style iterators in new pull requests
cockatrice/src/tab_game.cpp
Outdated
| } | ||
| } | ||
|
|
||
| if (!spectators.contains(playerId)) { |
There was a problem hiding this comment.
this check can be done earlier I think
cockatrice/src/thememanager.cpp
Outdated
| @@ -107,3 +121,8 @@ void ThemeManager::themeChangedSlot() | |||
|
|
|||
| emit themeChanged(); | |||
| } | |||
|
|
|||
| QBrush ThemeManager::getExtraTableBgBrush(QString extraNumber, QBrush fallbackBrush) | |||
There was a problem hiding this comment.
You should use references with & for these functions.
cockatrice/src/thememanager.cpp
Outdated
|
|
||
| QBrush ThemeManager::getExtraTableBgBrush(QString extraNumber, QBrush fallbackBrush) | ||
| { | ||
| return loadExtraBrush(TABLEZONE_BG_NAME + extraNumber, fallbackBrush); |
There was a problem hiding this comment.
it'd be preferable to store these in an array or map instead of needing to load them repeatedly.
|
Hey @fdipilla thanks for helping to improve on Cockatrice! I feel this is a duplicate of PR #3990 so I'll be closing it for now, but if you find there's more to bring to the table please let us know! Feel free to reach out at https://cockatrice.us/discord anytime :) |
|
It's also mentioned in the wiki docs 👍 |
|
I honestly thought this was already merged months ago.... |
|
The first version was. #3990 |
|
I know, and I thought this fix was already added |
Related Ticket(s)
Short roundup of the initial problem
Give the user the possibility of using multiple background images for each player
What will change with this Pull Request?
Screenshots