Change cards from sb<->mb with double-click #2606

ZeldaZach · 2017-04-20T05:34:37Z

Related Ticket(s)

This PR allows a person to double click on a card to change it from sideboard to mainboard and vice versa. This will not impact tokens (if you imported them to the deck) and will work in the exact same context as the drag/drop method.

ghost · 2017-04-20T08:21:10Z

cockatrice/src/deckview.cpp

@@ -1,5 +1,6 @@
 #include <QApplication>
 #include <QGraphicsSceneMouseEvent>
+#include <QDebug>


Is QDebug needed in the PR?
I see that it outputs "LEFT", though I assume this was for your own testing.

This is a WIP, so I have debugging information in here for my purposes. Will all be removed at the end.

ghost · 2017-04-20T08:32:21Z

cockatrice/src/deckview.cpp

+            m.set_card_name(c->getName().toStdString());
+            m.set_start_zone(c->getOriginZone().toStdString());
+
+            if (c->getOriginZone() == "main")


Aren't there some public const variables for these named zones?

Good catch.

Can you add some constants for the special zones?

Yeah i'll add that to the PR

The main problem now is that you are using getOriginZone() to determine where a card actually is and where it should move to, but the "original zone" is never updated, so if you move a card from "main" to "side" its original zone will still be "main"; double clicking it again will try to move it again from the main to the side.
You don't want to che the card "original zone", but its "current zone" instead; you should be able to get this information from the card's parentItem()

ZeldaZach · 2017-04-20T08:37:40Z

cockatrice/src/deckview.cpp

