Changeset View
Standalone View
platform.h
Show All 39 Lines | |||||
40 | 40 | | |||
41 | namespace KWin | 41 | namespace KWin | ||
42 | { | 42 | { | ||
43 | namespace ColorCorrect { | 43 | namespace ColorCorrect { | ||
44 | class Manager; | 44 | class Manager; | ||
45 | struct GammaRamp; | 45 | struct GammaRamp; | ||
46 | } | 46 | } | ||
47 | 47 | | |||
48 | class AbstractOutput; | ||||
48 | class Edge; | 49 | class Edge; | ||
49 | class Compositor; | 50 | class Compositor; | ||
50 | class OverlayWindow; | 51 | class OverlayWindow; | ||
51 | class OpenGLBackend; | 52 | class OpenGLBackend; | ||
52 | class Outline; | 53 | class Outline; | ||
53 | class OutlineVisual; | 54 | class OutlineVisual; | ||
54 | class QPainterBackend; | 55 | class QPainterBackend; | ||
55 | class Scene; | 56 | class Scene; | ||
56 | class Screens; | 57 | class Screens; | ||
57 | class ScreenEdges; | 58 | class ScreenEdges; | ||
58 | class Toplevel; | 59 | class Toplevel; | ||
59 | class WaylandCursorTheme; | 60 | class WaylandCursorTheme; | ||
60 | 61 | | |||
61 | namespace Decoration | 62 | namespace Decoration | ||
62 | { | 63 | { | ||
63 | class Renderer; | 64 | class Renderer; | ||
64 | class DecoratedClientImpl; | 65 | class DecoratedClientImpl; | ||
65 | } | 66 | } | ||
66 | 67 | | |||
68 | class KWIN_EXPORT Outputs : public QVector<AbstractOutput*> | ||||
romangg: Is the KWIN_EXPORT really needed? | |||||
69 | { | ||||
70 | public: | ||||
71 | Outputs(){}; | ||||
zzag: Why do we need it? | |||||
Backends have vector<Derived> in their private implementations. We want to have a You can't nicely cast a list, and someone above didn't want to unnicely cast the list. This new class does the list upcasting in a more implicit and central way. davidedmundson: Backends have vector<Derived> in their private implementations.
We want to have a
virtual… | |||||
zzag: I'm about default constructor. We don't need it, right?
| |||||
Oh I see, sorry. There's a base implementation in platform.cpp which returns an always empty list. davidedmundson: Oh I see, sorry.
There's a base implementation in platform.cpp which returns an always empty… | |||||
Why? Compiler will generate default constructor(and other special member functions) for us. Also, you could return {}. :-) zzag: > There's a base implementation in platform.cpp which returns an always empty list.
> Without… | |||||
Compiler will only implicitly declare a default constructors if I haven't made any other custom constructors, so not in this case. /home/david/projects/kde5/src/kde/workspace/kwin/kcmkwin/kwinrules/../../platform.h:432:24: error: no matching function for call to ‘KWin::Outputs::Outputs()’ return Outputs(); You are correct that I could change it to Outputs() = default; and use the generated definition, but it still needs exlicitly declaring. Just changing the return value to {} instead of the default constructor won't work. The compiler would have to guess the template of the "QVector<T> other" from our inner constructor which it can't do, even when the list is empty. /home/david/projects/kde5/src/kde/workspace/kwin/platform.h: In member function ‘virtual KWin::Outputs KWin::Platform::outputs() const’: /home/david/projects/kde5/src/kde/workspace/kwin/platform.h:428:17: error: could not convert ‘<brace-enclosed initializer list>()’ from ‘<brace-enclosed initializer list>’ to ‘KWin::Outputs’ return {}; A default constructor and {} works, but that's not really any better. davidedmundson: Compiler will only implicitly declare a default constructors if I haven't made any other custom… | |||||
Oh, yeah, you're right. I forgot that if any constructor is declared, then default constructor is not generated. Sorry, my bad. Anyway, let's stick with =default. zzag: Oh, yeah, you're right. I forgot that if any constructor is declared, then default constructor… | |||||
72 | template <typename T> | ||||
zzag: `typename T` | |||||
romangg: Either is fine. | |||||
Both do the same, but typename is the new "correct" one. I'm just very ancient. davidedmundson: Both do the same, but typename is the new "correct" one. I'm just very ancient.
Please can you… | |||||
73 | Outputs(const QVector<T> &other) { | ||||
74 | resize(other.size()); | ||||
This must be a call to resize. Otherwise std::copy doesn't copy anything. romangg: This must be a call to `resize`. Otherwise std::copy doesn't copy anything. | |||||
zzag: No, std::copy has to copy "to" std::back_inserter. | |||||
Yes, it should be resize for sure https://en.cppreference.com/w/cpp/algorithm/copy anthonyfieroni: Yes, it should be resize for sure https://en.cppreference.com/w/cpp/algorithm/copy | |||||
Could you please explain why? (I don't see any reference on that page why we have to resize vectors before copying) zzag: Could you please explain why? (I don't see any reference on that page why we have to resize… | |||||
Sure it has: template<class InputIt, class OutputIt> OutputIt copy(InputIt first, InputIt last, OutputIt d_first) { while (first != last) { *d_first++ = *first++; } return d_first; } There is no back_inserter here and never have., you can try it on online compiler. anthonyfieroni: Sure it has:
```
template<class InputIt, class OutputIt>
OutputIt copy(InputIt first, InputIt… | |||||
std::copy(other.constBegin(), other.constEnd(), std::back_inserter(*this)); I don't see anything wrong with this. zzag: std::copy(other.constBegin(), other.constEnd(), std::back_inserter(*this));
I don't see… | |||||
romangg: Does using std::back_inserter have advantages over calling resize? | |||||
It has the advantage that you don't initialise the space with the default value only to replace it. davidedmundson: It has the advantage that you don't initialise the space with the default value only to replace… | |||||
75 | std::copy(other.constBegin(), other.constEnd(), begin()); | ||||
76 | } | ||||
77 | }; | ||||
78 | | ||||
67 | class KWIN_EXPORT Platform : public QObject | 79 | class KWIN_EXPORT Platform : public QObject | ||
68 | { | 80 | { | ||
69 | Q_OBJECT | 81 | Q_OBJECT | ||
70 | public: | 82 | public: | ||
71 | virtual ~Platform(); | 83 | virtual ~Platform(); | ||
72 | 84 | | |||
73 | virtual void init() = 0; | 85 | virtual void init() = 0; | ||
74 | virtual Screens *createScreens(QObject *parent = nullptr); | 86 | virtual Screens *createScreens(QObject *parent = nullptr); | ||
▲ Show 20 Lines • Show All 331 Lines • ▼ Show 20 Line(s) | 416 | virtual int gammaRampSize(int screen) const { | |||
406 | return 0; | 418 | return 0; | ||
407 | } | 419 | } | ||
408 | virtual bool setGammaRamp(int screen, ColorCorrect::GammaRamp &gamma) { | 420 | virtual bool setGammaRamp(int screen, ColorCorrect::GammaRamp &gamma) { | ||
409 | Q_UNUSED(screen); | 421 | Q_UNUSED(screen); | ||
410 | Q_UNUSED(gamma); | 422 | Q_UNUSED(gamma); | ||
411 | return false; | 423 | return false; | ||
412 | } | 424 | } | ||
413 | 425 | | |||
426 | // outputs with connections (org_kde_kwin_outputdevice) | ||||
What's this method going to be for? It's currently unused Guessing at use cases, you'd probably want to sync it with Screens, at which point you're going to want something more like AbstractOutput* Screens::nativeOutput(int) which would make this a non-issue. davidedmundson: What's this method going to be for? It's currently unused
Guessing at use cases, you'd… | |||||
The methods are used in the follow-up patches. The sync with Screens is done already, but in the end we want to get rid of the Screens class and not depend on it through a AbstractOutput getter. romangg: The methods are used in the follow-up patches. The sync with Screens is done already, but in… | |||||
427 | virtual Outputs outputs() const { | ||||
428 | return Outputs(); | ||||
zzag: I think one could do just
```lang=cpp
return {};
``` | |||||
429 | } | ||||
430 | // actively compositing outputs (wl_output) | ||||
431 | virtual Outputs enabledOutputs() const { | ||||
432 | return Outputs(); | ||||
433 | } | ||||
434 | | ||||
414 | /* | 435 | /* | ||
415 | * A string of information to include in kwin debug output | 436 | * A string of information to include in kwin debug output | ||
416 | * It should not be translated. | 437 | * It should not be translated. | ||
417 | * | 438 | * | ||
418 | * The base implementation prints the name. | 439 | * The base implementation prints the name. | ||
419 | * @since 5.12 | 440 | * @since 5.12 | ||
420 | */ | 441 | */ | ||
421 | virtual QString supportInformation() const; | 442 | virtual QString supportInformation() const; | ||
Show All 23 Lines | 444 | public Q_SLOTS: | |||
445 | void processPinchGestureEnd(quint32 time); | 466 | void processPinchGestureEnd(quint32 time); | ||
446 | void processPinchGestureCancelled(quint32 time); | 467 | void processPinchGestureCancelled(quint32 time); | ||
447 | 468 | | |||
448 | Q_SIGNALS: | 469 | Q_SIGNALS: | ||
449 | void screensQueried(); | 470 | void screensQueried(); | ||
450 | void initFailed(); | 471 | void initFailed(); | ||
451 | void cursorChanged(); | 472 | void cursorChanged(); | ||
452 | void readyChanged(bool); | 473 | void readyChanged(bool); | ||
453 | /** | 474 | /** | ||
graesslin: nitpick :-) | |||||
454 | * Emitted by backends using a one screen (nested window) approach and when the size of that changes. | 475 | * Emitted by backends using a one screen (nested window) approach and when the size of that changes. | ||
455 | **/ | 476 | **/ | ||
456 | void screenSizeChanged(); | 477 | void screenSizeChanged(); | ||
457 | 478 | | |||
458 | protected: | 479 | protected: | ||
459 | explicit Platform(QObject *parent = nullptr); | 480 | explicit Platform(QObject *parent = nullptr); | ||
460 | void setSoftWareCursor(bool set); | 481 | void setSoftWareCursor(bool set); | ||
461 | void handleOutputs() { | 482 | void handleOutputs() { | ||
Show All 33 Lines | |||||
495 | * This method is invoked by showCursor if the cursor needs to be shown again. | 516 | * This method is invoked by showCursor if the cursor needs to be shown again. | ||
496 | * | 517 | * | ||
497 | * @see doShowCursor | 518 | * @see doShowCursor | ||
498 | * @see hideCursor | 519 | * @see hideCursor | ||
499 | * @see showCursor | 520 | * @see showCursor | ||
500 | **/ | 521 | **/ | ||
501 | virtual void doShowCursor(); | 522 | virtual void doShowCursor(); | ||
502 | 523 | | |||
503 | private: | 524 | private: | ||
there is no such thing as a protected variable in modern object oriented code. Variables are always private. If you need to access it should be through getters and setters. Please see for example https://softwareengineering.stackexchange.com/questions/162643/why-is-clean-code-suggesting-avoiding-protected-variables Also I'm not sure whether you are doing yourself a favor with how you did this. By making the variables protected and accessing them from the subclass, you have to constantly to static_cast. This looks like suboptimal object oriented design. If you would pass the m_outputs from subclass to parent class, you could pass the QVector<DrmOutput*> for the QVector<Output*> (it downcasts just fine) and you would not need to cast from the subclasses by either keeping the vector there as well or do something nice here like: template <typename T> QVector<T> outputs() const { static_assert(std::is_base_of<Output, T>::value, "Template T must be derived from Output"); return static_cast<QVector<T*>>(m_outputs); } (code is not compile checked) graesslin: there is no such thing as a protected variable in modern object oriented code. Variables are… | |||||
As mentioned earlier I wanted to keep these protected variables in this patch series as an intermediate solution and change them later in private variables with appropriate getters and protected setters. The reasons are to not again do massive rebasing of dependent diffs and keep the semantic changes in this patch here low.
This is not working for me. I have to cast the vector elements individually. I don't want to keep the vector in parent and child class, so I don't have to keep two vectors in sync all the time. The templated function is a nice idea for a protected getter. The problem is that at the moment the child classes assign the vectors though. So one need also several setter functions. I have now decided to put the QVectors into the child classes of Platform and copy the vector to QVector<AbstractOutput> if outputs() is called. This seems currently like the most straightforward solution. romangg: As mentioned earlier I wanted to keep these protected variables in this patch series as an… | |||||
504 | void triggerCursorRepaint(); | 525 | void triggerCursorRepaint(); | ||
505 | bool m_softWareCursor = false; | 526 | bool m_softWareCursor = false; | ||
506 | struct { | 527 | struct { | ||
507 | QRect lastRenderedGeometry; | 528 | QRect lastRenderedGeometry; | ||
508 | } m_cursor; | 529 | } m_cursor; | ||
509 | bool m_handlesOutputs = false; | 530 | bool m_handlesOutputs = false; | ||
510 | bool m_ready = false; | 531 | bool m_ready = false; | ||
511 | QSize m_initialWindowSize; | 532 | QSize m_initialWindowSize; | ||
Show All 19 Lines |
Is the KWIN_EXPORT really needed?