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

Always show control points in timing mode #29196

Closed
wants to merge 2 commits into from

Conversation

kadambishreyas
Copy link
Contributor

i was trying to time a piano-only beatmap yesterday and got very frustrated since i couldn't tell what bpm i was on in the timing panel... until i realized it was disabled, which for some reason also means that it's invisible in the panel specifically meant for timing.

maybe the top menu text should also change to Always show timing changes to indicate that timing changes may be shown forcibly at times.

thanks in advance for review, still very unfamiliar with codebase.

Comment on lines +85 to +86
private Bindable<bool> alwaysShowControlPoints;
private readonly Bindable<bool> controlPointsVisible = new Bindable<bool>();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think you need this new bindable, and can revert basically all your changes.

In the original controlPointsVisible.BindValueChanged you can just check the current editor mode and react there. Although one may argue this is too much local knowledge and the editor should be setting the value on the timeline, not vice-versa.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i'm unclear on how this would work, do you mind explaining to me why this is the case?
suppose we have the case of:

actions    | 1. start with config hidden -> 2. enable config -> 3. change to timing mode -> 4. disable config -> 5. go to compose mode
visibility | 1. hidden -> 2. shown -> 3. shown -> 4. shown -> 5. hidden
height     | 1. normal -> 2. expanded -> 3. expanded -> 4. normal -> 5. normal

if we don't create a bindable on mode state, at what point can code trigger at action (5) to correctly hide the config? is there a lifecycle thing here i'm missing..?

Copy link
Member

Choose a reason for hiding this comment

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

Check #29211, I've redone this feature in a slightly simpler way.

private Bindable<bool> ticksVisible;

private double trackLengthForZoom;

private readonly IBindable<Track> track = new Bindable<Track>();

[BackgroundDependencyLoader]
private void load(IBindable<WorkingBeatmap> beatmap, OsuColour colours, OsuConfigManager config)
private void load(IBindable<WorkingBeatmap> beatmap, OsuColour colours, OsuConfigManager config, Editor editor)
Copy link
Member

Choose a reason for hiding this comment

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

Editor has to be nullable for some tests to pass, as can be seen from CI failures.

@bdach
Copy link
Collaborator

bdach commented Jul 31, 2024

Superseded by #29211

@bdach bdach closed this Jul 31, 2024
CloneWith pushed a commit to CloneWith/osu that referenced this pull request Aug 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants