-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use enum class for waveform overview type #13370
Conversation
@@ -80,8 +81,8 @@ WOverview::WOverview( | |||
QStringLiteral("[Waveform]"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't understand those ControlFlag::NoAssertIfMissing
flags above. Some controls are read when painting and there is no null check. Even if default controls return 0 there will a division by 0 at one place.
Should we remove those flags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot imagine a valid reason that this is missing. Maybe a leftover from a refactoring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, so I'll remove those.
@@ -80,8 +81,8 @@ WOverview::WOverview( | |||
QStringLiteral("[Waveform]"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot imagine a valid reason that this is missing. Maybe a leftover from a refactoring?
aab4595
to
0758079
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thank you, couple more nits
int type = waveformOverviewComboBox->currentData().toInt(); | ||
m_pConfig->setValue(kOverviewTypeCfgKey, type); | ||
m_pTypeControl->forceSet(type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
int type = waveformOverviewComboBox->currentData().toInt(); | |
m_pConfig->setValue(kOverviewTypeCfgKey, type); | |
m_pTypeControl->forceSet(type); | |
QVariant comboboxData = waveformOverviewComboBox->currentData(); | |
DEBUG_ASSERT(comboboxData.canConvert<WOverview::Type>()); | |
auto type = comboboxData.value<WOverview::Type>(); | |
m_pConfig->setValue(kOverviewTypeCfgKey, type); | |
m_pTypeControl->forceSet(static_cast<double>(type)); |
0758079
to
9a7c1c8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. There is still more cleanup possible in future PRs, but this is already an improvement. Thank you.
0d69ec1
to
aa99a73
Compare
@daschuer do you want to take a look? |
1ffcbb0
to
91d9f6c
Compare
@Swiftb0y Looks good with the latest changes? |
yes, the last remaining unresolved comment is #13370 (comment) |
91d9f6c
to
e8eea9f
Compare
@ronso0 friendly ping ;) |
Isn't that fixed by the last fixup? |
Oh-oh-kay, got it now 😬 |
1741094
to
8d552b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Waiting for CI.
Lets merge this! Thank you! |
sorry, forgot, thank you. |
Follow-up for #13273