[Differential] [Commented On] D1784: Ensure that WaylandServer::shellClientAdded only gets emitted once

[sebas (Sebastian Kügler)]

sebas added a comment.


  a few questions and suggestions, feedback welcome...

INLINE COMMENTS

> shell_client_test.cpp:129
> +{
> +#define CLEANUP(name) \
> +    if (name) { \

aren't you going overboard here? I don't think this makes the backtrace a lot easier to read...

> shell_client_test.cpp:180
> +    surface->attachBuffer(Buffer::Ptr());
> +    surface->commit(Surface::CommitFlag::None);
> +    QVERIFY(hiddenSpy.wait());

I wonder if this should be the default arg in the API. What do you think?

> shell_client_test.cpp:190
> +    QCOMPARE(clientAddedSpy.count(), 1);
> +    QVERIFY(windowShownSpy.wait());
> +    QCOMPARE(clientAddedSpy.count(), 1);

Also check windowShownSpy.count()?

> shell_client_test.cpp:202
> +    surface.reset();
> +    QVERIFY(windowClosedSpy.wait());
> +}

Check count here?

> wayland_server.cpp:251
> +    ShellClient *c = dynamic_cast<ShellClient*>(t);
> +    if (!c) {
> +        return;

worth warning here, or is this a valid case? (Looks like an application bug if this return is hit)

REPOSITORY
  rKWIN KWin

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

EMAIL PREFERENCES
  https://phabricator.kde.org/settings/panel/emailpreferences/

To: graesslin, #kwin, #plasma_on_wayland
Cc: sebas, plasma-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20160607/fa5b2243/attachment.html>