Remove boilerplate and duplication in controller setting definition #13920
Conversation
acolombier
force-pushed
the
feat/refactor-controller-setting
branch
from
172c8df to
d4182ef
Compare
acolombier
force-pushed
the
feat/refactor-controller-setting
branch
from
d4182ef to
e6dbe5b
Compare
| doc.setContent(QString(kValidBoolean).arg("false").toLatin1()); | ||
| { | ||
| doc.setContent(QString(kValidBoolean).arg("false").toLatin1()); |
There was a problem hiding this comment.
Was the removal of the heap allocation performance-motivated? If so I think there is a lot more we can improve here. Would you like some suggestions for that?
There was a problem hiding this comment.
Not really performance motivated since those tests are already sufficiently performant IMO (the whole suite run in ~1s on my machine), this is only because there was no need for it to be dynamically allocated
There was a problem hiding this comment.
Would you be okay to submit a commit to improve the tests?
| void reset() override { | ||
| m_editedValue = m_defaultValue; | ||
| emit valueReset(); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think there are some more members we could try to implement conditionally. For example we could automatically implement stringify if the underlying type supports that (using C++20 concepts of SFINAE). Same goes for value(). Wdyt?
There was a problem hiding this comment.
Sounds like a great idea, although I wouldn't know how to do it. Would you be interested in submitting a commit to address that?
There was a problem hiding this comment.
Still don't have time to do a proper commit, but you can simply leverage the duck-typing nature of templates to do it. Something like this demo I came up with. The compiler message is not pretty in the error-case, but that could be rectified by guarding the print with a concept Printable = ... and a requires or static_assert.
| using LegacyControllerSettingBase<SettingType>::m_savedValue; | ||
| using LegacyControllerSettingBase<SettingType>::m_defaultValue; | ||
| using LegacyControllerSettingBase<SettingType>::m_editedValue; | ||
| using LegacyControllerSettingBase<SettingType>::save; | ||
| using LegacyControllerSettingBase<SettingType>::reset; | ||
| using LegacyControllerSettingBase<SettingType>::connect; | ||
| using LegacyControllerSettingBase<SettingType>::changed; |
There was a problem hiding this comment.
can you explain why this is needed? The member functions should already be public while the member variables shouldn't be.
There was a problem hiding this comment.
Without this, I get error: ‘m_...’ was not declared in this scope
I believe this has to do with the multiple inheritance but I'm not quite sure tbh. Any better fix for that?
There was a problem hiding this comment.
thats weird, especially considering the other classes don't seem to need that. I'd love to see the full error message on that. I'll try to find some time to look into it.
There was a problem hiding this comment.
I'll try and post the full error here when I get the time
There was a problem hiding this comment.
The exact error is
error: ‘<reference>’ was not declared in this scope
My gut feeling is that it has to do with the multiple inheritance causing issue with visibility, but not entirely sure
There was a problem hiding this comment.
does it say <reference> verbatim? I've never seen that.
There was a problem hiding this comment.
No it gives me the error for m_savedValue, m_defaultValue, m_editedValue, etc... at every lines referencing this symbol, e.g:
...: error: ‘m_savedValue’ was not declared in this scope
...
...: error: ‘m_defaultValue’ was not declared in this scope
...
There was a problem hiding this comment.
Ah yeah, that makes more sense. I'll try to look into it when I inevitably stumble over this in the future.
|
Friendly ping @Swiftb0y :) Appreciate this is not perfect, but it feels like an improvement from where we are at. Are you happy to merge it as is after fixup squash? |
| } | ||
|
|
||
| protected: | ||
| LegacyControllerSettingMixin(const QDomElement& element) |
There was a problem hiding this comment.
https://en.m.wikipedia.org/wiki/Mixin
I must confess I didn't know it either a few seconds ago 😃
acolombier
force-pushed
the
feat/refactor-controller-setting
branch
from
14fef57 to
277af1f
Compare
acolombier
force-pushed
the
feat/refactor-controller-setting
branch
from
277af1f to
5b9dfa5
Compare
acolombier
force-pushed
the
feat/refactor-controller-setting
branch
from
5b9dfa5 to
f45cd37
Compare
|
Removing the last commit as it doesn't seem to be the fix for the warning. Further work will be required to investigate #14315 |
|
I'm not aware of any blocker,s so I think this can go in! Thanks for looking into it |
|
Great. Thanks. |
As discussed in #13669, this PR introduced a refactor of the setting definition with a generic to remove code duplication and the needed boilerplate when introduced new setting type.