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

[WIP] Tango skin :: skin settings bug fix & various enhancements #1259

Merged

daschuer merged 62 commits into mixxxdj:master from ronso0:Tango-keylock-sampler-fix

Jul 7, 2017

Member

ronso0 commented May 11, 2017 •

edited

Loading

changes introduced by this PR:

re-enabled 2.1 loop controls, redesigned loopsize spinbox
skin settings: select which deck controls are always visible
Keylock button is consistent with other deck controls: Off = hollow/grey, On = white
skin settings: symmetric time/duration display in left and right decks
moved FX assignment buttons to respective decks
moved compact mode toolbar below scrolling waveforms, it kind of sticks to decks now
increased spacing between main components
mixer bar stick to bottom when Library is hidden
separate Effect enable button from Effect selector
separate Meta link button from FX parameter name, add Meta link invert button

Bugs fixed:

removed font family definition for the Library (as reported in the forum, font set in Preferences was overridden by qss)
moved skin settings beside the main skin (due to visibility bug in OSX)
correct FX tooltips

ronso0 added 2 commits

May 11, 2017 18:12


          restyle keylock button

c6af345


          minor fixes: sampler buttons, sample row selection etc.

afdbd28

ronso0 mentioned this pull request

Tango skin #1151

Merged


          Library: don't override font family set in Preferences

e900b43

ronso0 changed the title ~~Tango keylock sampler fix~~ Tango skin :: keylock button, sampler and Library font fix


          make all deck indicators' visibility optional

02d825f

ronso0 changed the title ~~Tango skin :: keylock button, sampler and Library font fix~~ [WIP] Tango skin :: deck indicators, sampler and Library font fix


          add skin defaults for deck indicators

2b3482e

Contributor

esbrandt commented May 12, 2017

@ronso0
Just a head-up, please update the skins license terms.
Currently, you try to restrict the use for non-commercial purposes only.

With that license, the skin can not be shipped with Debian. Has to be DFSG compatible, see https://wiki.debian.org/DFSGLicenses

Read the license terms from LateNight and Deere (the skins you build upon), https://creativecommons.org/licenses/by-sa/3.0/

ShareAlike — If you remix, transform, or build upon the material, you must distribute your contributions under the same license as the original.

Additionally, we might consider to relicense the skins under https://creativecommons.org/licenses/by-sa/4.0/ , which has been published since.


          update license terms

31fd765

Member Author

ronso0 commented May 12, 2017

Just a head-up, please update the skins license terms.

Done. Thanks for the hint!

ronso0 added 2 commits

May 12, 2017 17:05


          fix right deck CF orientation button

164db0a


          fix right deck CF orientation button II

165cfc8

Contributor

Be-ing commented May 12, 2017

Is there a use case for spinnies without cover art? How about just offering a choice between spinny with cover art, cover art, or none?

Contributor

Be-ing commented May 12, 2017 •

edited

Loading

How about renaming the "Stacked Overviews" option to "Symmetrical decks"?

Contributor

Be-ing commented May 12, 2017

Other than the waveform background colors, I feel that this is looking really good. I'll try playing with different colors.

I got confused by the "1 2 3 4" icon. I thought it would toggle 2/4 decks. I don't think the hotcue 4/8 and spinny/cover option buttons need to be front and center. They could just be in the settings menu.

Contributor

Be-ing commented May 12, 2017 •

edited

Loading

I still think the effect unit assignment buttons should be in the decks.

I have hidden superknobs from the default view in Deere. In usability testing people have been confused by them and they're not very useful. People were also confused by the effect unit enable switch, so I re-removed that. Looking into that again, I found only one controller, the Kontrol S4, that has a chain on/off switch (none of Native Instruments' other controllers have a button for that).

Contributor

Be-ing commented May 13, 2017

Is there no cue button?

Contributor

Be-ing commented May 13, 2017

The spinny/cover art overlay the right side of the overviews for the left decks. On the right, the overview resizes appropriately for the narrower space.

Contributor

Be-ing commented May 13, 2017

There are no buttons for vinyl control options.

Member Author

ronso0 commented May 13, 2017

Is there a use case for spinnies without cover art?

Moving indicator for play state without Cover distraction
For unmotorized controller platters it indicates the play position which is helpful when you hold a platter to scratch or to drop the record. I watched Traktor Scratch DJs with DVS not caring about CoverArt but using the Spinny.

Member Author

ronso0 commented May 13, 2017 •

edited

Loading

Is there no cue button?

Check tooltips of Play button: it's mapped to right click
Left: play/pause
Right: cue, depending on Cue Preferences

Anyone else having difficulties with this?

Member Author

ronso0 commented May 13, 2017 •

edited

Loading

The spinny/cover art overlay the right side of the overviews for the left decks. On the right, the overview resizes appropriately for the narrower space.

During normal use I cannot confirm this.
A few times I observed that Overview didn't get refreshed properly when quickly toggling Spinny or Vinyl options On & Off & On & Off while track is paused, then this overlap happens. The issue was gone as soon as I toggled Play or clicked somewhere on the Overview.
This also happened a few times with SizeAwareStacks in developer mode.
So I think this is due to some bug in skin system.

Member Author

ronso0 commented May 13, 2017 •

edited

Loading

There are no buttons for vinyl control options.

View > Show Vinyl Control Section (default setting in Tango is ON already)
Then you'll find a toggler at the inner side of overviews, which allows you to toggle Vinyl Controls for each deck independently.

Member Author

ronso0 commented May 13, 2017

How about renaming the "Stacked Overviews" option to "Symmetrical decks"?

Thought about that. Good idea!

Member Author

ronso0 commented May 13, 2017

I got confused by the "1 2 3 4" icon. I thought it would toggle 2/4 decks. I don't think the hotcue 4/8 and spinny/cover option buttons need to be front and center. They could just be in the settings menu.

So you discovered a new icon and were curious. Then you learned what the button does. Great!
About HotCue and Spinny/Cover togglers we had a discussion in #1511, I thought I layed out the respective use cases there.
So, I think the HotCue toggler should stay where it is. About the Spinny toggler I'm not sure, but I'd rather keep it instead of resizing the other toggler icons in top row to rather odd proportions. Or do you have any other control in mind that could replace the Spinny toggler?

Anyone else having difficulties with this?

Member Author

ronso0 commented May 13, 2017

How about renaming the "Stacked Overviews" option to "Symmetrical decks"?

Thought about that. Good idea!

This is where @nikmartin's idea to display track duration/elapsed time at the inner side of decks comes in. Will think about how to implement this.

Member Author

ronso0 commented May 13, 2017 •

edited

Loading

@Be-ing Your opinion on the actual purpose of this PR?

It turned out that feature discussion on Tango continues in #1151, which is good since everything stays in one place.

Member Author

ronso0 commented May 13, 2017

I still think the effect unit assignment buttons should be in the decks.

And I somehow still don't like to detach this effect unit specific control from effect units.
For the case that effect units are hidden I understand this approach, as well as for samplers to keep effect units clean.

Making effect assignment fully consistent would also mean to move buttons for Master/Headphone to mixer bar, so you'd need to enable mixer bar only to access effect assignment for Pfl.
I agree on making skins consistent, but regarding Tango as controller extension, I feel this would make things more complicated than necessary and contradicts the space saving strategy.


          redesign h-size detectors (netbook & stars)

a7a0d12

Member Author

ronso0 commented Jun 29, 2017

I moved 4/8 HotCues option to skin settings. Please test!
@daschuer As long no one discovers any critical bugs, I think this is ready to merge.
All remaining issues will be discussed in another PR.

esbrandt reviewed

View reviewed changes

Contributor

esbrandt left a comment

Thanks for the latest changes.

Some additional comments

The updated keylock icon in the decks look nearly invisible to me in inactive state. Also the icon differs from the one used for samplers, intentionally?
In skin settings widget, change string Load/Save Sample Banks--> Load/Save Sampler Banks. Thats what the Save modal file-dialog says when saving banks.

res/skins/Tango/skin_settings.xml

+                              <SetVariable name="Setting">[Tango],symmetric_overviews</SetVariable>
+                            </Template>
+                            <Template src="skin:skin_settings_button_2state.xml">

Contributor

esbrandt Jun 29, 2017

Duplicate line Symmetric Time/Duration

Member Author

ronso0 Jun 29, 2017

fixed

res/skins/Tango/skin_settings.xml Outdated

-                              <SetVariable name="state_0_text">      Star Rating</SetVariable>
-                              <SetVariable name="state_1_text"> &#10004; Star Rating</SetVariable>
-                              <SetVariable name="Setting">[Tango],stars</SetVariable>
+                              <SetVariable name="state_0_text">      4 &#9664;  8 Hotcues</SetVariable>

Contributor

esbrandt Jun 29, 2017

Using ◀ and ▶ to indicate a changed button state is unusual and pretty abstract. Not particular bad, the settings menu should be as simple and clear as possible though.
What about just 4 Hotcues and 8 Hotcues?

Member Author

ronso0 Jun 29, 2017 •

edited

Loading

The idea was to design it like a physical flip switch: both states are clear no matter in which state it currently is.
Do you insist on changing it to 4 Hotcues and 8 Hotcues?

res/skins/Tango/skin_settings.xml Outdated

@@ @@ -281,8 +453,8 @@ Description: @@
                           <Children>
                             <Template src="skin:skin_settings_button_2state.xml">
-                              <SetVariable name="state_0_text">      2 &#9668; 4  Effect Units</SetVariable>
-                              <SetVariable name="state_1_text">      2 &#9658; 4  Effect Units</SetVariable>
+                              <SetVariable name="state_0_text">      2 &#9664;  4  Effect Units</SetVariable>

Contributor

esbrandt Jun 29, 2017

Another abstract button state indicator, see comment above

res/skins/Tango/skin_settings.xml Outdated

+                                    <Template src="skin:button_2state_persist.xml">
+                                      <SetVariable name="ObjectName">DeckButtonVisibility</SetVariable>
+                                      <SetVariable name="Size">15f,15f</SetVariable>
+                                      <SetVariable name="TooltipId">orientation</SetVariable>

Contributor

esbrandt Jun 29, 2017

TooltipID wrong

Member Author

ronso0 Jun 29, 2017

Thanks. I'll remove all tooltips from this section in skin settings since button labels are quite explicit.

res/skins/Tango/skin_settings.xml Outdated

+                                    <Template src="skin:button_2state_persist.xml">
+                                      <SetVariable name="ObjectName">DeckButtonVisibility</SetVariable>
+                                      <SetVariable name="Size">16f,15f</SetVariable>
+                                      <SetVariable name="TooltipId">orientation</SetVariable>

Contributor

esbrandt Jun 29, 2017

TooltipID wrong

res/skins/Tango/skin_settings.xml Outdated

+                                  <Children>
+                                    <Template src="skin:skin_settings_button_2state.xml">
+                                      <SetVariable name="state_0_text">      ° &#9664;  O</SetVariable>
+                                      <SetVariable name="state_1_text">      °  &#9654; O  [ hide Channel mixer! ]</SetVariable>

Contributor

esbrandt Jun 29, 2017

Another abstract button state here ;)

Hard to gasp what to do when O [ hide Channel mixer! ] appears.
What are the considerations behind this design?

Member Author

ronso0 Jun 29, 2017

The general design in Tango is to only show big cover/spinny if channel mixer is collapsed.
@Be-ing pointed out that this option in skin settings (small or big spinny) wasn't clear if channel mixer is expanded.
I could make the label more explicit: Hide channel mixer to choose big Cover or Spinny
Do you consider the options ° and O clear enough?

Member Author

ronso0 commented Jun 29, 2017

The updated keylock icon in the decks look nearly invisible to me in inactive state. Also the icon differs from the one used for samplers, intentionally?

Thanks, didn't notice that on my screen. I'll see what I can do and fix this for both samplers and decks.

In skin settings widget, change string Load/Save Sample Banks--> Load/Save Sampler Banks. Thats what the Save modal file-dialog says when saving banks

Fixed.


          Skin settings: fixing tooltips and duplicate button

8cd65c2

Member Author

ronso0 commented Jun 29, 2017

@esbrandt
The off state of keylock button could look like this

but I'm curious if this would work as well

I like the second icon much more, also it's similiar to Broadcast failure indicator.

Contributor

esbrandt commented Jun 30, 2017

Do you insist on changing it to 4 Hotcues and 8 Hotcues?

No. My point is that these kind of indicators might not as clear to users of the skins, as they are to the developer.

Contributor

esbrandt commented Jun 30, 2017

I like the second icon much more, also it's similiar to Broadcast failure indicator.

You come really up with some neat ideas :)
But again, i´m in favor of keeping it simple and unambiguous. What about just using the default keylock (similiar samplers), and use a touch of color for the active state. All the other buttons in the row have it anyway.

Contributor

esbrandt commented Jun 30, 2017

The general design in Tango is to only show big cover/spinny if channel mixer is collapsed.
@Be-ing pointed out that this option in skin settings (small or big spinny) wasn't clear if channel mixer is expanded.

That´s my impression too.

I could make the label more explicit: Hide channel mixer to choose big Cover or Spinny

Could work, but the string might be to long for the width of the settings widget. A pity we can not translate these kind of label.

Do you consider the options ° and O clear enough?

Not really. I got it, admittedly not at the first attempt :)

Last things:

White peak indicator if VU is clipping, intentionally?
Asking because it is usually red elsewhere.
Something is not right with the spacing in AutoDJ view.

Member Author

ronso0 commented Jun 30, 2017

But again, i´m in favor of keeping it simple and unambiguous. What about just using the default keylock (similiar samplers), and use a touch of color for the active state. All the other buttons in the row have it anyway.

Alright. We discussed that issue extensively so far and I was resisting yet another indicator color, but now that decks' font colours are harmonized it could work.
There will come some changes to colors anyway since @Be-ing discovered an accessibilty issue for (partially) color-blind users in all skins, mainly red-ish indicators and knobs. But this will be handled in another PR, probably for all skins at once..

Could work, but the string might be to long for the width of the settings widget

I thought about making it a two-liner. And I'll find a way to make those flip switches more clear.

White peak indicator if VU is clipping, intentionally?

Yes! A year ago, I had an outdoor gig and due to the brightness there was no way to distinguish green/yellow from red since it's darker. White stands out much more.
So it's a feature -_o

Something is not right with the spacing in AutoDJ view

I'll have a look. Are you on Windows? On Linux all buttons are rendered perfectly, even with expanded side pane. Also, it seems button labels are correctly using Ubuntu font but another font weight than 'normal'..

Contributor

Be-ing commented Jun 30, 2017 •

edited

Loading

I'm also in favor of switching to a lock icon instead of a music note for keylock.

Yes! A year ago, I had an outdoor gig and due to the brightness there was no way to distinguish green/yellow from red since it's darker. White stands out much more.
So it's a feature -_o

I understand that makes it easier to see. On the other hand, using red to indicate clipping is a strong convention across all audio gear. Some users may not even be familiar with the term "clipping" or understand what it actually means and only know it as "going in the red" or "redlining". Maybe you could use a brighter red than you did initially.

it seems button labels are correctly using Ubuntu font but another font weight than 'normal'..

I'm not sure the library buttons are WPushButton widgets. I'm not sure what QSS selector to use to style them.

Contributor

nikmartin commented Jun 30, 2017

