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

List:       kde-core-devel
Subject:    Re: Comparing KFileItems
From:       Frank Reininghaus <frank78ac () googlemail ! com>
Date:       2012-10-24 8:47:46
Message-ID: CAFoZWWg2j6gSGEe8-DKNC3OsdN-V2dYrdurv0Lqk0xc9o4tQuQ () mail ! gmail ! com
[Download RAW message or body]

Hi David,

I see that I should probably have created a review request to make
review easier for you - sorry about that! But I think we're getting
closer to the final solution, so I'll just reply to your message with
a new patch.

2012/10/23 David Faure:
> On Monday 22 October 2012 20:10:23 Frank Reininghaus wrote:

>>  bool KFileItem::operator!=(const KFileItem& other) const
>>  {
>> -    return d != other.d;
>> +    return d != other.d && d->m_url != other.d->m_url;
>>  }
>
> Or return !operator==(other), inline, for easier maintainance.

I haven't implemented the inline part yet. Would you prefer to have
the inline method inside the class definition or rather just move the
function into the header file and prepend it with 'inline'? Or just
leave it as it is right now in my patch?

>
>>  #ifndef KDE_NO_DEPRECATED
>> diff --git a/kio/tests/kfileitemtest.cpp b/kio/tests/kfileitemtest.cpp
>> index fa88af7..1ac5d98 100644
>> --- a/kio/tests/kfileitemtest.cpp
>> +++ b/kio/tests/kfileitemtest.cpp
>> @@ -108,11 +108,12 @@ void KFileItemTest::testDetach()
>>      fileItem2.mark();
>>      QVERIFY(fileItem2.isMarked());
>>      QVERIFY(!fileItem.isMarked());
>> -    QVERIFY(fileItem != fileItem2);
>> +    QVERIFY(fileItem == fileItem2);
>
> The point of that test was to ensure that detaching did happen.
> Maybe add "friend class KFileItemTest;" and compare the value of the d
> pointers?
> In addition to QVERIFY(fileItem == fileItem2), of course.

Done.

>>      fileItem = fileItem2;
>>      QVERIFY(fileItem2.isMarked());
>>      QVERIFY(fileItem == fileItem2);
>> +    QVERIFY(!(fileItem != fileItem2));
>>  }
>>
>>  void KFileItemTest::testBasic()
>> @@ -246,7 +247,8 @@ void KFileItemTest::testCmp()
>>
>>      KFileItem fileItem(KFileItem::Unknown, KFileItem::Unknown,
>> KUrl(file.fileName()), true /*on demand*/); KFileItem
>> fileItem2(KFileItem::Unknown, KFileItem::Unknown, KUrl(file.fileName()),
>> false); -    QVERIFY(fileItem != fileItem2); // created independently so
>> not 'equal' +    QVERIFY(fileItem == fileItem2); // created independently,
>> but still 'equal' +    QVERIFY(!(fileItem != fileItem2));
>>      QVERIFY(fileItem.cmp(fileItem2));
>
> Thank you kmail for messing up the lines in the reply...
> The change looks good though.
>
> Could you add a bit of docu while at it, about the difference between == and
> cmp as discussed here? Thanks.

Done.

Best regards,
Frank

["KFileItem-version2.diff" (application/octet-stream)]

diff --git a/kio/kio/kfileitem.cpp b/kio/kio/kfileitem.cpp
index 482c4b6..54520d5 100644
--- a/kio/kio/kfileitem.cpp
+++ b/kio/kio/kfileitem.cpp
@@ -1307,13 +1307,12 @@ bool KFileItem::cmp( const KFileItem & item ) const
 
 bool KFileItem::operator==(const KFileItem& other) const
 {
-    // is this enough?
-    return d == other.d;
+    return d == other.d || d->m_url == other.d->m_url;
 }
 
 bool KFileItem::operator!=(const KFileItem& other) const
 {
-    return d != other.d;
+    return !operator==(other);
 }
 
 #ifndef KDE_NO_DEPRECATED
diff --git a/kio/kio/kfileitem.h b/kio/kio/kfileitem.h
index fe7ea29..9177a45 100644
--- a/kio/kio/kfileitem.h
+++ b/kio/kio/kfileitem.h
@@ -491,15 +491,21 @@ public:
     /**
      * Somewhat like a comparison operator, but more explicit,
      * and it can detect that two kfileitems are equal even when they do
-     * not share the same internal pointer - e.g. when KDirLister compares
+     * not share the same URL - e.g. when KDirLister compares
      * fileitems after listing a directory again, to detect changes.
      * @param item the item to compare
      * @return true if all values are equal
      */
     bool cmp( const KFileItem & item ) const;
 
+    /**
+     * Returns true if both items share the same URL.
+     */
     bool operator==(const KFileItem& other) const;
 
+    /**
+     * Returns true if both items do not share the same URL.
+     */
     bool operator!=(const KFileItem& other) const;
 
 
@@ -646,6 +652,8 @@ private:
 private:
     KIO_EXPORT friend QDataStream & operator<< ( QDataStream & s, const KFileItem & \
                a );
     KIO_EXPORT friend QDataStream & operator>> ( QDataStream & s, KFileItem & a );
+
+    friend class KFileItemTest;
 };
 
 Q_DECLARE_METATYPE(KFileItem)
diff --git a/kio/tests/kfileitemtest.cpp b/kio/tests/kfileitemtest.cpp
index fa88af7..13d8d3f 100644
--- a/kio/tests/kfileitemtest.cpp
+++ b/kio/tests/kfileitemtest.cpp
@@ -105,14 +105,18 @@ void KFileItemTest::testDetach()
     QCOMPARE(fileItem2.extraData(this), (const void*)this);
 #endif
     QVERIFY(fileItem == fileItem2);
+    QVERIFY(fileItem.d == fileItem2.d);
     fileItem2.mark();
     QVERIFY(fileItem2.isMarked());
     QVERIFY(!fileItem.isMarked());
-    QVERIFY(fileItem != fileItem2);
+    QVERIFY(fileItem == fileItem2);
+    QVERIFY(fileItem.d != fileItem2.d);
 
     fileItem = fileItem2;
     QVERIFY(fileItem2.isMarked());
     QVERIFY(fileItem == fileItem2);
+    QVERIFY(fileItem.d == fileItem2.d);
+    QVERIFY(!(fileItem != fileItem2));
 }
 
 void KFileItemTest::testBasic()
@@ -246,7 +250,8 @@ void KFileItemTest::testCmp()
 
     KFileItem fileItem(KFileItem::Unknown, KFileItem::Unknown, \
                KUrl(file.fileName()), true /*on demand*/);
     KFileItem fileItem2(KFileItem::Unknown, KFileItem::Unknown, \
                KUrl(file.fileName()), false);
-    QVERIFY(fileItem != fileItem2); // created independently so not 'equal'
+    QVERIFY(fileItem == fileItem2); // created independently, but still 'equal'
+    QVERIFY(!(fileItem != fileItem2));
     QVERIFY(fileItem.cmp(fileItem2));
 }
 



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