Would you agree on such a change?
case "Worksheet"_hash: is still human-readable (compare to if (className == "Worksheet").
The very unlikely case of a hash collision would appear at compile time (if new dock widgets are introduced).
On the other hand the selection of the right dock is now more efficient than before.

In D18420#397243, @croick wrote:

Would you agree on such a change?

Thanks for addressing this. This part grew historically with the help of copy&paste. It's time to do some cleanup here :-)

case "Worksheet"_hash: is still human-readable (compare to if (className == "Worksheet").
The very unlikely case of a hash collision would appear at compile time (if new dock widgets are introduced).

It's human readable but it's even more readable if we use somethink like this:

enum className {Spreadsheet, Worksheet, ...};
map classNames{ {"Spreadsheet", Spreadsheet}, ...};

switch (name)
     case Spreadsheet:
...

Here we'd would need to register every class name in the enum and to do the mapping of the string class name to the enum value. This would result in some additional maintenance (you need to know that you need to register the new class here) but at costs of better readability. And maybe we can use this (global) enum in future at other places where we probably do some dirty workarounds now because we don't have such an enum.

On the other hand the selection of the right dock is now more efficient than before.

I don't think the performance is relevant here. I'd rather go for a greater readability of the code. Performance-wise I'd say a simple lookup in the map as mentioned above is faster than the calculation of the hash.

Would you agree with such a solution?

I still prefer my solution ;)

But I agree, that the enum would be more consistent with the rest of the project. Actually, AbstractAspect could have a type() function that contains the actual type (like in AbstractFileFilter). That would entirely spare the use of a map and string comparisons, but would require touching 20 constructors or so.
What do you think?

In D18420#398876, @croick wrote:

I still prefer my solution ;)

But I agree, that the enum would be more consistent with the rest of the project. Actually, AbstractAspect could have a type() function that contains the actual type (like in AbstractFileFilter). That would entirely spare the use of a map and string comparisons, but would require touching 20 constructors or so.
What do you think?

This would be the best solution I think. We'd maintain this global enum in AbstractAspect.h ("AspectType" maybe to be consistent with FileType in AbstractFileFilter) and use it in the constructors. No map required. But yes, we'd need to touch now some code. Also, every tyme we add a new enum value in AbstractAspect.h we'd need to basically re-compile the whole project since this header file is pulled either directly or indirectly in many places... But let's go for a consistent solution here and benefit from compile time checks. Would you like to do this change as part of this patch?

If I had known in advance...

I applied the type() identifier where I thought it reasonable.
The naming is still a little unfortunate, but I cannot name the enum elements as their corresponding classes, because then the compiler doesn't know whether the enum type is meant or the class. Do you have a better idea than a "Type" suffix?
In some place there are checks for a non-existant "FileDataSource". I deleted those for now, since I don't know what they are referring to. Is that called "LiveDataSource" now?

In D18420#400631, @croick wrote:

If I had known in advance...

I applied the type() identifier where I thought it reasonable.
The naming is still a little unfortunate, but I cannot name the enum elements as their corresponding classes, because then the compiler doesn't know whether the enum type is meant or the class. Do you have a better idea than a "Type" suffix?

Yes, the naming is sub-optimal somehow... I think what we're trying to achieve here is kind of compile time reflection capabilities where c++ is lacking a lot of support for. And we're trying to add new stuff on top of Qt's meta-object system which already tries to overcome some of the limitations of the c++ language.

Should we try to utilize QMetaClassInfo and simply set a new property "classtype" to the corresponding value of the new enum you introduced? With this we'd only need to add a Q_CLASSINFO line in every header and don't need to touch the constructors and the cpp files. At run-time, we read this value out of QMetaClassInfo, cast it to the enum and use it in the switch-case block. With this we can probably remove "Type" suffix from the enum values.

In some place there are checks for a non-existant "FileDataSource". I deleted those for now, since I don't know what they are referring to. Is that called "LiveDataSource" now?

Yes, FileDataSource was renamed to LiveDataSource later. Thanks for cleaning up here.

Instead of an enum inside AbstractAspect, there now is an enum class outside of the class. This way, the items can be named properly.

QMetaClassInfo (const char*) and a mapping to enums would again be a source for new bugs once classes are renamed since there are no compile time checks.