Code review comment for lp://staging/~nick-dedekind/qtmir/WindowNotifierObserver

Revision history for this message
Gerry Boland (gerboland) wrote :

+class WindowNotifierObserver : public QObject
+{
+ Q_OBJECT
+public:
+ WindowNotifierObserver(const miral::Window &window);
+ virtual ~WindowNotifierObserver();
+
+Q_SIGNALS:
+ void surfaceCreated();
+ void surfaceRemoved();
...

Since you use inheritance for the Impl, I find it wasteful to use signal/slots - you could just define virtual functions in the interface that the implementation can override. So

+class WindowNotifierObserver : public QObject
+{
+ Q_OBJECT
+public:
+ WindowNotifierObserver(const miral::Window &window);
+ virtual ~WindowNotifierObserver();
+
+ virtual void surfaceCreated();
+ virtual void surfaceRemoved();
...

Also instead of having a Hash table of miral::Window to observers, could you use the miral::WindowInfo::user_data, and attach the Observer to that? As you've said before, we don't really need multiple observers. Would mean one less data structure to maintain.

Would still need to take care with threading tho. WDYT?

review: Needs Information

« Back to merge proposal