Is it safe? Shouldn't you test the pointer for validity?
Also, it looks like DrmOutput deletes objects that it doesn't own. Is it a good idea?

virtual is redundant. It's already virtual.

You can delete a nullptr.

But in general yes, the waylandOutput() and waylandOutputDevice() data fields should be deleted on Output level. I just wanted to keep the logical changes minimal and delete them at the same position as before. But yea, accepting this logical change in this case probably makes more sense.

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.

Ok, let me change this after merge. I forgot it now on the rebase and all the rebasing is nerve wrecking.

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.

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.

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)

static_cast? That sounds wrong. I doubt you need it. Just do:

auto o = *it;

same here, static_cast looks wrong

here as well the static cast looks like not needed

and here as well

I don't understand why you changed from QVector<DrmOutput*> to QVector<Output*>. The list still gets filled with DrmOutput and afterwards you make your life hard by having to static_cast back to DrmOutput.

Please no static_casts. Either change the signature of outputRemoved signal or do a safe cast like qobject_cast or dynamic_cast. Static_cast is evil :-)

If connectedOutputs would still be QVector<DrmOutput*> you would not need the cast here.

same here. The change would not be needed if the signature were not changed.

please a safe cast like qobject_cast or dynamic_cast

same here

and here

Does the class need a DrmOutput or could it be changed to operate on Output instead?

just as an idea: maybe change name of the new Output to AbstractOutput to not need to have to rename here. I personally like to call abstract classes AbstractOutput. But I leave that to you, it's just an idea I got while seeing this change.

Hmm, this was similar to how DrmOutput was. But I will add one.

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?

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.

you could pass the QVector<DrmOutput*> for the QVector<Output*> (it downcasts just fine)

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.

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.

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.

Nitpick: it would be great to sort them.

Why do we need it?

typename T

I think one could do just

return {};

Is the KWIN_EXPORT really needed?

Backends have vector<Derived> in their private implementations.

We want to have a
virtual vector<Base>() in the

You can't nicely cast a list, and someone above didn't want to unnicely cast the list.
Roman had effectively this code in all backends.

This new class does the list upcasting in a more implicit and central way.

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.
Without this constructor that line becomes quite messy.

There's a base implementation in platform.cpp which returns an always empty list.
Without this constructor that line becomes quite messy.

Why?

Compiler will generate default constructor(and other special member functions) for us. Also, you could return {}. :-)

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.

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.

Ok. On commit.

Ok. On commit.

Either is fine.

This must be a call to resize. Otherwise std::copy doesn't copy anything.

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

Could you please explain why? (I don't see any reference on that page why we have to resize vectors before copying)

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.

std::copy(other.constBegin(), other.constEnd(), std::back_inserter(*this));

I don't see anything wrong with this.

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.