use single class for all allshader waveformwidget types with info class #13249
Conversation
| useOpenGLShaders = info->m_useGLSL; | ||
| category = info->m_category; | ||
| } else { | ||
| switch (type) { |
There was a problem hiding this comment.
once WaveformWidgetInfo's have been implemented for all WaveformWidgetTypes this switch can be removed.
| // waveform widget types registered with WaveformWidgetInfo's | ||
| widget = info->create(viewer->getGroup(), viewer).release(); | ||
| } else { | ||
| switch (type) { |
There was a problem hiding this comment.
likewise, once WaveformWidgetInfo's have been implemented for all WaveformWidgetTypes this switch can be removed, and the creation of the WaveformWidget's is done with
widget = info->create(viewer->getGroup(), viewer).release();
above.
| WaveformWidgetInfoBase(WaveformWidgetType::Type type, | ||
| QString name, | ||
| bool useGL, | ||
| bool useGLES, | ||
| bool useGLSL, | ||
| bool highDetail, | ||
| WaveformWidgetCategory category) |
There was a problem hiding this comment.
instead of an opaque constructor with a lot of bools, it would be clearer for this take a flag enum or bitset. or maybe this sweet new c++20 feature? https://www.cppstories.com/2017/03/on-toggle-parameters/#how-about-c20
There was a problem hiding this comment.
Yes, it most definitely would, but I prefer to do these refactorings one step at a time. The original code uses these booleans, so I will stay close to that approach in this PR. A follow up PR can be uses to change them for flags.
| const WaveformWidgetType::Type m_type; | ||
| const QString m_name; | ||
| const bool m_useGL; | ||
| const bool m_useGLES; | ||
| const bool m_useGLSL; | ||
| const bool m_highDetail; | ||
| const WaveformWidgetCategory m_category; |
There was a problem hiding this comment.
CppCoreGuidelines C.12: Don't make data members const or references in a copyable or movable type.
| const WaveformWidgetType::Type m_type; | |
| const QString m_name; | |
| const bool m_useGL; | |
| const bool m_useGLES; | |
| const bool m_useGLSL; | |
| const bool m_highDetail; | |
| const WaveformWidgetCategory m_category; | |
| WaveformWidgetType::Type m_type; | |
| QString m_name; | |
| bool m_useGL; | |
| bool m_useGLES; | |
| bool m_useGLSL; | |
| bool m_highDetail; | |
| WaveformWidgetCategory m_category; |
| category) { | ||
| infos().push_back(this); | ||
| } |
There was a problem hiding this comment.
this class looks like a value-type but in reality uses mutable static data internally thus calling this constructor is not thread-safe. This is a giant code-smell to me.
There was a problem hiding this comment.
I see the value of the approach here, but I don't like the state in the value-type. Instead it would be nicer if the WaveformWidgetInfo remained a POD and all the static data handling was happening in a separate WaveformWidgetInfoRegistry class. That then contains the WaveformWidget::registerInfos and WaveformWidgetInfoBase::find implementations acting on its own private static data. Does that make sense?
|
This has been replaced with acolombier#2 , which is on top of @acolombier PR #13220 |
EDIT: I will redo this work on top of 13220 instead.
This is a code quality improvement PR.
There is a lot of code duplication in the waveform widget classes. This PR proposes a way to reduce this by introducing the WaveformWidgetInfo class which contains the information for each WaveformWidgetType, rather than having this information in concrete WaveformWidget classes. The WaveformWidgetInfo class also serves as a factory (creator) for the WaveformWidget class.
I have so far implemented this for the allshader widgets only, but this can easily be extended to the rest of the code.
Apart from the code duplication reduction, other advantages are that: it removes the need for huge switches in WaveformWidgetFactory, it replaces the need to use ifdefs with registered WaveformWidgetInfo's, and allows to obtain info directly based on the WaveformWidgetType instead of having to know the concrete WaveformWidget class.