BookmarksRunner: Avoid multiple connections of identical signal
ClosedPublic

Authored by bruns on Sep 6 2018, 2:03 AM.

Details

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

Diff Detail

Repository
R120 Plasma Workspace
Branch
T9626
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2562
Build 2580: arc lint + arc unit
bruns created this revision.Sep 6 2018, 2:03 AM
Restricted Application added a project: Plasma. · View Herald TranscriptSep 6 2018, 2:03 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
bruns requested review of this revision.Sep 6 2018, 2:03 AM
broulik added a subscriber: broulik.Sep 6 2018, 6:42 AM

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

runners/bookmarks/bookmarksrunner.cpp
66

Please provide this as context.

davidedmundson added inline comments.
runners/bookmarks/bookmarksrunner.cpp
65

do we not also need a disconnect on m_browser?

bruns added inline comments.Sep 6 2018, 1:18 PM
runners/bookmarks/bookmarksrunner.cpp
65

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

66

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

broulik added inline comments.Sep 6 2018, 1:19 PM
runners/bookmarks/bookmarksrunner.cpp
66

No, four-argument connect:

connect(this, &..., this, ....

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

bruns planned changes to this revision.Sep 6 2018, 1:36 PM
bruns added inline comments.
runners/bookmarks/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.

broulik added inline comments.Sep 6 2018, 1:37 PM
runners/bookmarks/bookmarksrunner.cpp
66

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

connect(this, &Plasma::AbstractRunner:teardown, m_browser, &Browser::teardown);
bruns added inline comments.Sep 6 2018, 1:39 PM
runners/bookmarks/bookmarksrunner.cpp
66

yes, after readding the dynamic_cast<QObject*>, m_browser is a non-QObject interface class.

bruns added inline comments.Sep 6 2018, 10:37 PM
runners/bookmarks/bookmarksrunner.cpp
66

The only syntax which works is

connect(this, &Plasma::AbstractRunner:teardown,
        dynamic_cast<QObject*>(m_browser), [this] () { m_browser->teardown(); });

The context object has to be a QObject* (or derived).
&Browser::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.

bruns updated this revision to Diff 41125.Sep 6 2018, 11:20 PM

add context object to disconnect old instance automatically

bruns marked 8 inline comments as done.Sep 7 2018, 12:20 PM
broulik accepted this revision.Sep 7 2018, 12:23 PM
This revision is now accepted and ready to land.Sep 7 2018, 12:23 PM
This revision was automatically updated to reflect the committed changes.