D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> dfaure wrote in monitorcommand.cpp:53
> Hmm, why not just use QDbusServiceWatcher's signals, to connect to 
> serviceRegistered, if the service isn't available when we start?

The idea to do it that way came to late. :-) 
I'll make an update soon.

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure, alexeymin
Cc: cfeck, alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[David Faure]

dfaure added inline comments.

INLINE COMMENTS

> monitorcommand.cpp:53
> +while (!m_dbusInterface->isValid()) {
> +QThread::msleep(50);
> +m_dbusInterface->disconnect();

Hmm, why not just use QDbusServiceWatcher's signals, to connect to 
serviceRegistered, if the service isn't available when we start?

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure, alexeymin
Cc: cfeck, alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

This revision was automatically updated to reflect the committed changes.
Closed by commit R293:94da1850ec6c: balooctl monitor: Wait for dbus interface 
(authored by michaelh).

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10578?vs=27416=27417

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

AFFECTED FILES
  src/tools/balooctl/monitorcommand.cpp
  src/tools/balooctl/monitorcommand.h

To: michaelh, dfaure, alexeymin
Cc: cfeck, alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh marked 11 inline comments as done.

REPOSITORY
  R293 Baloo

BRANCH
  wait-for-dbus (branched from master)

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

To: michaelh, dfaure, alexeymin
Cc: cfeck, alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

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


  - balooctl monitor: Adhere to KDE coding-style

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10578?vs=27394=27416

BRANCH
  wait-for-dbus (branched from master)

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

AFFECTED FILES
  src/tools/balooctl/monitorcommand.cpp
  src/tools/balooctl/monitorcommand.h

To: michaelh, dfaure, alexeymin
Cc: cfeck, alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Christoph Feck]

cfeck added inline comments.

INLINE COMMENTS

> michaelh wrote in monitorcommand.cpp:45
> You're right. I just looked at the example in Qt's coding style rules. In 
> fact it is operators BOL, commas EOL.
> I'm ok with putting the commas at EOL (it's the rule, after all). Since to me 
> it is more readable this way I'd like to wait for a second opinion or your 
> objection.

I have only seen commas at the beginning of a line for constructor 
initializers, because adding a new one would affect two lines, instead of 
inserting a single new line. It just looks better in diffs.

Otherwise, yes, commas at the end, please.

REPOSITORY
  R293 Baloo

BRANCH
  wait-for-dbus (branched from master)

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

To: michaelh, dfaure, alexeymin
Cc: cfeck, alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, 
nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh added a comment.


  In D10578#208132 , @alexeymin 
wrote:
  
  > There is also https://techbase.kde.org/Policies/Frameworks_Coding_Style.
  
  
  ... and it refers to Qt.
  
  > I've never seen commas at the start of the line.
  
  It's part of the npm coding-style. I found it strange too at first. Probably 
it makes more sense in javascript. Again i don't insist on (... thanks for your 
accepting this diff ...) it.

REPOSITORY
  R293 Baloo

BRANCH
  wait-for-dbus (branched from master)

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

To: michaelh, dfaure, alexeymin
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Alexey Min]

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

REPOSITORY
  R293 Baloo

BRANCH
  wait-for-dbus (branched from master)

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

To: michaelh, dfaure, alexeymin
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Alexey Min]

alexeymin added a comment.


  There is also https://techbase.kde.org/Policies/Frameworks_Coding_Style. 
Guess I was wrong about `#include`s order, but honestly I've never seen commas 
at the start of the line.

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh added inline comments.

INLINE COMMENTS

> alexeymin wrote in monitorcommand.cpp:45
> Strange formatting of commas in above 3 lines. It looks OK in constructor, 
> where member initialization may be added later, but here, when the parameter 
> count is fixed, no need to start new line with a comma. I'd suggest to keep 
> as it was before

You're right. I just looked at the example in Qt's coding style rules. In fact 
it is operators BOL, commas EOL.
I'm ok with putting the commas at EOL (it's the rule, after all). Since to me 
it is more readable this way I'd like to wait for a second opinion or your 
objection.

> alexeymin wrote in monitorcommand.cpp:54
> `delete m_dbusInterface;` after this line, because below you are creating a 
> new object? Each every 50 ms? Memory leaks here

Yeah, massively. Thanks for pointing that out.

> alexeymin wrote in monitorcommand.h:28
> Why dod you move local includes up? Usually they are below globals

Not in baloo's code it seems. So it's for consistency.

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh updated this revision to Diff 27394.
michaelh marked an inline comment as done.
michaelh added a comment.


  - balooctl monitor: Fix memory leak
  - balooctl monitor: Whitespace cleanup

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10578?vs=27360=27394

BRANCH
  wait-for-dbus (branched from master)

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

AFFECTED FILES
  src/tools/balooctl/monitorcommand.cpp
  src/tools/balooctl/monitorcommand.h

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Alexey Min]

alexeymin added a comment.


  Looks almost fine, just fix these:

INLINE COMMENTS

> monitorcommand.cpp:45
> +, QDBusConnection::sessionBus()
> +, this
> +);

Strange formatting of commas in above 3 lines. It looks OK in constructor, 
where member initialization may be added later, but here, when the parameter 
count is fixed, no need to start new line with a comma. I'd suggest to keep as 
it was before

> monitorcommand.cpp:54
> +QThread::msleep(50);
> +m_dbusInterface->disconnect();
> +m_dbusInterface = new 
> org::kde::baloo::fileindexer(QStringLiteral("org.kde.baloo")

`delete m_dbusInterface;` after this line, because below you are creating a new 
object? Each every 50 ms? Memory leaks here

> monitorcommand.cpp:58
> +, QDBusConnection::sessionBus()
> +, this
> +);

Again, strange formatting of commas

> monitorcommand.cpp:65
> +connect(m_dbusInterface, 
> ::kde::baloo::fileindexer::startedIndexingFile
> +, this, ::startedIndexingFile);
> +connect(m_dbusInterface, 
> ::kde::baloo::fileindexer::finishedIndexingFile

Strange formatting of commas

> monitorcommand.cpp:67
> +connect(m_dbusInterface, 
> ::kde::baloo::fileindexer::finishedIndexingFile
> +, this, ::finishedIndexingFile);
> +

Strange formatting of commas

> monitorcommand.cpp:72
> +connect(balooWatcher, ::serviceUnregistered
> +, [this, diedMessage]() {
> +m_out << diedMessage << endl;

Strange formatting of commas

> monitorcommand.h:28
> +#include "command.h"
> +#include "fileindexerinterface.h"
>  #include 

Why dod you move local includes up? Usually they are below globals

> monitorcommand.h:60
>  QString m_currentFile;
> +
>  };

extra empty string?

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh added a comment.


  In D10578#207745 , @alexeymin 
wrote:
  
  > Can you add simple instructions how to reproduce full testing scenario?
  
  
  See test plan

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh updated this revision to Diff 27360.
michaelh marked an inline comment as done.
michaelh edited the summary of this revision.
michaelh edited the test plan for this revision.
michaelh added a comment.


  - balooctl monitor: Use QCoreApplication::exit()
  - balooctl monitor: Explicit lamba captures

REPOSITORY
  R293 Baloo

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D10578?vs=27334=27360

BRANCH
  wait-for-dbus (branched from master)

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

AFFECTED FILES
  src/tools/balooctl/monitorcommand.cpp
  src/tools/balooctl/monitorcommand.h

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh edited the test plan for this revision.

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Alexey Min]

alexeymin added a comment.


  Can you add simple instructions how to reproduce full testing scenario?

INLINE COMMENTS

> monitorcommand.cpp:75
> +//TODO: Wait again for dbus interface
> +QCoreApplication::instance()->exit(0);
> +});

No need to call `instance()` here; `QCoreApplication::exit()` is a static 
function and does not need object instance.

REPOSITORY
  R293 Baloo

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

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella

D10578: balooctl monitor: Wait for dbus interface

[Michael Heidelbach]

michaelh created this revision.
michaelh added a reviewer: dfaure.
michaelh added a project: Baloo.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
michaelh requested review of this revision.

REVISION SUMMARY
  If baloo is not running, monitor will wait for its start.
  This will  simplify baloo diagnostics.

TEST PLAN
  run on command line

REPOSITORY
  R293 Baloo

BRANCH
  wait-for-dbus (branched from master)

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

AFFECTED FILES
  src/tools/balooctl/monitorcommand.cpp
  src/tools/balooctl/monitorcommand.h

To: michaelh, dfaure
Cc: alexeymin, #frameworks, ashaposhnikov, michaelh, spoorun, nicolasfella