Disable saving of decks when the deck is empty #3384
Conversation
cockatrice/src/tab_deck_editor.cpp
Outdated
| DeckLoader *const deck = deckModel->getDeckList(); | ||
| QString decklistUrlString; | ||
| if (deck) { | ||
| decklistUrlString = deck->exportDeckToDecklist(); |
There was a problem hiding this comment.
I know this is only run when a card is removed, but it looks to me like a lot of wasted cpu cycles.
Why not use deck->isEmpty()?
There was a problem hiding this comment.
Because I couldn't find it, but I see it now! Will update the branch with the suggested change
There was a problem hiding this comment.
The basics do work in-client! 👍
I stumbled upon these cases that are not covered yet:
New deckandLoad deck...doesn't change the status of the menu options yet.
Not covered by the initial issue, but highly related would be to take care of the
Save deck to clipboard option as well.
cockatrice/src/tab_deck_editor.cpp
Outdated
| aSaveDeck->setEnabled(false); | ||
| aSaveDeckAs->setEnabled(false); | ||
| } | ||
| else { |
There was a problem hiding this comment.
I'll get on all that stuff right away!
Speaking of style errors, I noticed a lot of if/else statements omitting braces for single-statements which goes against the code style guide. Would there be any utility to refactoring them to match the style guide?
There was a problem hiding this comment.
Normally clang-format should catch and report those. It can also fix them with a simple command, that's why we use it. :)
Speaking about that, we still have no description setup on how to properly use it... we point to the CONTRIBUTING file from within the error reported in travis, but there is no endpoint yet. (--> #3065)
Maybe somebody else know what's up with the if/else style you mentioned.
Config got added and discussed in #3028 by the way.
In the meantime can you point to 1-2 examples?
There was a problem hiding this comment.
This if/else block contradicts the Braces section in the contributing guide in that the braces are not on their own line.
This also contradicts the Braces section of the Contributing guide since the braces are omitted.
I think maybe it's the contributing guide that should be updated to reflect the way the automatic code review bots enforce the code styling?
leestran1995
force-pushed
the
leestran-fixes
branch
from
1702d4e
to
5ba13dd
Compare
|
The sub-options of |
|
What about print deck and send deck to online service? Shouldnt they be disabled too? |
cockatrice/src/tab_deck_editor.cpp
Outdated
| @@ -988,6 +995,13 @@ void TabDeckEditor::actRemoveCard() | |||
| if (!currentIndex.isValid() || deckModel->hasChildren(currentIndex)) | |||
| return; | |||
| deckModel->removeRow(currentIndex.row(), currentIndex.parent()); | |||
|
|
|||
| DeckLoader *const deck = deckModel->getDeckList(); | |||
| if (deck->isEmpty()) { | |||
There was a problem hiding this comment.
Can simplify to setSaveStatus(!deck->isEmpty())
There was a problem hiding this comment.
Of course! I'll commit that ASAP
|
@tooomm are you ok with this? Shall we merge? |
|
Can check tomorrow again. |
|
Right after loading a deck, the options shouldn't be disabled. Example workflows which are not possible due to the current behavior: |
|
Any news here @leestran1995? |
|
@tooomm aaahhhh very sorry, I've had a busy couple weeks. I'll get to this tomorrow and get those last fixes in! |
|
No worries, great! 🎉 |
|
@ctrlaltca do you know what's up with the appveyor failures? |
|
@ZeldaZach ready to go :) |
|
It technically works, so I'll get this in :) |
Related Ticket(s)
Short roundup of the initial problem
What will change with this Pull Request?
Screenshots