© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
src/modules/
"""
public Q_SLOTS:
void onScreenAdded(
void onScreenRemoved
"""
Shouldn't they be private?
-------
src/platforms/
"""
EGLDisplay m_eglDisplay;
EGLContext m_eglContext;
"""
You store them as member variables but they are still only used in the constructor.
-------
In MirServerIntegr
"""
qDebug() << "createPlatform
"""
A leftover.
"""
// If Screen was not specified, just grab an unused one, if available
"""
I don't get the first "if". This method doesn't seem to handle the case where a screen *is* specified. It doesn't even seem possible.
"""
if (!screens) {
qDebug() << "Screens are not initialized, unable to create a new QWindow/
return nullptr;
}
[...]
if (!screen) {
qDebug() << "No available Screens to create a new QWindow/
return nullptr;
}
"""
I think these should be turned into criticals (and also use the logging category API).
"""
qDebug() << "New" << window << "with geom" << window->geometry()
<< "is backed by a" << screen << "with geometry" << screen->geometry();
"""
You might want to keep this, but using the logging category API.
-------
In MirServerIntegr
"""
qDebug() << "ScreenController not initialized";
"""
More qDebug. Maybe this should be a qFatal?
-------
In src/platforms/
"""
#include <QDebug>
"""
You don't need that.
-------
In src/platforms/
"""
QWeakPointe
"""
Why return a weak pointer if all users of it do "screenControll
-------
QtEventFeeder:
I think it would be cleaner to have two constructors instead:
QtEventFeeder:
: QtEventFeeder(new QtWindowSystem(
{
}
QtEventFeeder:
{
- if (windowSystem) {
- mQtWindowSystem = windowSystem;
- } else {
- mQtWindowSystem = new QtWindowSystem;
- }
}
And remove the "= nullptr" from the signature of the second one.
-------
"""
void QtEventFeeder:
{
- auto type = mir_event_
- if (type != mir_event_
- return;
"""
Why remove it?
-------
"""
void QtEventFeeder:
{
[...]
+ mQtWindowSystem
}
"""
You have to pass the window chosen in QtEventFeeder:
-------
Please update copyright header of src/platforms/
-------
src/platforms/
I thought we dropped use of "Authors" field in copyright headers.
-------
src/platforms/
I think it would be better to have the class documentation in the header instead of in the .cpp.
"""
if (window && window->window()) { qDebug() << "SHOW" << window;
[...]
if (window && window->window()) { qDebug() << "HIDE" << window;
"""
Do you want to keep these debug messages?
"""
"""
Old code to be removed?
"""
auto displayConfig = display-
"""
I think you're abusing the use of "auto" here. I would prefer to know the class of this object.
"""
// The screen is automatically removed from Qt's internal list by the QPlatformScreen deconstructor.
auto window = static_
if (window && window->window() && window-
}
"""
s/deconstructor
Again, do you intend to keep this debug message and the commented-out code?
"""
qCDebug(
"""
Is this message useful in general?
-------
src/platforms/
"""
// local
#include "mirserver.h"
"""
Is this include really needed?
"""
virtual Screen *screenFactory(
"""
I would prefer to be straightforward and name it "createScreen".
"""
friend class MirServer;
"""
I don't like this design of making methods protected/private and adding the consumer of them as a friend class. It just obfuscates the interface this class exposes to consumers. Now the friend class also has direct access to all member variables etc and I'm sure you don't mean it to.
I vote for removing the friendship and making the relevant methods public. In the header you could group methods by consumer and document them (e.g "// Used by MirServer"). Like lightweight interfaces.
If you're really paranoid about controlling access you can go all the way and do like mir, defining an actual interface for each kind of consumer and making ScreenController implement them all.
-------
src/platforms/
"""
* Copyright (C) 2013 Canonical, Ltd.
"""
Please update.
"""
//bool isExposed() const override;
"""
Please delete it.
-------
src/platforms/
"""
* Copyright (C) 2013,2014 Canonical, Ltd.
"""
Please update.
"""
qDebug() << "ScreenWindow:
"""
This is a good oportunity to update this class to use categorized logging.
"""
auto windowscreen = static_cast<Screen *>(screen());
Q_ASSERT(
windowscree
"""
A windowScreen inside a screenWindow... Calling it simply "screen" would help keeping my sanity.
"""
//bool ScreenWindow:
//{
// return m_isExposed;
//}
"""
Please delete it.
"""
// Intercept Hide event and convert to Expose event, as Hide causes Qt to release GL
// resources, which we don't want. Must intercept Show to un-do hide.
if (event->type() == QEvent::Hide) {
qDebug() << "ScreenWindow:
m_isExposed = false;
//return true;
} else if (event->type() == QEvent::Show) {
qDebug() << "ScreenWindow:
m_isExposed = true;
//QRect rect(QPoint(), geometry().size());
//return true;
}
"""
And all this commented-out code too.
By the way, does the comment above this if{}else{} still hold? You're no longer converting them to expose events here.
Now that this class no longer has a isExposed() getter, is this m_isExposed variable being used for anything?
If not, I guess you could even get rid of the entire ScreenWindow:
"""
qDebug() << "ScreenWindow:
"""
Do we want to keep this?
-------
src/platforms/
"""
* Copyright (C) 2014 Canonical, Ltd.
"""
Time has passed.
-------
src/platforms/
"""
* Copyright (C) 2014 Canonical, Ltd.
"""
Same here.
-------
tests/common/
"""
const mg::DisplayConf
"""
We use camelCase.
-------
tests/mirserver
"""
ScreenContr
"""
I don't think this two-letters name helps with readability.