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.

Sign up for GitHub

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

Jump to bottom

show Passthrough cover on track overview #2616

Merged
merged 3 commits into from
Apr 15, 2020
Merged

show Passthrough cover on track overview #2616

merged 3 commits into from
Apr 15, 2020

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Apr 1, 2020 •
edited
Loading

image

Inspired by #2575

Show passthrough cover over entire overview, picks color from <PassthroughOverlayColor> node.
Stack Passthrough label on top, accepts custom styles via qss: WOverview #PassthroughLabel.
All mouse events are still passed to the overview.

I added the PassthroughOverlayColor picker to /waveform/renderers/waveformsignalcolors.cpp, I guess it can also be used for the scrolling waveform then?

  • add styles to all skins
  • add default color to C++?

@ronso0 ronso0 requested a review from Holzhaus April 3, 2020 15:45
@ronso0 ronso0 added this to the 2.3.0 milestone Apr 3, 2020
@ronso0
Copy link
Member Author

ronso0 commented Apr 5, 2020

It would be nice if someone colud review this to let me know if I'm on the right track.
If so, I'll continue to add the skin styles and a default color.

Holzhaus
Holzhaus reviewed Apr 5, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM so far, thanks!

@ronso0
Copy link
Member Author

ronso0 commented Apr 5, 2020

well, that was quick!

@ronso0
Copy link
Member Author

ronso0 commented Apr 5, 2020

Be-ing removed the beta non-blocker label

why? "passthrough" is displayed over the overview #2575 , this upgrade just keeps the overview usable and can be added during beta (if I don't finish the skin implementation before that)

@Be-ing
Copy link
Contributor

Be-ing commented Apr 5, 2020

"beta non-blocker" is a useless tag. Everything not tagged as a beta blocker is a non-blocker; having a tag for that is superfluous.

@ronso0
Copy link
Member Author

ronso0 commented Apr 5, 2020 •
edited
Loading

There were some embarassing copy/paste mistakes and a bug with skin reload that slipped through earlier, so I started over and rebased. Please review.
Some functions are also more effective now, for example the cover color is only read once during setup. I'll use that for the playedOverlayColor, too.

With default.qss it looks like this:
image

Custom skin styles coming soon..

Be-ing
Be-ing reviewed Apr 5, 2020
View reviewed changes
src/widget/woverview.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Apr 6, 2020

image
image

image
image
image

Tango with #2619 : no passthrough cover on Play, no highlight on Vinyl Controls toggle
image
image

image

@ronso0 ronso0 requested a review from Be-ing April 6, 2020 00:49
Holzhaus
Holzhaus requested changes Apr 6, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great! Code looks good.

Now for the obligatory bikeshedding:
I think we should try to re-use the skin's track title label text colors for the Passthrough label. Different colors look a bit out of place. For shade, this would be dark grey instead of green, and in Tango blue or Fuchsia depending on color scheme. I'm not sure about Deere, but I think either white or blue (the same shade that is used for buttons) would look good.

src/widget/woverview.h Outdated Show resolved Hide resolved
src/widget/woverview.cpp Outdated Show resolved Hide resolved
@ronso0
Copy link
Member Author

ronso0 commented Apr 6, 2020

Now for the obligatory bikeshedding:
I think we should try to re-use the skin's track title label text colors for the Passthrough label. Different colors look a bit out of place.

Here we go :)
I think we should emphasize the indicator nature of the Passthrough label because it's a different information level than a track title, so I chose scheme-specific indiciator/button colors*. IMO that helps to notice it, especially since the Passthrough toggles are either very small (Shade), can be hidden (Tango, Deere) or might be far away from the decks (Deere w/ stacked waveforms).
*only in LateNight this doesn't make difference

@Holzhaus Holzhaus marked this pull request as draft April 9, 2020 11:05
@Holzhaus
Copy link
Member

Holzhaus commented Apr 9, 2020

I converted this to draft to prevent premature merging because there are still things left to do (according to the checkboxes in your PR description). If this is ready, feel free to convert it back.

Holzhaus
Holzhaus approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Code LGTM so far (except for one variable name). Thank you.

src/widget/woverview.cpp Outdated Show resolved Hide resolved
@ronso0 ronso0 marked this pull request as ready for review April 9, 2020 14:45
@ronso0
Copy link
Member Author

ronso0 commented Apr 9, 2020

Thx, comments addressed, ToDos finished.
Ready for merge if the colors are reasonable.

@ronso0
Copy link
Member Author

ronso0 commented Apr 9, 2020

I converted this to draft

Ha! Now I found that 'button', too.

Holzhaus
Holzhaus approved these changes Apr 9, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Ready for merge if the colors are reasonable.

I still don't like the colors (except for LateNight), but this subjective and I don't want to start endless discussions about this. I you think they fit in nicely, so be it (I'm using LateNight anyway 😉).

Holzhaus
Holzhaus requested changes Apr 10, 2020
View reviewed changes
src/widget/woverview.cpp Outdated Show resolved Hide resolved
@ronso0
draw passthrough overlay on overview waveform
389bb23 
When vinyl passthrough is enabled, dimm waveform overviews with a
translucent overlay on top of all overview itemsgraphics and put
'Passthrough' QLabel on top.
Any mouse events are still passed to the overview widget.
Skin style:
The cover color is picked from skin node <PassthroughOverlayColor>
in the Overview widget (default #bb000000), the default label style
can be adjusted in qss (#PassthroughLabel).
@ronso0
Copy link
Member Author

ronso0 commented Apr 14, 2020

Fixed and squashed.
Ready for merge.

Holzhaus
Holzhaus reviewed Apr 15, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Thanks!

I retested. The font for Tango is incorrect on my machine:

2020-04-15-105714_747x385_scrot

Other than that, LGTM.

@ronso0
Copy link
Member Author

ronso0 commented Apr 15, 2020 •
edited
Loading

I retested. The font for Tango is incorrect on my machine:

Strange, here it looks as intended.
I added font-weight: bold; to all stylesheets to ensure it on all OS.
After further testing I made the label color in Shade Dark a bit brighter.
For the sake of completeness I also removed the Passthrough hints from Tango. (might create conflicts for #2619 though)
Ready!

Holzhaus
Holzhaus reviewed Apr 15, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

Still looks like this:

2020-04-15-115852_664x293_scrot

@ronso0
Copy link
Member Author

ronso0 commented Apr 15, 2020

This is deck1 with ClubTwist, right?
I have no idea why the font style is not applied, ClubTwist doesn't have a separate stylesheet.
Issue should be solved after merging #2647

@Holzhaus
Copy link
Member

This is deck1 with ClubTwist, right?
I have no idea why the font style is not applied, ClubTwist doesn't have a separate stylesheet.
Issue should be solved after merging #2647

I merged #2647 locally and the issue is indeed fixed.

Holzhaus
Holzhaus approved these changes Apr 15, 2020
View reviewed changes
Copy link
Member

@Holzhaus Holzhaus left a comment

Choose a reason for hiding this comment

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

LGTM.

@Holzhaus Holzhaus merged commit f0e8bf1 into mixxxdj:master Apr 15, 2020
@ronso0 ronso0 deleted the passthrough-cover branch April 15, 2020 13:09
@ronso0 ronso0 mentioned this pull request Dec 10, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Holzhaus Holzhaus Holzhaus approved these changes

@Be-ing Be-ing Awaiting requested review from Be-ing

Assignees
No one assigned
Labels
skins ui
Projects
None yet
Milestone
2.3.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ronso0 @Be-ing @Holzhaus