Skip to content
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

QML Pt. 2: Add demo skin #3907

Merged
merged 14 commits into from
May 29, 2021
Merged

QML Pt. 2: Add demo skin #3907

merged 14 commits into from
May 29, 2021

Conversation

Holzhaus
Copy link
Member

Based on #3891 and #3894. This is just the WIP QML demo skin, no C++ changes.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took a quick look at some of QML code and made some comments about what I consider best practice from my limited research on QML. I haven't had the opportunity to run the demo in practice yet.

Styling is still a topic to be discussed. QtQuick primarily seems to support styling by using visual objects to delegate the drawing to the caller/instatiator. We might want to consider using the same pattern for QML components we ship with mixxx.

Comment on lines +33 to +35
anchors.top: parent.top
anchors.left: parent.left
anchors.bottom: parent.bottom
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
anchors.top: parent.top
anchors.left: parent.left
anchors.bottom: parent.bottom
anchors.verticalCenter: parent.verticalCenter
anchors.left: parent.left

can you try this out? not sure if it has the same result though...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this centers the item, but it won't show up because it has no height. The current code sets the height to that of the parent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok good to know, so the anchor doubles as a responsive layout guide.

anchors.top: parent.top
anchors.left: parent.left
anchors.bottom: parent.bottom
width: height
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if hardcoding the 1:1 aspect ration here is a good idea, but we can keep it like this for the sake of simplicity I guess. I'd prefer if this was documented in an inline comment though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI I tried to make a separation between skin-local QML and reusable QML that may be reused by multiple skins, similar to controller mappings vs. midi-components. The latter is in the Mixxx QML module (subdirectory).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong comment?
I was primarily talking about that album covers might not be square so we can't assume this in the UI

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah okay, I misunderstood. We could use an Item/Rectangle instead, and put the image inside of it that keeps the aspect ratio. But let's discuss this later on the qml-coverart PR to avoid merge conflicts. This is just a placeholder.

import QtQuick.Controls 2.15
import QtQuick.Layouts 1.11

Item {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is already quite big, I'd prefer to split everything into its own file if possible. At least Items which are already decoupled. Accessing properties from the deck itself could either be done by making use of QMLs dynamic scoping or by using (alias) properties.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I'd rather not make too many changes to not cause too many conflicts with the follow-up QML PRs. This is just a demo skin anyway, so we can refactor later :D It's more important that the reusable components in the Mixxx subdirectory are well-structured.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As long as it is a Demo skin thats fine, but the first QML skin should be well polished and using QML best practices. Otherwise others will copy that mediocre code and we end up with a similar situation as with the mappings.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I hope that once the demo skin is merged, other people start working on it and improve it, until we end up with a nice, polished skin.

Comment on lines +23 to +33
background: Image {
source: "../LateNight/palemoon/knobs/knob_bg_master.svg"
width: 35
height: 30
}

foreground: Image {
source: "../LateNight/palemoon/knobs/knob_indicator_regular_red.svg"
width: 35
height: 30
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use a Component to keep the code a bit more DRY:

Suggested change
background: Image {
source: "../LateNight/palemoon/knobs/knob_bg_master.svg"
width: 35
height: 30
}
foreground: Image {
source: "../LateNight/palemoon/knobs/knob_indicator_regular_red.svg"
width: 35
height: 30
}
knobImage: Component {
property alias source: image.source
Image: {
id: image
width: 35
height: 30
}
}
background: knobImage {
source: "../LateNight/palemoon/knobs/knob_bg_master.svg"
}
foreground: knobImage {
source: "../LateNight/palemoon/knobs/knob_indicator_regular_red.svg"
}

Its overkill in this case, but a handy technique to create components inline if they don't deserve their own file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks. Does this work in all QtQuick versions? I have Qt 5.15 and don't know if the current version even works on Qt 5.12.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm pretty pretty sure it does work on all versions.
Making the alias properties required would make sense as well, but I'm not sure if that has implications for the Qt version.

Copy link
Member Author

@Holzhaus Holzhaus May 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making the alias properties required would make sense as well

Nope, not possible. For normal properties you use one of these:

[default] [required] [readonly] property <propertyType> <propertyName>
[default] property <propertyType> <propertyName> : <value>

But for alias properties the signature is:

[default] property alias <name>: <alias reference>

So you can't make them required.

https://doc.qt.io/qt-5/qtqml-syntax-objectattributes.html#property-aliases

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, good to know, so I guess alias properties would get required automatically when they refer to a required resource?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so.

@Holzhaus Holzhaus force-pushed the qmldemo branch 2 times, most recently from a66efd6 to 059998f Compare May 28, 2021 16:30
@Holzhaus
Copy link
Member Author

I also added a pure-QML implementation of spinnies. Ready now.

@Holzhaus
Copy link
Member Author

I rebased on latest main. Ready to test and review. I do not intend to do further changes to this PR unless you find issues.

I'm running Qt 5.15, so it's possible that some QtQuick components are not present in Qt 5.12. Maybe someone can check that.

@Holzhaus
Copy link
Member Author

OK, apparently I mustn't use any QML modules with a version greater than 2.12 (for Qt 5.12). I'll check if and where I do that.

@Swiftb0y
Copy link
Member

OK, apparently I mustn't use any QML modules with a version greater than 2.12 (for Qt 5.12). I'll check if and where I do that.

Afaik QML Modules follow the Qt version unless they have not been changed. If they haven't been changed they stay on the version they last have been changed in. In general, we can only use QML modules with a version lower or equal than the minimum Qt version we support.

Copy link
Contributor

@uklotzde uklotzde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Swiftb0y Please merge when you think this is ready.

@Holzhaus
Copy link
Member Author

I downgraded all module versions to the Qt 5.12 versions.

@Swiftb0y
Copy link
Member

I'm building the branch right now to try it out. I'll merge if I don't notice any issues during testing.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise LGTM


group: root.group
key: "playposition"
onValueChanged: root.update()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a little bit worried how often this will get called. it would be ideal if it was just once per frame but can we guarantee that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll check if we can improve it by using a declarative binding that defines a relationship. I guess QML is then smart enough to figure it out itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your patience

@Swiftb0y Swiftb0y merged commit 4edb4a1 into mixxxdj:main May 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants