Changeset View
Standalone View
output.h
Show First 20 Lines • Show All 42 Lines • ▼ Show 20 Line(s) | |||||
43 | 43 | | |||
44 | namespace KWin | 44 | namespace KWin | ||
45 | { | 45 | { | ||
46 | 46 | | |||
47 | /** | 47 | /** | ||
48 | * Generic output representation in a Wayland session | 48 | * Generic output representation in a Wayland session | ||
49 | **/ | 49 | **/ | ||
50 | class KWIN_EXPORT Output : public QObject | 50 | class KWIN_EXPORT Output : public QObject | ||
51 | { | 51 | { | ||
graesslin: I'm missing a ctor in this class. If you don't specify the default ctor is added which is not… | |||||
romangg: Hmm, this was similar to how DrmOutput was. But I will add one. | |||||
52 | Q_OBJECT | 52 | Q_OBJECT | ||
53 | public: | 53 | public: | ||
54 | virtual ~Output() {} | 54 | virtual ~Output() {} | ||
55 | 55 | | |||
56 | QString name() const; | 56 | QString name() const; | ||
57 | bool isEnabled() const { | 57 | bool isEnabled() const { | ||
58 | return !m_waylandOutput.isNull(); | 58 | return !m_waylandOutput.isNull(); | ||
59 | } | 59 | } | ||
60 | 60 | | |||
61 | virtual QSize pixelSize() const = 0; | 61 | virtual QSize pixelSize() const = 0; | ||
62 | qreal scale() const { | 62 | qreal scale() const { | ||
63 | return m_scale; | 63 | return m_scale; | ||
64 | } | 64 | } | ||
65 | /* | 65 | /* | ||
66 | * The geometry of this output in global compositor co-ordinates (i.e scaled) | 66 | * The geometry of this output in global compositor co-ordinates (i.e scaled) | ||
Don't refer to anything in code/comments as scaled. It could mean scaled from logical to device or could mean scaled from device to logical. davidedmundson: Don't refer to anything in code/comments as scaled.
It could mean scaled from logical to… | |||||
Ok, let me change this after merge. I forgot it now on the rebase and all the rebasing is nerve wrecking. romangg: Ok, let me change this after merge. I forgot it now on the rebase and all the rebasing is nerve… | |||||
67 | */ | 67 | */ | ||
68 | QRect geometry() const; | 68 | QRect geometry() const; | ||
69 | QSize physicalSize() const; | 69 | QSize physicalSize() const; | ||
70 | Qt::ScreenOrientation orientation() const { | 70 | Qt::ScreenOrientation orientation() const { | ||
71 | return m_orientation; | 71 | return m_orientation; | ||
72 | } | 72 | } | ||
73 | 73 | | |||
74 | bool isInternal() const { | 74 | bool isInternal() const { | ||
75 | return m_internal; | 75 | return m_internal; | ||
76 | } | 76 | } | ||
77 | 77 | | |||
78 | void setGlobalPos(const QPoint &pos); | 78 | void setGlobalPos(const QPoint &pos); | ||
79 | void setScale(qreal scale); | 79 | void setScale(qreal scale); | ||
80 | 80 | | |||
81 | /** | 81 | /** | ||
82 | * This sets the changes and tests them against the specific output | 82 | * This sets the changes and tests them against the specific output | ||
83 | */ | 83 | */ | ||
84 | void setChanges(KWayland::Server::OutputChangeSet *changeset); | 84 | void setChanges(KWayland::Server::OutputChangeSet *changeset); | ||
85 | virtual bool commitChanges() { return false; } | 85 | virtual bool commitChanges() { return false; } | ||
86 | 86 | | |||
87 | const QPointer<KWayland::Server::OutputInterface> getWaylandInterface() const { | 87 | const QPointer<KWayland::Server::OutputInterface> getWaylandInterface() const { | ||
88 | return m_waylandOutput; | 88 | return m_waylandOutput; | ||
89 | } | 89 | } | ||
90 | 90 | | |||
91 | protected: | 91 | protected: | ||
please no protected variables (see e.g. https://softwareengineering.stackexchange.com/questions/162643/why-is-clean-code-suggesting-avoiding-protected-variables ) graesslin: please no protected variables (see e.g. https://softwareengineering.stackexchange. | |||||
92 | QPointer<KWayland::Server::OutputChangeSet> getChangeset() const { | ||||
93 | return m_changeset; | ||||
94 | } | ||||
95 | QPointer<KWayland::Server::OutputInterface> getWaylandOutput() const { | ||||
96 | return m_waylandOutput; | ||||
97 | } | ||||
98 | void setWaylandOutput(KWayland::Server::OutputInterface *set); | ||||
99 | QPointer<KWayland::Server::OutputDeviceInterface> getWaylandOutputDevice() const { | ||||
100 | return m_waylandOutputDevice; | ||||
101 | } | ||||
102 | void setWaylandOutputDevice(KWayland::Server::OutputDeviceInterface *set); | ||||
103 | QPoint globalPos() const { | ||||
104 | return m_globalPos; | ||||
105 | } | ||||
106 | | ||||
107 | QSize rawPhysicalSize() const { | ||||
108 | return m_physicalSize; | ||||
109 | } | ||||
110 | void setRawPhysicalSize(const QSize &set) { | ||||
111 | m_physicalSize = set; | ||||
112 | } | ||||
113 | | ||||
114 | void setOrientation(Qt::ScreenOrientation set) { | ||||
115 | m_orientation = set; | ||||
116 | } | ||||
117 | bool internal() const { | ||||
118 | return m_internal; | ||||
119 | } | ||||
120 | void setInternal(bool set) { | ||||
121 | m_internal = set; | ||||
122 | } | ||||
123 | | ||||
124 | private: | ||||
92 | QPointer<KWayland::Server::OutputChangeSet> m_changeset; | 125 | QPointer<KWayland::Server::OutputChangeSet> m_changeset; | ||
93 | QPointer<KWayland::Server::OutputInterface> m_waylandOutput; | 126 | QPointer<KWayland::Server::OutputInterface> m_waylandOutput; | ||
94 | QPointer<KWayland::Server::OutputDeviceInterface> m_waylandOutputDevice; | 127 | QPointer<KWayland::Server::OutputDeviceInterface> m_waylandOutputDevice; | ||
95 | 128 | | |||
96 | QPoint m_globalPos; | 129 | QPoint m_globalPos; | ||
97 | qreal m_scale = 1; | 130 | qreal m_scale = 1; | ||
98 | QSize m_physicalSize; | 131 | QSize m_physicalSize; | ||
99 | Qt::ScreenOrientation m_orientation = Qt::PrimaryOrientation; | 132 | Qt::ScreenOrientation m_orientation = Qt::PrimaryOrientation; | ||
100 | bool m_internal = false; | 133 | bool m_internal = false; | ||
101 | }; | 134 | }; | ||
I have problems understanding the ownership concept of these pointers. On the one hand they are QPointer, which would indicate they are owned by another class (otherwise QPointer just doesn't make any sense, the only advantage is that it tracks when a QObject gets destroyed), on the other hand the pointers are passed in as raw pointers and last but not least they are deleted in the dtor. If you want to have ownership of the pointers in this class (the delete in dtor) I suggest using a smart pointer which supports this concept (e.g. std::unique_ptr), if you want to have shared pointers, you could use std::shared_ptr or QSharedPointer. graesslin: I have problems understanding the ownership concept of these pointers. On the one hand they are… | |||||
The QPointers were also copied from DrmOutput. Were they used differently there? I wanted to copy as much semantic as possible without changes to minimize risk for regressions with this large patch. The objects are created in the Display class of KWayland, which shares ownership and for example KWayland::Server::Display::createOutput only returns a raw pointer. I assume that's why these were QPointers? romangg: The QPointers were also copied from DrmOutput. Were they used differently there? I wanted to… | |||||
102 | 135 | | |||
103 | } | 136 | } | ||
104 | 137 | | |||
105 | #endif // KWIN_OUTPUT_H | 138 | #endif // KWIN_OUTPUT_H |
I'm missing a ctor in this class. If you don't specify the default ctor is added which is not typical for a QObject derived class. You want to have a ctor taking a QObject *parent = nullptr argument.