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

gradually "compact" the markers if the waveform height is reduced #12501

Merged
merged 3 commits into from
Jan 3, 2024

Conversation

m0dB
Copy link
Contributor

@m0dB m0dB commented Jan 2, 2024

To address this issue: #12273 (comment)

Screenshot 2024-01-03 at 00 11 34 Screenshot 2024-01-03 at 00 11 56 Screenshot 2024-01-03 at 00 12 03 Screenshot 2024-01-03 at 00 12 09

@m0dB m0dB mentioned this pull request Jan 2, 2024
@JoergAtGithub
Copy link
Member

In the last case you can't click on the marker anymore. A small section of should be always visible.

@m0dB m0dB requested a review from fwcd January 2, 2024 23:21
@fwcd
Copy link
Member

fwcd commented Jan 2, 2024

In the last case you can't click on the marker anymore. A small section of should be always visible.

Sounds reasonable, though from what I can tell, the markers in the "main" waveform aren't clickable, only the preview ones are (or am I missing something?)

@JoergAtGithub
Copy link
Member

In the last case you can't click on the marker anymore. A small section of should be always visible.

Sounds reasonable, though from what I can tell, the markers in the "main" waveform aren't clickable, only the preview ones are (or am I missing something?)

Look at this example, a track from a DJ download pool prepared with Serato Hotcues and Saved Loops, which you can't access anywhere else in the Mixxx UI, because Mixxx skins have only 8 Hotcue-Buttons:

TrackWithSeratoHotcues.mp4

@fwcd
Copy link
Member

fwcd commented Jan 2, 2024

Ah nice, TIL, didn't know this worked differently for hotcues. Though I think it's reasonable to prioritize glanceability over editability for the compact layout, anyone interested in editing several hotcues would probably work with a larger view anyway.

@fwcd fwcd added the waveform label Jan 2, 2024
@ronso0 ronso0 added this to the 2.4.0 milestone Jan 3, 2024
@fwcd
Copy link
Member

fwcd commented Jan 3, 2024

Did some quick testing, looks and works great!

@m0dB
Copy link
Contributor Author

m0dB commented Jan 3, 2024

Great! Would you adjust the threshold or do you think it's fine link this? (I personally think it could kick in a little bit sooner)

I disagree with @JoergAtGithub's assessment that "a small section of should be always visible", and IIUC @fwcd also thinks this isn't necessary. Indeed, if you want make the waveforms small like this, you probably aren't particularly interested in the markers at that point. But my opinion on this is not a very strong one, so maybe after merging we can ask what people think and adjust it if a majority prefers to keep a small section visible.

@JoergAtGithub
Copy link
Member

Let's discuss this on Zulip, to get more opinions

@m0dB
Copy link
Contributor Author

m0dB commented Jan 3, 2024

I just tried it out with a minimum increment of 2 and actually I like it better! It's just 2 pixels but in exchange you know that there is more than one marker there.

@fwcd
Copy link
Member

fwcd commented Jan 3, 2024

I think leaving a small section visible is a fine compromise, it hints at the user that there's more content behind the marker even if it's not shown. If the user is interested, they can expand the view as needed.

(I just typed out the same thing, what a coincidence haha)

@m0dB m0dB marked this pull request as ready for review January 3, 2024 22:52
@m0dB
Copy link
Contributor Author

m0dB commented Jan 3, 2024

Done!

@fwcd fwcd added the polish label Jan 3, 2024
@fwcd
Copy link
Member

fwcd commented Jan 3, 2024

Thanks! Since this is a rather small, but useful UI improvement over the status quo, and the concerns by @JoergAtGithub have been addressed, I'll go ahead and merge it. Tweaking the thresholds or making them configurable could be done in a future PR, if desired, but this is a good default for now.

@fwcd fwcd merged commit ee1263b into mixxxdj:2.4 Jan 3, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants