Skip to content

Deere :: add Key/BPM expansion indicators, put Key controls next to play position#1880

Merged
rryan merged 2 commits intomixxxdj:2.2from
ronso0:deere-beatgrid-key-controls-down
Nov 3, 2018
Merged

Deere :: add Key/BPM expansion indicators, put Key controls next to play position#1880
rryan merged 2 commits intomixxxdj:2.2from
ronso0:deere-beatgrid-key-controls-down

Conversation

@ronso0
Copy link
Copy Markdown
Member

@ronso0 ronso0 commented Nov 1, 2018

https://bugs.launchpad.net/mixxx/+bug/1800705

Either beatgrid or key controls can be expanded. They replace Stars and play position then.
I choose to expand downwards as horizontal expansion creates ugly situations i.e. if the track cover or the mixer incl. Kill buttons is visible. For the same reason Stars and play position are hidden. The preparation use case justifies hiding those IMO.

deere-bpmkey-expansion_0
deere-bpmkey-expansion_1
deere-bpmkey-expansion_2

As soon as this would be merged I'll compact the SVGs.

@benis
Copy link
Copy Markdown
Contributor

benis commented Nov 1, 2018

I actually prefer to leave both open, and in particular to have the key controls open permanently. Expanding downward does make more sense though I think.

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 2, 2018

Okay. Nevertheless, as soon as we add even small expansion indicators we don't have enough space with a standart mixer on small screens.
For me this raises the question if we should stick to 1024px as Deere's minimal size or if we should introduce a 'recommended screen size' (1280px) while Deere would still fit 1024px screens but user would have to juggle with the components they need from case to case (Prep scenario vs. Live scenario)

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 2, 2018

Other possibilities:
simply add expansion indicators, expand left
(needs ~1280px w/ mixer visible before key/bpm controls get squeezed)
deere-bpmkey-expansion_v1

move Key controls next to play position, expand left
(Key controls should replace position display on small screens)
deere-bpmkey-expansion_v2
deere-bpmkey-expansion_v2_closed

@benis
Copy link
Copy Markdown
Contributor

benis commented Nov 2, 2018

The stacked key and beatgrid controls looks good. But even if you go with the original it's not gonna ruin Mixxx for me or anything. It would be nice to have track time still visible but I appreciate it's difficult to work with limited space.

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 3, 2018

@beenisss @Be-ing What do you think?
I'd like this fix to be added to 2.2.0, and the easist way seems to be this:
Key controls are moved down, both Key & BPM controls can easily be visible at the same time, and nothing needs to be hidden to still have a nice UX
deere-bpmkey-expansion_v2
deere-bpmkey-expansion_v2_closed

That's a polished version from another branch ronso/deere-beatgrid-key-controls-2rows2. If you go along I'd force-push it into this branch here.

@benis
Copy link
Copy Markdown
Contributor

benis commented Nov 3, 2018

I like it. Thumbs up from me.

@rryan
Copy link
Copy Markdown
Member

rryan commented Nov 3, 2018

Do the waveforms resize when you expand/collapse the key/bpm sections? They do for me on macOS.

@benis
Copy link
Copy Markdown
Contributor

benis commented Nov 3, 2018

I did notice some minor glitches when I tried it out. I haven't tested it properly, but let me know if I should.

@ronso0 ronso0 force-pushed the deere-beatgrid-key-controls-down branch from 4d973e9 to 63c734a Compare November 3, 2018 20:09
@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 3, 2018

I'd like this fix to be added to 2.2.0, and the easist way seems to be this:
Key controls are moved down, both Key & BPM controls can easily be visible at the same time, and nothing needs to be hidden to still have a nice UX
deere-bpmkey-expansion_v2
deere-bpmkey-expansion_v2_closed

I picked this commit so it's easier to test. Layout is fixed (no glitches).
Expanding downwards (and only one controls row at a time) is probably too radical to squeeze it into 2.2. Sorry for the confusion..

@rryan
Copy link
Copy Markdown
Member

rryan commented Nov 3, 2018

It works well for me now (horizontal expansion) and as you say helps to have them on separate rows, especially when at Deere's minimum size. 👍

The "MATCH" button text is cut off, but that's true for me without this PR too.
screen shot 2018-11-03 at 1 22 55 pm

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Nov 3, 2018

LGTM so far, however there are some issues, not prevent merging for me:

  • If both expanded they are squashed to unreadable if you open the skin preferences
  • If you enable both times, the first is truncated from the left to unusable. I think it should be hidden in this case.
  • It is odd that the "<" moves away under the mouse. I think the cent display should go the the left.
  • the time is truncated too early:
    image

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 3, 2018

The "MATCH" button text is cut off, but that's true for me without this PR too.

@rryan
Your screenshot shows OpenSans is not used.. this was noticed in #1867 (comment) too

@daschuer
There are plenty of issues at minimal size with the skin settings...
I'll continue to fix those, but for now this is already a huge improvement IMO
The only simple way to fix this is to force-open the key controls at full width:
deere-bpmkey-expansion_v2__squeezefix

It is odd that the "<" moves away under the mouse.

The border and the highlight indicate that you don't need to hit that small arrow but the whole container is responsive. You think that doesn't suffice?

@ronso0 ronso0 changed the title Deere :: add Key/BPM expansion indicators, expand controls downwards Deere :: add Key/BPM expansion indicators, put Key controls next to play position Nov 3, 2018
@daschuer
Copy link
Copy Markdown
Member

daschuer commented Nov 3, 2018

I know, "perfect" is here probably not worth the effort. So we can IMHO merge it when you think it is ready.

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 3, 2018

Yup, ready to go!

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Nov 3, 2018

@beenisss @rryan merge?

@rryan
Copy link
Copy Markdown
Member

rryan commented Nov 3, 2018

SGTM

@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Nov 3, 2018

👍

@rryan rryan merged commit ca9e01b into mixxxdj:2.2 Nov 3, 2018
@ronso0 ronso0 deleted the deere-beatgrid-key-controls-down branch November 3, 2018 21:19
@Be-ing
Copy link
Copy Markdown
Contributor

Be-ing commented Nov 25, 2018

I have noticed a drawback of this design. The strings for different keys can be different widths, which can make the key adjustment buttons move from under the cursor when clicking them repeatedly.

@benis
Copy link
Copy Markdown
Contributor

benis commented Nov 25, 2018

I've noticed that too, it usually catches me out if I double-click to drop down by two semitones and the second click just cancels out the first.

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 26, 2018

yeah, we had that issue before.
we can fix the key display's size, either to a medium width where short strings will be centered and long strings will be cut off, or adapt to the widest possible strings.
I'd prefer the 'medium solution' as it would look okay with Lancelot and Openkey notation, as well:

The tricky part:

  • <Elide> should be set "right" and only applied to key
  • cents should always be visible, they range from "0" to "+NN"

My attempt to solve this:
https://github.com/ronso0/mixxx/tree/key-display-tweak
94fed90 tweaks WKey widget to show cents only (if enabled) if <DisplayKey> is false.
ef6eab4 implements the tweak in Deere.
It works fine AFAICT: Cents expand if necessary, key uses the remaining space.
traditional
deere-key-trad

Openkey
deere-key-open

What do you think? Any other idea how to fix this?

@ronso0
Copy link
Copy Markdown
Member Author

ronso0 commented Nov 29, 2018

proposed changes are in #1925

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants