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 demo skin additions #3992

Merged
merged 19 commits into from
Jun 15, 2021
Merged

Conversation

Holzhaus
Copy link
Member

Some improvements for the demo skin, that brings it closer to being usable:

  • Add support for knob arcs not starting at center
  • Add sampler row
  • Add preliminary effect sections (not expandable and the comboboxes don't work yet)
  • Add microphone/aux units to crossfader row
  • Improve section backgrounds (borders between mixer/decks/etc)
  • Return transparent color if track color is not set (otherwise QML will silently convert and invalid QColor to black, indistinguishable from actual blackl

@github-actions github-actions bot added the skins label Jun 13, 2021
@Swiftb0y Swiftb0y self-assigned this Jun 14, 2021
@ronso0
Copy link
Member

ronso0 commented Jun 14, 2021

Wooo, this gets nicer and nicer!

Though, I have an issue with the sliders: single-click to set the position interferes with double-click to reset because clicks are processed separately, which leads to double click first setting the new value, then resetting the knob.

@Holzhaus Holzhaus force-pushed the qml-demo-skin-additions branch 2 times, most recently from f6da4f8 to 9ab6532 Compare June 14, 2021 20:19
@Holzhaus
Copy link
Member Author

I improved the mic/aux units and added ducking settings. This is ready now.

@Holzhaus Holzhaus marked this pull request as ready for review June 14, 2021 20:50
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.

Thanks for your continued hard work on this!

horizontalTileMode: BorderImage.Stretch
verticalTileMode: BorderImage.Stretch
source: Theme.imgSectionBackground

Copy link
Member

Choose a reason for hiding this comment

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

would it work to set anchor.fill: parent here? it would make sense since that is the most common usecase

Copy link
Member Author

Choose a reason for hiding this comment

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

It is, but if you don't want to use anchor.fill, defining it in the component makes it weird to use. Use need to explicitly set it to undefined then. I think it's the "callers" responsibility to define the size.

Copy link
Member

Choose a reason for hiding this comment

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

Mhmm right...

res/skins/QMLDemo/EffectUnit.qml Outdated Show resolved Hide resolved
res/skins/QMLDemo/EmbeddedBackground.qml Show resolved Hide resolved
res/skins/QMLDemo/Theme/Theme.qml Show resolved Hide resolved
res/skins/QMLDemo/Sampler.qml Outdated Show resolved Hide resolved
res/skins/QMLDemo/DeckInfoBar.qml Outdated Show resolved Hide resolved
height: parent.height
width: 56

Skin.InfoBarButton {
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 why this happens, but clicking this button, pressing cancel in the "no input device selected"-popup and then pressing anywhere else activates the button again. I don't know whether we can do much about that or whether its our responsibility to do that. Same issue also occurs with the passthrough buttons of the decks.

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 idea, probably because the button didn't receive the release signal or something? We can try to debug that separately.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we should document it on the project board though.

res/skins/QMLDemo/MicrophoneUnit.qml Outdated Show resolved Hide resolved
res/skins/QMLDemo/MiniKnob.qml Outdated Show resolved Hide resolved
res/skins/QMLDemo/Mixxx/Controls/Knob.qml Outdated Show resolved Hide resolved
Comment on lines +137 to +160
Mixxx.ControlProxy {
id: playControl

readonly property bool playing: value != 0

function stop() {
value = 0;
}

group: root.group
key: "play"
}

Mixxx.ControlProxy {
id: ejectControl

function trigger() {
value = 1;
value = 0;
}

group: root.group
key: "eject"
}
Copy link
Member

Choose a reason for hiding this comment

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

IMO it would make sense that once we make COs "richer", we should add them to the built-in components we already ship.

Copy link
Member Author

Choose a reason for hiding this comment

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

True, we can consider that once we have a pretty complete, polished skin. Right now, we're refactoring all the time so I think it's too early to extract stuff like this.

Copy link
Member

Choose a reason for hiding this comment

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

I agree. We just shouldn't forget to go back and actually do that once we're at the appropriate stage.

Comment on lines +154 to +155
value = 1;
value = 0;
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 not sure, but I think setting the value twice is unnecessary. Whats important is just that the signal for the change in value gets emitted. Even if the value wasn't actually changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

But it kind of seems hacky to me tbh. Maybe the cpp ControlProxy object should have a trigger method to emit a value change signal without changing anything.

Copy link
Member

Choose a reason for hiding this comment

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

Yes. The CO interface is a bit strange for that though. IIRC you can configure a CO to swallow such signals when the value doesn't change as well. I guess this was a design decision made driven by the old Skin system...

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.

LGTM. Thank you very much

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

Successfully merging this pull request may close these issues.

3 participants