D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

This revision was automatically updated to reflect the committed changes.
Closed by commit R241:f893784ea4f1: Add == and != operators to KIO::UDSEntry 
(authored by meven).

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=66086=66883

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h
  tests/udsentrybenchmark.cpp

To: meven, #frameworks, ngraham, dfaure
Cc: dfaure, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23667: Add == and != operators to KIO::UDSEntry

[David Faure]

dfaure added a comment.


  Note that there is `KFileItem::cmp` for a more thorough comparison of two 
fileitems. But it only looks at some UDS fields -- those that matter at the 
KDirLister level.
  Or maybe it shows that UDSEntry::operator== was missing so KFileItem::cmp 
uses a poor man's version of that ;)
  This confirms the need for such an operator== indeed (though I wouldn't 
change the implementation of KFileItem::cmp until we see a need to do that).
  
  Thanks for the explanation.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks, ngraham, dfaure
Cc: dfaure, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven added a comment.


  In D23667#535222 , @dfaure wrote:
  
  > Looks ok, but I'm just curious about the use case. "I felt it was missing" 
doesn't sound as strong an argument as "I need this"...
  >
  > Slaves are supposed to mostly create those, not compare them, and apps are 
supposed to use KFileItem rather than UDSEntry directly.
  
  
  The use case I felt I might need it, is when running stats for a file which I 
already stated earlier and have a KFileItem already.
  And checking if the file changed between the two stats.
  Without this it would be hard to do.
  
  Using the entry to compare the KFileItem seemed to me the simplest course of 
action (KFileItem equals compares only file path) but lacked the equal 
operators.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks, ngraham, dfaure
Cc: dfaure, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23667: Add == and != operators to KIO::UDSEntry

[David Faure]

dfaure accepted this revision.
dfaure added a comment.


  Looks ok, but I'm just curious about the use case. "I felt it was missing" 
doesn't sound as strong an argument as "I need this"...
  
  Slaves are supposed to mostly create those, not compare them, and apps are 
supposed to use KFileItem rather than UDSEntry directly.

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks, ngraham, dfaure
Cc: dfaure, pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, 
bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven added a comment.


  @pino if this is fine for you...

REPOSITORY
  R241 KIO

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 66086.
meven added a comment.


  Update @since references

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65705=66086

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h
  tests/udsentrybenchmark.cpp

To: meven, #frameworks, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 65705.
meven added a comment.


  Remove unneeded friend function declarations

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65608=65705

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h
  tests/udsentrybenchmark.cpp

To: meven, #frameworks, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 65608.
meven marked 5 inline comments as done.
meven added a comment.


  typo fix

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65607=65608

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h
  tests/udsentrybenchmark.cpp

To: meven, #frameworks, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 65607.
meven added a comment.


  Fix test and implementation

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65298=65607

BRANCH
  arcpatch-D23667

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h
  tests/udsentrybenchmark.cpp

To: meven, #frameworks, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Pino Toscano]

pino added a comment.


  please use `QCOMPARE`/`QVERIFY` instead of `Q_ASSERT` in QTest tests

INLINE COMMENTS

> udsentrytest.cpp:301
> +
> +// 4th entry : an additionnal field
> +KIO::UDSEntry entry4;

typo, "additional"

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Nathaniel Graham]

ngraham accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks, ngraham
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 65298.
meven added a comment.


  fix since mention

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65297=65298

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h
  tests/udsentrybenchmark.cpp

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven edited the test plan for this revision.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 65297.
meven added a comment.


  Fix test udsentrybenchmark

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65296=65297

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h
  tests/udsentrybenchmark.cpp

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 65296.
meven added a comment.


  Clean spaces changes

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65295=65296

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven updated this revision to Diff 65295.
meven added a comment.


  Move impl to cpp file, fix comment, add unit test

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D23667?vs=65218=65295

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  autotests/udsentrytest.cpp
  autotests/udsentrytest.h
  src/core/udsentry.cpp
  src/core/udsentry.h

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Pino Toscano]

pino added a comment.


  Oh, and also please add tests for them in UDSEntryTest.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Pino Toscano]

pino added a comment.


  Please move the implementations in the cpp file, otherwise it will be 
impossible to change/fix the implementation later on in a binary compatible way.
  
  Also:
  
  - both the operators ought to be const, since they do not mutate the object 
(and otherwise they cannot be used to compare const objects)
  - please fix the style in the declarations: `const UDSEntry& other` -> `const 
UDSEntry `

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks
Cc: pino, ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Nathaniel Graham]

ngraham added a comment.


  I know it seems silly for such tiny functions, but can you follow the pattern 
and keep only the function definitions in the header file and move the actual 
logic into the .cpp file?

INLINE COMMENTS

> udsentry.h:118
> +/**
> + * Returns true if the entry contains the same data as other
> + * @since 5.62.0

as other -> as the other

> udsentry.h:119
> + * Returns true if the entry contains the same data as other
> + * @since 5.62.0
> + */

Just `5.62` (Frameworks don't have minor versions)

> udsentry.h:127
> +/**
> + * Returns true if the entry does not contain the same data as other
> + * @since 5.62.0

ditto

> udsentry.h:128
> + * Returns true if the entry does not contain the same data as other
> + * @since 5.62.0
> + */

ditto

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D23667

To: meven, #frameworks
Cc: ngraham, kde-frameworks-devel, LeGast00n, GB_2, michaelh, bruns

D23667: Add == and != operators to KIO::UDSEntry

[Méven Car]

meven created this revision.
meven added a reviewer: Frameworks.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
meven requested review of this revision.

REVISION SUMMARY
  I felt it was missing.

TEST PLAN
  Compiles

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D23667

AFFECTED FILES
  src/core/udsentry.cpp
  src/core/udsentry.h

To: meven, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns