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

Add enum for modes of locking buttons #1955

Merged
merged 3 commits into from
Jul 24, 2024
Merged

Add enum for modes of locking buttons #1955

merged 3 commits into from
Jul 24, 2024

Conversation

ia
Copy link
Collaborator

@ia ia commented Jul 24, 2024

  • Please check if the PR fulfills these requirements
  • The changes have been tested locally
  • There are no breaking changes
  • What kind of change does this PR introduce?
    Add enum for modes of locking buttons in Settings.

  • What is the current behavior?
    Constants are used for SettingsOptions::LockingMode.

  • What is the new behavior (if this is a feature change)?
    Use self-documented enum values for SettingsOptions::LockingMode.

  • Other information:
    With this tiny refactoring now all the constants for settings should be ironed out ;)

typedef enum {
DISABLED = 0, // Locking buttons is disabled
BOOST = 1, // Locking buttons for Boost mode only
FULL = 2, // Locking buttons fully
Copy link
Owner

@Ralim Ralim Jul 24, 2024

Choose a reason for hiding this comment

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

Suggested change
FULL = 2, // Locking buttons fully
ALL = 2, // Locking all buttons

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are you sure that this will be better? I just tried to keep the code as coherent as possible in accordance with the other parts including description itself and documentation to avoid any unnecessary ambiguity and not to puzzle anyone without extra need in the future.

We could change all the other descriptions after the suggestion though, but it would lead to the need of updating all translation files as well, which is - in my very humble opinion - total overkill.

TL;DR: this option-related value is notated as full in the description and in the documentation so why it should be all in the code exactly? Just curious.

Copy link
Owner

Choose a reason for hiding this comment

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

It can be full, mostly just wanted to not have "Locking buttons fully" as it feels a very clunky description, and we are not length limited here like we are in the description.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Aha, ok. I think I got it... Let me prepare a slightly updated solution then.

@@ -113,7 +113,7 @@ typedef enum {
typedef enum {
DISABLED = 0, // Locking buttons is disabled
BOOST = 1, // Locking buttons for Boost mode only
FULL = 2, // Locking buttons fully
FULL = 2, // Locking buttons for Boost mode AND for Soldering mode
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How about this? 👉 👈

Copy link
Owner

Choose a reason for hiding this comment

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

👍🏼

@Ralim Ralim merged commit 95eb154 into Ralim:dev Jul 24, 2024
18 checks passed
@neon12345
Copy link
Contributor

Is there a missing delay between lock and unlock? If I hold both buttons, it very quickly toggles between locked and unlocked.

@Ralim
Copy link
Owner

Ralim commented Jul 31, 2024

@neon12345 Best to open a new issue for this as this PR has already been merged and is not directly related.

@neon12345
Copy link
Contributor

@Ralim see #1956

@ia ia deleted the enum-lockingmode branch March 4, 2025 17:16
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.

3 participants