[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-games-devel
Subject:    [Kde-games-devel] [PATCH] Single Pile Highlighting in KPat
From:       "Parker Coates" <parker.coates () gmail ! com>
Date:       2008-12-02 6:21:47
Message-ID: 85d347350812012221n45971882oc460d2761c14bf6b () mail ! gmail ! com
[Download RAW message or body]

Hello list,

The attached patch for KPat "fixes" a behaviour that has mildly
annoyed me for some time.

Currently when dragging a card, KPat checks all piles currently under
the card and highlights all those that are valid drop points for the
card being moved. In certain situations in certain games, this can
lead to up to four piles being highlighted at once. The problem is
that from any given position, the card can only actually be dropped on
a single pile. Highlighting more than one pile, when in reality only
one is the "real" drop target is a little misleading and (in my
opinion) looks bad.

My patch essentially extracts the pile selection code from the drop
function into its own method. This method is then called by both the
hover logic (for selecting which pile to highlight) and the drop
function (for selecting the pile to drop on). The result is less code
and more consistency.

So I come to you with the following questions. Do we agree that
highlighting a single pile is preferred to highlighting multiple
piles? Does my patch look alright? This is my first time touching the
KPat code base, so I wouldn't be surprised if I missed something.

Thanks,

Parker

["KPatSingleHighlight.diff" (text/x-patch)]

Index: dealer.h
===================================================================
--- dealer.h	(revision 890191)
+++ dealer.h	(working copy)
@@ -75,8 +75,10 @@
 
     DealerScene();
     ~DealerScene();
+
     void unmarkAll();
     void mark(Card *c);
+    Pile * targetPile();
 
 protected:
     virtual void mouseDoubleClickEvent ( QGraphicsSceneMouseEvent * mouseEvent );
Index: dealer.cpp
===================================================================
--- dealer.cpp	(revision 890191)
+++ dealer.cpp	(working copy)
@@ -664,51 +664,18 @@
         (*it)->setPos( ( *it )->pos() + e->scenePos() - moving_start );
     }
 
-    PileList sources;
-    QList<QGraphicsItem *> list = collidingItems( movingCards.first() );
-
-    // kDebug() << "movingCards" << movingCards.first()->sceneBoundingRect();
-    for (QList<QGraphicsItem *>::Iterator it = list.begin(); it != list.end(); ++it)
-    {
-        //kDebug() << "it" << ( *it )->type() << " " << ( *it )->sceneBoundingRect();
-        if ((*it)->type() == QGraphicsItem::UserType + DealerScene::CardTypeId) {
-            Card *c = dynamic_cast<Card*>(*it);
-            assert(c);
-            if (!c->isFaceUp())
-                continue;
-            if (c->source() == movingCards.first()->source())
-                continue;
-            if (sources.indexOf(c->source()) != -1)
-                continue;
-            sources.append(c->source());
-        } else {
-            if ((*it)->type() == QGraphicsItem::UserType + DealerScene::PileTypeId ) {
-                Pile *p = static_cast<Pile*>(*it);
-                if (p->isEmpty() && !sources.contains(p))
-                    sources.append(p);
-            } else {
-                kDebug(11111) << "unknown object" << *it << " " << (*it)->type();
-            }
-        }
-    }
-
     // TODO some caching of the results
     unmarkAll();
 
-    for (PileList::Iterator it = sources.begin(); it != sources.end(); ++it)
-    {
-        bool b = (*it)->legalAdd(movingCards);
-        // kDebug() << "legalAdd" << b << " " << ( *it )->x();
-        if (b) {
-            if ((*it)->isEmpty()) {
-                (*it)->setHighlighted(true);
-                marked.append(*it);
-            } else {
-                mark((*it)->top());
-            }
+    Pile * dropPile = targetPile();
+    if (dropPile) {
+        if (dropPile->isEmpty()) {
+            dropPile->setHighlighted(true);
+            marked.append(dropPile);
+        } else {
+            mark(dropPile->top());
         }
     }
-
     moving_start = e->scenePos();
 }
 
@@ -927,13 +894,25 @@
         }
     }
 
-    if (!movingCards.count())
+    if (movingCards.isEmpty())
         return;
-    Card *c = static_cast<Card*>(movingCards.first());
-    assert(c);
 
     unmarkAll();
 
+    Pile * destination = targetPile();
+    if (destination) {
+        Card *c = static_cast<Card*>(movingCards.first());
+        assert(c);
+        countGame();
+        c->source()->moveCards(movingCards, destination);
+        takeState();
+        eraseRedo();
+    }
+    movingCards.clear();
+}
+
+Pile * DealerScene::targetPile()
+{
     QList<QGraphicsItem *> list = collidingItems( movingCards.first() );
     HitList sources;
 
@@ -993,9 +972,7 @@
             ++it;
     }
 
-    if (sources.isEmpty()) {
-        c->source()->moveCardsBack(movingCards);
-    } else {
+    if (!sources.isEmpty()) {
         HitList::Iterator best = sources.begin();
         HitList::Iterator it = best;
         for (++it; it != sources.end(); ++it )
@@ -1007,12 +984,10 @@
                 best = it;
             }
         }
-        countGame();
-        c->source()->moveCards(movingCards, (*best).source);
-        takeState();
-        eraseRedo();
+        return (*best).source;
+    } else {
+        return 0;
     }
-    movingCards.clear();
 }
 
 void DealerScene::mouseDoubleClickEvent( QGraphicsSceneMouseEvent *e )


_______________________________________________
kde-games-devel mailing list
kde-games-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-games-devel


[prev in list] [next in list] [prev in thread] [next in thread]