As a close follower of the Tango skin since being merged into master are the changes going on in this PR significant enough to start building mixx with this PR merged in, or is building from master good enough now?

Member Author

ronso0 commented Jun 30, 2017

... and only know it as "going in the red" or "redlining". Maybe you could use a brighter red than you did initially.

Bright magenta could work, like font colour of deck 3/4.

Member Author

ronso0 commented Jun 30, 2017

@nikmartin
You can use pre-build master and just link or copy /res/skins/Tango to resource directory. Ubuntu font is merged already.

Member Author

ronso0 commented Jun 30, 2017 •

edited

Loading

I'm not sure the library buttons are WPushButton widgets. I'm not sure what QSS selector to use to style them.

I'll try how it is done in Deere https://github.com/mixxxdj/mixxx/blob/master/res/skins/Deere/style.qss#L490
#DlgAutoDJ > QPushButton#pushButtonAutoDJ
...#pushButtonFadeNow
...#pushButtonSkipNext etc from /src/library/x/y.ui

Member Author

ronso0 commented Jun 30, 2017

I'm also in favor of switching to a lock icon instead of a music note for keylock.

Most of the discussion about making the interface intuitive we had was implicitly dominated by first time users or beginners, meaning: about clear icons that can be read with little or no experience in DJ software. IMO the lock icon (like in LateNight) only communicates something is locked (deck? tempo?), whereas the note icon is clearly tone related.

Contributor

esbrandt commented Jun 30, 2017 •

edited

Loading

I'm not sure the library buttons are WPushButton widgets. I'm not sure what QSS selector to use to style them.
I'll try how it is done in Deere https://github.com/mixxxdj/mixxx/blob/master/res/skins/Deere/style.qss#L490

Uhm, just noticed this is an issue in the other skins too.
Regression probably introduced with #883

Edit: Using macOS 10.11.6

Member Author

ronso0 commented Jul 3, 2017

@esbrandt
How do the button layouts look like in Analysis & Recording feature on OSX?

Contributor

foss- commented Jul 5, 2017

Sorry to ask, but what's preventing this PR from getting merged? I thought the assumption was to discuss further tweaks in new PRs.

Member Author

ronso0 commented Jul 5, 2017 •

edited

Loading

@foss-
I will still fix the keylock icon and clipping indicator this week, then it can be merged.

ronso0 added 2 commits

July 6, 2017 15:06


          fix keylock icon, clipping indicator, sampler volume tootlip

64fed86


          show a hint when big cover/spinny is not shown due to channel mixer

c8f9749

Member Author

ronso0 commented Jul 6, 2017 •

edited

Loading

Done.
I built in a hint to clearify that big cover/spinny is visible only when Channel Mixer is hidden.
That's the only thing left to discuss here, I guess: is the small hint next to that toggle sufficient or do we need the text in Spinny/Cover section as well?
And I used 'channel_volume' tooltipID for samplers' volume knob since there's no 'sampler_volume' tooltip..

Contributor

esbrandt commented Jul 6, 2017

That's the only thing left to discuss here, I guess: is the small hint next to that toggle sufficient or do we need the text in Spinny/Cover section as well?

Thats better.
Lets fix other things that come up eventually in another PR.

@ronso0
PR ready to merge now?

Member Author

ronso0 commented Jul 7, 2017

PR ready to merge now?

Yeah!!

Member

daschuer commented Jul 7, 2017

Yeah! Thank you veeerry much!

daschuer merged commit f0188da into mixxxdj:master

Member Author

ronso0 commented Jul 7, 2017

Thanks!! Nice to see things evolve since my first LateNight hack in the forums..

ronso0 deleted the Tango-keylock-sampler-fix branch

July 7, 2017 17:20

ronso0 mentioned this pull request

LateNight 2.1 :: fine tuning, 4 FX units, more samplers, skin menu #1489

Merged

17 tasks

mixxxbot mentioned this pull request

tango skin: cannot access hamburger menu options on macOS #8859

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet