D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh added a dependent revision: D10119: Create test to assert 
metaDataRequestFinished is emitted once only.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh retitled this revision from "Emit metaDataRequestFinished once per 
request" to "baloo-widgets: Emit metaDataRequestFinished once per request".

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Dominik Haumann]

dhaumann added inline comments.

INLINE COMMENTS

> filemetadataprovider.h:114
>  void loadingFinished();
> +void dataAvailable();
>  

Could you add API documentation there what this signal indicates and when to 
use it? Typically, if you find code that is documented it is good practice to 
do the same :-)

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26071.
michaelh added a comment.


  Update signal descriptions

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=25962&id=26071

BRANCH
  dataavail

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh marked an inline comment as done.
michaelh added inline comments.

INLINE COMMENTS

> filemetadataprovider.cpp:323
> +emit dataAvailable();
>  
>  } else {

Is it guaranteed, that this is emitted before IndexedDataRetriever job is 
finished?

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Elvis Angelaccio]

elvisangelaccio added a comment.


  +1, looks good to me.

INLINE COMMENTS

> michaelh wrote in filemetadataprovider.cpp:323
> Is it guaranteed, that this is emitted before IndexedDataRetriever job is 
> finished?

I don't think so, because `IndexedDataRetriever::start()` does not block.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> elvisangelaccio wrote in filemetadataprovider.cpp:323
> I don't think so, because `IndexedDataRetriever::start()` does not block.

In that case it might be safer to do

 IndexedDataRetriever *ret = new IndexedDataRetriever(filePath, this);
​connect(ret, SIGNAL(finished(KJob*)), this, 
SLOT(slotLoadingFinished(KJob*)));   
 
  insertBasicData();
​insertEditableData();
​emit dataAvailable();
  
  ​ret->start();

Because once loadingFinished() is signalled it's over. dataAvailable(); won't 
get processed anymore.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26117.
michaelh added a comment.


  Apply suggested changes

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26071&id=26117

BRANCH
  unittest

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filemetadatawidgettest.cpp
  autotests/filemetadatawidgettest.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26120.
michaelh added a comment.


  Revert incorrect diff update

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26117&id=26120

BRANCH
  dataavail

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26129.
michaelh added a comment.


  Add Q_ASSERT

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26120&id=26129

BRANCH
  dataavail

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

AFFECTED FILES
  src/filemetadataprovider.cpp

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26131.

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26129&id=26131

BRANCH
  dataavail

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26135.
michaelh added a comment.


  Correct yet another update error

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26131&id=26135

BRANCH
  dataavail

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26149.
michaelh added a comment.


  Change execution order in setFileItem

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26135&id=26149

BRANCH
  dataavail

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh planned changes to this revision.
michaelh added a comment.


  Needs rebasing

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26183.
michaelh added a comment.


  Merge branch 'master' of git://anongit.kde.org/baloo-widgets
  Change execution order in setFileItems

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26149&id=26183

BRANCH
  signalavailable (branched from master)

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Elvis Angelaccio]

elvisangelaccio added inline comments.

INLINE COMMENTS

> michaelh wrote in filemetadataprovider.cpp:323
> In that case it might be safer to do
> 
>  IndexedDataRetriever *ret = new IndexedDataRetriever(filePath, this);
>   ​connect(ret, SIGNAL(finished(KJob*)), this, 
> SLOT(slotLoadingFinished(KJob*)));   
>  
>   insertBasicData();
>   ​insertEditableData();
>   ​emit dataAvailable();
>   
>   ​ret->start();
> 
> Because once loadingFinished() is signalled it's over. dataAvailable(); won't 
> get processed anymore.

Sounds good, but please explain why we are moving this signal in the commit 
message. This will make easier to `git blame` this code in the future.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh edited the summary of this revision.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh added a comment.


  Changed description
  Or did you mean the commit messages of 1e3aa22d15d8 and 0f362a3e0de0?
  Also, I've committed the accepted revisions to master not Applications/17.1. 
Correct?

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26218.
michaelh added a comment.


  Added test for empty file list
  Added test for file list with empty url
  Simplified path construction

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26183&id=26218

BRANCH
  arcpatch-D10119 (branched from master)

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

AFFECTED FILES
  autotests/CMakeLists.txt
  autotests/filemetadatawidgettest.cpp
  autotests/filemetadatawidgettest.h
  autotests/testtagged.m4a
  autotests/testtagged.mp3

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh added a comment.


  Both test for empty failed because the spy timed out!
  IMO they should not, because consumers rely on the `metaDataRequestFinished` 
signal.
  Please tell me if you disagree.
  
  I don't know why they fail, yet. Because line 371 is reached in 
src/filemetadataprovider.cpp 
.
  
370if (items.isEmpty()) {
371 ​emit loadingFinished();

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh updated this revision to Diff 26222.
michaelh added a comment.


  Change commit message

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26218&id=26222

BRANCH
  signalavailable (branched from master)

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> filemetadatawidget.cpp:119
> +slotDataAvailable(); 
> +emit q->metaDataRequestFinished(m_provider->items());
> +}

test with empty `items()`: This code is reached, but the `moc` doesn't signal.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

michaelh marked 3 inline comments as done.

REPOSITORY
  R824 Baloo Widgets

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Elvis Angelaccio]

elvisangelaccio accepted this revision as: elvisangelaccio.
This revision is now accepted and ready to land.

REPOSITORY
  R824 Baloo Widgets

BRANCH
  signalavailable (branched from master)

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Nathaniel Graham]

ngraham accepted this revision.

REPOSITORY
  R824 Baloo Widgets

BRANCH
  signalavailable (branched from master)

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

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann

D10113: baloo-widgets: Emit metaDataRequestFinished once per request

[Michael Heidelbach]

This revision was automatically updated to reflect the committed changes.
Closed by commit R824:5e4203cd323a: baloo-widgets: Emit metaDataRequestFinished 
once per request (authored by michaelh).

REPOSITORY
  R824 Baloo Widgets

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10113?vs=26222&id=26745

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

AFFECTED FILES
  src/filemetadataprovider.cpp
  src/filemetadataprovider.h
  src/filemetadatawidget.cpp
  src/filemetadatawidget.h

To: michaelh, elvisangelaccio, smithjd, vhanda, ngraham, #dolphin, #frameworks
Cc: dhaumann