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

Skins: Add a <NumberDuration> control for skins #13176

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cr7pt0gr4ph7
Copy link

This PR adds a WNumberDurationcontrol for displaying a control value given in seconds as a formatted time (e.g. hh:mm:ss).
Although the WNumberPos has a similar purpose, it is tailored strictly for displaying the play position of a deck. The WTime control is also similar, but only displays the current time.

The goal is to be able to display e.g. the remaining duration of the Auto DJ queue in the UI in a skin-controllable way, like so:
Example screenshot

(Note: The above screenshot is from one of my development builds. Automatically calculating the remaining duration of the Auto DJ queue has its own gotchas, and so will be provided in a separate PR)

Although the WNumberPos has a similar purpose, it is tailored strictly for
displaying the play position of a deck. The WTime control is also similar,
but only displays the current time.
@github-actions github-actions bot added the ui label Apr 27, 2024
@cr7pt0gr4ph7 cr7pt0gr4ph7 changed the title Skins: Add a <NumberDuration> control Skins: Add a <NumberDuration> control for skins Apr 27, 2024
@github-actions github-actions bot added the build label Apr 27, 2024
Copy link
Contributor

@acolombier acolombier left a comment

Choose a reason for hiding this comment

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

That's looking good. Perhaps it could be nice to add this component in one skin?


class ControlProxy;

class WNumberDuration : public WNumber {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add some doc-string to document what this component is used for?

m_displayFormat = TrackTime::DisplayFormat::TRADITIONAL;
} else if (formatString == "traditional_coarse") {
m_displayFormat = TrackTime::DisplayFormat::TRADITIONAL_COARSE;
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to use SKIN_WARNING macro to alert of a potentially unsupported format. I understand that TimeFormat may be an arbitrary string passed to QTime::toString. Perhaps you could use two attributes (e.g TimeFormat is an enum, including custom and CustomFormat allow you to define the arbitrary string, stored in m_displayFormatString. This way, it will help debugging cases where user accidentally mistype a pre-constructed enum.

Copy link
Author

Choose a reason for hiding this comment

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

How about two properties PredefinedFormat (or StandardFormat/DefaultFormat) and CustomFormat)? And just use the one that is specified, and emit a warning if both are specified (and fallback to CustomFormat).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that sounds perfect!


fmt = t.toString(m_displayFormatString);
} else if (m_displayFormat == TrackTime::DisplayFormat::KILO_SECONDS) {
fmt = mixxx::Duration::formatKiloSeconds(dSecondsAbs);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps using a switch for the m_displayFormat might help with readability, although strictly equivalent?

@ronso0
Copy link
Member

ronso0 commented May 7, 2024

Tbh I'm reluctant adding a widget that is not used in official skins.
which duration control values can this be hhooked up to currently?

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.

None yet

3 participants