Enhancement: Make reset button grayed out if no changes are made or if it's already pressed once

[sepehr-rs]

Description

This PR fixes #19 by making the reset button grayed out if no changes are made or if it is already pressed once.

Type of Change

• [ ] Bug fix
• [x] New feature
• [ ] Breaking change
• [ ] Documentation update

Additional Notes

I don't know if we should merge this however, as the reset button does not reset all settings ( like BPM ). Also this approach would fail if the user manually resets the blocks one by one.

[Revisto]

Thanks for the PR, I think BPM shouldn't reset to default (120), but about this feature in general, I'm not sure if disabling the reset button in any case is a good idea. As of my personal experience, I even like pressing the reset button multiple times to make sure it's applied :D, would love to have your opinion on this.

[sepehr-rs]

Thanks for the PR, I think BPM shouldn't reset to default (120), but about this feature in general, I'm not sure if disabling the reset button in any case is a good idea. As of my personal experience, I even like pressing the reset button multiple times to make sure it's applied :D, would love to have your opinion on this.

I can see the appeal of pressing it a few times, kind of like double-checking the stove before leaving the house. Given that other apps follow the same pattern, I think this could be a solid addition.

As for the BPM, I don't understand why it shouldn't go back to the default. Could you please elaborate more on this?

[Revisto]

Hi, after doing some research, I understand how it's a good idea for the reset button to be disabled and for the BPM to be set back to default (120). The reset button looks good to me now. Could you please implement the BPM reset functionality as well?

I'm not sure what you mean here. Would you be able to elaborate?

Also this approach would fail if the user manually resets the blocks one by one.

[sepehr-rs]

Hi, after doing some research, I understand how it's a good idea for the reset button to be disabled and for the BPM to be set back to default (120). The reset button looks good to me now. Could you please implement the BPM reset functionality as well?

I'm not sure what you mean here. Would you be able to elaborate?

Also this approach would fail if the user manually resets the blocks one by one.

If the user toggles some buttons, the reset button is activated. However, if the user doesn't press that and instead resets each of the buttons back to their default state one by one, the reset button will still be active. this ideally shouldn't happen.

[Revisto]

Ah, I understand it now, it would be better to implement this. I think we need a method to check drum machine's state after each toggle press and bpm change, that method can check if any drum part has an active toggle, and also can check if the bpm is default or not.

[sepehr-rs]

Ah, I understand it now, it would be better to implement this. I think we need a method to check drum machine's state after each toggle press and bpm change, that method can check if any drum part has an active toggle, and also can check if the bpm is default or not.

Yeah, that should happen, I agree. Do you think this should be covered in another PR or should I do a commit here for it ?

[Revisto]

Let's commit it here and rename the PR title to a more general one.

[sepehr-rs]

Hi @Revisto, does this look good to you?

[Revisto]

Hi Sepehr,

There are some issues,

Architectural Issues:

• mark_unsaved_changes() shouldn't manage UI elements like the clear button. Use signals/observers instead to decouple the service from UI logic.

BPM Handling Issues:

• Hardcoded default: Replace hardcoded 120 with a config constant to keep it clean
• Verbose boolean: has_bpm_changed = True if value != 120 else False should be has_bpm_changed = value != 120

Clear Button Logic Issues:

• Clear button sensitivity should be based on whether there are patterns to clear, not unsaved changes

Thanks.

[sepehr-rs]

Hi Sepehr,

There are some issues,

Architectural Issues:

* `mark_unsaved_changes()` shouldn't manage UI elements like the clear button. Use signals/observers instead to decouple the service from UI logic.

BPM Handling Issues:

* **Hardcoded default**: Replace hardcoded `120` with a config constant to keep it clean

* **Verbose boolean**: `has_bpm_changed = True if value != 120 else False` should be `has_bpm_changed = value != 120`

Clear Button Logic Issues:

* Clear button sensitivity should be based on whether there are patterns to clear, not unsaved changes

Thanks.

Hi @Revisto, thanks a lot for reviewing my changes thoroughly. I’ll soon update the code to address these points.