feat: improve screen rendering framework#13737
Conversation
github-actions
Bot
added
controller mappings
build
code quality
qml
controller backend
labels
acolombier
force-pushed
the
feat/improve-screen-rendering-framework
branch
from
c6fd897 to
99fed94
Compare
Swiftb0y
left a comment
Swiftb0y
left a comment
There was a problem hiding this comment.
only remaining complaint from me tbh
acolombier
force-pushed
the
feat/improve-screen-rendering-framework
branch
from
f114197 to
9b2aafa
Compare
| #ifdef MIXXX_USE_QML | ||
| m_rootItems.clear(); | ||
| #endif |
There was a problem hiding this comment.
does it work without the explicit .clear()? Ideally RAII would take care of this fully by itself.
There was a problem hiding this comment.
It will probably work, but I preferred to make it explicit so if something was to crash when deallocating the screen, it still happens as part of the test hooks
There was a problem hiding this comment.
makes sense. It was more of a general question whether this would be needed in production code.
| } | ||
|
|
||
| return rootItem; | ||
| return std::unique_ptr<mixxx::qml::QmlMixxxControllerScreen>(rootItem); |
There was a problem hiding this comment.
I think in order to truly benefit from resource safety here, the unique_ptr wrapping needs to be done on the return value of qmlComponent.createWithInitialProperties (directly in the same statement) (potentially after a qobject_cast). I don't want to force you to do it, but I don't have the time right now. If you're up for it, feel free to do it now and if not, remind me on sunday and I will file a PR against your branch.
There was a problem hiding this comment.
I did try to give this a go but could not get around doing the dynamic_cast for the unique_ptr. Are you happy to address that as a follow up PR? I know you are busy lately, so I wouldn't want to add too much to your todolist, but also keen to close that PR ASAP to move forward with S4 Mk3 if we want it to hit the 2.6 deadline.
There was a problem hiding this comment.
yeah I'm just worried about forgetting about it. But you're right, lets postpone for now.
Swiftb0y
left a comment
Swiftb0y
left a comment
There was a problem hiding this comment.
LGTM. Thank you for the cleanup. Please rebase.
acolombier
force-pushed
the
feat/improve-screen-rendering-framework
branch
from
c192b8a to
533cedb
Compare
|
Merge? |
|
yup, sorry. I've been busy until now. |
2 participants
This is a refactor largely inspired from the work in #13459 to simplify the hook system used for screen rendering.