+    if (event->button() == Qt::LeftButton)
+    {
+        qDebug() << "LEFT";
+        QList<MoveCard_ToZone> *result = new QList<MoveCard_ToZone>();


Left it allocated on purpose until I could get it to work, then would find a safe place to de-allocate

ctrlaltca · 2017-04-20T09:47:27Z

cockatrice/src/deckview.cpp

@@ -1,5 +1,6 @@
 #include <QApplication>
 #include <QGraphicsSceneMouseEvent>
+#include <QDebug>


Add an #include <QMouseEvent>, should fix compilation:
https://travis-ci.org/Cockatrice/Cockatrice/jobs/223830200#L1507
https://ci.appveyor.com/project/Daenyth/cockatrice-8k7y5/build/0.0.1-branch-master-build-809/job/2p5nwyxxn0l0jq7l#L3374

ctrlaltca · 2017-04-20T09:54:29Z

cockatrice/src/tab_game.cpp

@@ -274,13 +275,26 @@ void DeckViewContainer::sideboardLockButtonClicked()

 void DeckViewContainer::sideboardPlanChanged()


Please refactor this method in order to make use of your new method, to avoid code duplication.
Alternatively, just drop it and make the old code call directly your method.

ctrlaltca · 2017-04-20T10:05:48Z

cockatrice/src/tab_game.cpp

+        cmd.add_move_list()->CopyFrom(newPlan->at(i));
+    parentGame->sendGameCommand(cmd, playerId);
+
+    // TODO: Need to figure out how to re-paint the deck view


DeckViewScene::applySideboardPlan() is probably a good starting point.
You probably want to handle this inside DeckView::mouseDoubleClickEvent, get the old and new DeckViewCardContainer zones from the scene()->cardContainers list, and then move the card from one zone to another the same way it's implemented in DeckViewCardDragItem::handleDrop

ZeldaZach · 2017-04-20T19:55:05Z

So the current WIP has some issues still, but it's a step in the right direction.

Cards move on double click now to an opposite zone. Issue with this is if I move a card from main to side, then if I click on any copy of that card in either it will force it to the side and cannot be returned to main.
When this move happens, the server isn't updated with the settings and the deck, when you start the game, still shows you as the 60 card main deck (even if you made changes)

tooomm · 2017-04-20T22:03:42Z

Oh wow, codacy even does reviews of open pr's with comments and info!? sweeet

ZeldaZach · 2017-04-22T22:37:41Z

Any further help @ctrlaltca @Daenyth ?

ctrlaltca · 2017-04-23T20:41:29Z

cockatrice/src/tab_game.cpp

 {
+    QList<MoveCard_ToZone> newPlan = *input;


You should check if input is null before dereferencing the pointer

ctrlaltca · 2017-04-23T20:42:41Z

cockatrice/src/deckview.h

@@ -95,6 +94,9 @@ class DeckViewScene : public QGraphicsScene {
    void updateContents();
    QList<MoveCard_ToZone> getSideboardPlan() const;
    void resetSideboardPlan();
+    QMap<QString, DeckViewCardContainer *> getCardContainers() { return cardContainers; };


Unused, remove?

ctrlaltca · 2017-04-23T20:43:33Z

cockatrice/src/deckview.h

 public:
    DeckViewCardDragItem(DeckViewCard *_item, const QPointF &_hotSpot, AbstractCardDragItem *parentDrag = 0);
    void updatePosition(const QPointF &cursorScenePos);
+    void handleDrop(DeckViewCardContainer *target);


is making this method public this really needed?

ctrlaltca · 2017-04-23T20:45:02Z

cockatrice/src/deckview.cpp

@@ -462,7 +494,8 @@ DeckView::DeckView(QWidget *parent)

    connect(deckViewScene, SIGNAL(sceneRectChanged(const QRectF &)), this, SLOT(updateSceneRect(const QRectF &)));
    connect(deckViewScene, SIGNAL(newCardAdded(AbstractCardItem *)), this, SIGNAL(newCardAdded(AbstractCardItem *)));
-    connect(deckViewScene, SIGNAL(sideboardPlanChanged()), this, SIGNAL(sideboardPlanChanged()));
+    connect(deckViewScene, SIGNAL(sideboardPlanChanged(QList<MoveCard_ToZone>*)), this, SIGNAL(sideboardPlanChanged(QList<MoveCard_ToZone>*)));
+    connect(this, SIGNAL(sideboardPlanChanged(QList<MoveCard_ToZone>*)), this, SIGNAL(sideboardPlanChanged(QObject *)));


The method signature seems incorrect:
QObject::connect: No such signal DeckView::sideboardPlanChanged(QObject *)

ctrlaltca · 2017-04-23T21:14:25Z

cockatrice/src/deckview.cpp

+            m.set_card_name(c->getName().toStdString());
+            m.set_start_zone(c->getOriginZone().toStdString());
+
+            if (c->getOriginZone() == "main")


The main problem now is that you are using getOriginZone() to determine where a card actually is and where it should move to, but the "original zone" is never updated, so if you move a card from "main" to "side" its original zone will still be "main"; double clicking it again will try to move it again from the main to the side.
You don't want to che the card "original zone", but its "current zone" instead; you should be able to get this information from the card's parentItem()

ZeldaZach · 2017-04-25T05:53:00Z

This PR is ready to be reviewed.

tooomm

I can switch cards around with double click with no issues in a local game.

No idea of the underlying things and the changes to the deck on the server...

ZeldaZach · 2017-04-25T17:39:44Z

@tooomm I fixed it to work server-side too

ctrlaltca

Code looks fine and works nicely 👍

ZeldaZach requested review from ctrlaltca and Daenyth April 20, 2017 05:34

ghost reviewed Apr 20, 2017

View reviewed changes

ZeldaZach commented Apr 20, 2017

View reviewed changes

ctrlaltca requested changes Apr 20, 2017

View reviewed changes

ZeldaZach requested a review from ctrlaltca April 20, 2017 19:55

ctrlaltca requested changes Apr 23, 2017

View reviewed changes

change cards from sb<->mb with double-click

8236281

ZeldaZach changed the title ~~[WIP] change cards from sb<->mb with double-click~~ Change cards from sb<->mb with double-click Apr 25, 2017

ZeldaZach requested review from ctrlaltca and tooomm April 25, 2017 05:53

tooomm reviewed Apr 25, 2017

View reviewed changes

ctrlaltca approved these changes Apr 25, 2017

View reviewed changes

ZeldaZach merged commit 62d8f5a into Cockatrice:master Apr 25, 2017

ZeldaZach deleted the click_to_move branch April 25, 2017 18:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Change cards from sb<->mb with double-click #2606

Change cards from sb<->mb with double-click #2606

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -274,13 +275,26 @@ void DeckViewContainer::sideboardLockButtonClicked()

		void DeckViewContainer::sideboardPlanChanged()

Uh oh!

Change cards from sb<->mb with double-click #2606

Change cards from sb<->mb with double-click #2606

Uh oh!

Conversation

Uh oh!

Related Ticket(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!