-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: add file and color controller setting types #13669
base: main
Are you sure you want to change the base?
feat: add file and color controller setting types #13669
Conversation
These setting types are great features! For the file selector I would expect a widget like this: More complicated might be a generic definiton for colors, as:
|
Thanks for the early feedback!
Yes, that makes sense. It's in the todo list already :)
The current widget are more tailored for palette AFAIK, not unique colour selection, so not sure how to do this? I was thinking of adding a color indication inside/next to the pick button
Yes, this usecase is already covered with enum, and actually used by the S4Mk3. One bits of improvement could be to add some color indication in the dropdown, I guess we could easily add an attribute on the |
I think using the Qt color picker is fine. If we want to we can pre-fill their color with for example the users currently track-color palette (the palette source could also be configured as part of the setting). But lets KISS for now until we have a good reason to couple this to our palettes. A palette selector could be interesting indeed, but that is separate from the color picker. |
b167d82
to
74eebc4
Compare
a1cf79d
to
d91587e
Compare
As suggested by @JoergAtGithub , I've also added the ability to add a color icon next to a dropdown menu, which may be used for color selection. |
friend class LegacyControllerMappingSettingsTest_enumSettingEditing_Test; | ||
friend class ControllerS4MK3SettingTest_ensureLibrarySettingValueAndEnumEquals; |
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.
prefer gtest FRIEND_TEST macro.
@@ -230,7 +230,8 @@ LegacyControllerEnumSetting::LegacyControllerEnumSetting( | |||
!value.isNull(); | |||
value = value.nextSiblingElement("value")) { | |||
QString val = value.text(); | |||
m_options.append(std::tuple<QString, QString>(val, value.attribute("label", val))); | |||
QColor color = QColor(value.attribute("color")); | |||
m_options.append(Item{val, value.attribute("label", val), color}); |
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.
m_options.append(Item{val, value.attribute("label", val), color}); | |
m_options.emplace_back(val, value.attribute("label", val), color); |
const QDomElement& element) | ||
: AbstractLegacyControllerSetting(element) { | ||
m_defaultValue = QColor(element.attribute("default")); | ||
m_savedValue = m_defaultValue; | ||
m_editedValue = m_defaultValue; |
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.
prefer ctor initializer list. they're evaluated in order and you can refer between members. So this works without issues I'm pretty sure.
const QDomElement& element) | |
: AbstractLegacyControllerSetting(element) { | |
m_defaultValue = QColor(element.attribute("default")); | |
m_savedValue = m_defaultValue; | |
m_editedValue = m_defaultValue; | |
const QDomElement& element) | |
: AbstractLegacyControllerSetting(element), m_defaultValue(QColor(element.attribute("default")), m_savedValue(m_defaultValue), m_editedValue(m_defaultValue) { |
if (ok != nullptr) { | ||
*ok = false; | ||
} | ||
reset(); | ||
save(); | ||
|
||
m_savedValue = QColor(in); | ||
if (!m_editedValue.isValid()) { | ||
*ok = false; | ||
return; | ||
} | ||
m_editedValue = m_savedValue; |
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.
this doesn't work properly. you didn't check for nullptr
on the second access to *ok
and you never actually set it true
, potentially leaving it uninitialized (which should never be the case for an out pointer like this).
const QDomElement& element) | ||
: AbstractLegacyControllerSetting(element) { | ||
m_defaultValue = QFileInfo(element.attribute("default")); | ||
m_fileFilter = element.attribute("pattern"); | ||
m_savedValue = m_defaultValue; | ||
m_editedValue = m_defaultValue; |
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.
same here. prefer ctor init list.
if (ok != nullptr) { | ||
*ok = false; | ||
} | ||
reset(); | ||
save(); | ||
|
||
m_editedValue = QFileInfo(in); | ||
if (!m_editedValue.exists()) { | ||
*ok = false; | ||
return; | ||
} | ||
m_savedValue = m_editedValue; |
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.
same here.
auto* pWidget = new QWidget(pParent); | ||
pWidget->setLayout(new QHBoxLayout); | ||
auto* pPushButton = new QPushButton(tr("Browse..."), pWidget); | ||
auto* pLabel = new QLabel(QStringLiteral("<i>%1</i>").arg(tr("No file selected")), pWidget); |
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.
wdyt of this to make sure the format string doesn't get out of sync?
auto* pLabel = new QLabel(QStringLiteral("<i>%1</i>").arg(tr("No file selected")), pWidget); | |
auto* pLabel = new QLabel(QStringLiteral("<i>%1</i>").arg(tr("No file selected")), pWidget); | |
auto setLabelText = [pLabel](QString&& text) { | |
pLabel->setText(QStringLiteral("<i>%1</i>").arg(text)); | |
}; |
public: | ||
LegacyControllerColorSetting(const QDomElement& element); | ||
|
||
virtual ~LegacyControllerColorSetting() = default; |
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.
consider putting the = default;
into the implementation. Since there call needs to be virtual, there is no advantage to it being in the header (because it can't be inlined).
Also use override
when overriding instead of something that looks like a redeclaration.
virtual ~LegacyControllerColorSetting() = default; | |
~LegacyControllerColorSetting() override; |
friend class LegacyControllerMappingSettingsTest_enumSettingEditing_Test; | ||
friend class ControllerS4MK3SettingTest_ensureLibrarySettingValueAndEnumEquals; |
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'd prefer FRIEND_TEST
from <gtest/gtest_prod.h>
.
static AbstractLegacyControllerSetting* createFrom(const QDomElement& element) { | ||
return new LegacyControllerColorSetting(element); | ||
} | ||
static inline bool match(const QDomElement& element); |
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.
inline
? doesn't make sense afaict. If you want to optimize the call away, you'll have to move the implementation here.
Add new controller setting type, particularly useful for screen mapping.
Screencast.from.19-09-24.10.02.23.mp4
TODO: