D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Stefan Brüns]

This revision was automatically updated to reflect the committed changes.
Closed by commit R120:cb6805374d12: BookmarksRunner: Avoid multiple connections 
of identical signal (authored by bruns).

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15306?vs=41125=41180

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

AFFECTED FILES
  runners/bookmarks/bookmarksrunner.cpp

To: bruns, #plasma, broulik
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Stefan Brüns]

bruns updated this revision to Diff 41125.
bruns added a comment.


  add context object to disconnect old instance automatically

REPOSITORY
  R120 Plasma Workspace

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D15306?vs=41080=41125

BRANCH
  T9626

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

AFFECTED FILES
  runners/bookmarks/bookmarksrunner.cpp

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> bruns wrote in bookmarksrunner.cpp:66
> yes, after readding the `dynamic_cast`, m_browser is a non-QObject 
> interface class.

The only syntax which works is

  connect(this, ::AbstractRunner:teardown,
  dynamic_cast(m_browser), [this] () { m_browser->teardown(); 
});

The context object has to be a QObject* (or derived).
::teardown is not a pointer to a QObject member function, thus we need 
a functor.

The other approach would be to make Browser a QObject derived class.

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> broulik wrote in bookmarksrunner.cpp:66
> Ah, yeah, right, and then you could connect direcftly:
> 
>   connect(this, ::AbstractRunner:teardown, m_browser, 
> ::teardown);

yes, after readding the `dynamic_cast`, m_browser is a non-QObject 
interface class.

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> bruns wrote in bookmarksrunner.cpp:66
> Hm, just found that auto-disconnect (see @davidedmundson s comment) only 
> works if you provide a context. The context has to be `m_browser`  here.

Ah, yeah, right, and then you could connect direcftly:

  connect(this, ::AbstractRunner:teardown, m_browser, 
::teardown);

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Stefan Brüns]

bruns planned changes to this revision.
bruns added inline comments.

INLINE COMMENTS

> broulik wrote in bookmarksrunner.cpp:66
> No, four-argument connect:
> 
>   connect(this, &..., this, 
> 
> Though you just connect from `this` anyway, so I guess this isn't needed

Hm, just found that auto-disconnect (see @davidedmundson s comment) only works 
if you provide a context. The context has to be `m_browser`  here.

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> bruns wrote in bookmarksrunner.cpp:66
> you mean `this->mbrowser->teardown()`?

No, four-argument connect:

  connect(this, &..., this, 

Though you just connect from `this` anyway, so I guess this isn't needed

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> davidedmundson wrote in bookmarksrunner.cpp:65
> do we not also need a disconnect on m_browser?

Browser is just an interface class, but the subclasses also derive from QObject 
and should auto-disconnect, AFAIK.

> broulik wrote in bookmarksrunner.cpp:66
> Please provide `this` as context.

you mean `this->mbrowser->teardown()`?

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[David Edmundson]

davidedmundson added inline comments.

INLINE COMMENTS

> bookmarksrunner.cpp:65
> +if (m_browser != browser) {
> +m_browser = browser;
> +connect(this, ::AbstractRunner::teardown, [this]() { 
> m_browser->teardown(); });

do we not also need a disconnect on m_browser?

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: davidedmundson, broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, 
ali-mohamed, jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Kai Uwe Broulik]

broulik added a comment.


  Can't you connect to the slot directly and use `Qt::UniqueConnection`?

INLINE COMMENTS

> bookmarksrunner.cpp:66
> +m_browser = browser;
> +connect(this, ::AbstractRunner::teardown, [this]() { 
> m_browser->teardown(); });
> +}

Please provide `this` as context.

REPOSITORY
  R120 Plasma Workspace

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

To: bruns, #plasma
Cc: broulik, plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart

D15306: BookmarksRunner: Avoid multiple connections of identical signal

[Stefan Brüns]

bruns created this revision.
bruns added a reviewer: Plasma.
Herald added a project: Plasma.
Herald added a subscriber: plasma-devel.
bruns requested review of this revision.

REVISION SUMMARY
  The factory returns the same object when the browser name is not changed.
  Connecting the signal again leads to multiple calls to the slot each
  time the signal is emitted.
  
  See also T9626 

TEST PLAN
  1. Add some debug output to the teardown() slot
  2. Open the krunner multiple times and enter some query
  3. teardown() is called exactly once

REPOSITORY
  R120 Plasma Workspace

BRANCH
  T9626

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

AFFECTED FILES
  runners/bookmarks/bookmarksrunner.cpp

To: bruns, #plasma
Cc: plasma-devel, ragreen, Pitel, ZrenBot, lesliezhai, ali-mohamed, 
jensreuterberg, abetts, sebas, apol, mart