Code review comment for lp:~gerboland/qtmir/multimonitor-spike

Revision history for this message
Daniel d'Andrada (dandrader) wrote :

Good clean up! Only a couple of minor issues left:

----------------------

Please update copyright year of src/platforms/mirserver/miropenglcontext.cpp

---------------------

src/platforms/mirserver/screencontroller.cpp

It still has some qDebug() messages and commented-out code.

"""
        if (window && window->window()) { qDebug() << "HIDE" << window;
"""

"""
            //window->setVisible(false);
"""

NB: the stuff above appears in two separate locations in the file

----------------------------------

In tests/mirserver/ScreenController/screencontroller_test.cpp

"""
    ASSERT_EQ(screenController->screens().count(), 1);
"""

Google test format for assertions is as follows: ASSERT_EQ(expected, actual)

But in this file you're doing the other way around, which will make for confusing messages in case of failure.

Same goes for EXPECT_EQ() and friends.

review: Needs Fixing

« Back to merge proposal