IronOS icon indicating copy to clipboard operation
IronOS copied to clipboard

Disable short press of + button to change temperature to prevent accidentally changing temp only when using boost mode.

Open nonokirby opened this issue 11 months ago • 10 comments

  • Please check if the PR fulfills these requirements
  • [x] The changes have been tested locally tested on pinecilV2
  • [x] There are no breaking changes
  • What kind of change does this PR introduce? Feature addition

  • What is the current behavior? #1663

  • What is the new behavior (if this is a feature change)? A new option in locking mode has been added to accommodate those who prefer the original behavior. When locking mode is set to T or Temperature mode, the boost button's temperature change feature will be disabled.

  • Other information: Should documentation changes be needed, I will gladly make the changes to reflect this update.

Please make sure I added the changes to the translations in compliance with the current code, as it is the change I am least confident i did correctly.

This is the first project I have contributed on in my life, so any guidance on what I need to change for docs or how to best test would be greatly appreciated.

Status: Ready for Review

nonokirby avatar Jan 21 '25 05:01 nonokirby

@ia Could you let me know if there's any better way I could have/should have done the translation.json file changes? I'm almost 100% certain this was not the best way, and I want to work on a few more unrelated changes, but not before learning how to do this properly. Thanks for all the work you guys put into this stuff!

nonokirby avatar Jan 22 '25 03:01 nonokirby

I want to work on a few more unrelated changes

Rule of thumb as the advises for the future:

  • do not break the default behavior, because there are a lot of users who got used to the defaults;
  • one feature/change/fix - one pull-request, because the more changes in one patch are, the more hard it is to review, check and roll back in case of a problem;
  • "weight" the cost of changes (if it's not a bug fix but some new feature or addition), from the point of firmware size and/or complicating code base.

ia avatar Jan 23 '25 19:01 ia

Status update on my side: during this weekend I would like to make a couple of additional commits related to the management of displaying this option in the settings properly to avoid ambiguity for users, and to make testing to make sure that everything works as expected.

ia avatar Jan 25 '25 00:01 ia

@ia Sounds good. Let me know whatever you need me to do to help. Whenever you're ready, let me know and i'll download the build and do final testing.

nonokirby avatar Jan 25 '25 16:01 nonokirby

How about just setting lower limit of temperature step to 0 instead of 1? Can be a potential solution, and no new menu item is required

resistancelion avatar Jan 26 '25 05:01 resistancelion

How about just setting lower limit of temperature step to 0 instead of 1?

Do you mean Soldering settings - Temp change short setting? As far as I understand, in the context of the requested feature it doesn't make much of a sense, since there still must be the way to actually change the soldering temperature in the soldering mode.

ia avatar Jan 26 '25 21:01 ia

Status update: I did make a few tests using the latest build from this branch and here is my feedback in a form of report.

Steps to reproduce:

  • enable T as Locking mode in the settings;
  • go to soldering mode;
  • press +/A button.

What should happen:

  • soldering temp change since locking mode is not enabled yet, because user did not press A+B buttons yet to enable locking mode, i.e. lock the buttons.

What actually happens:

  • -/A button is permanently lock without invoking locking/unlocking familiar routine (the only way to disable it: go to settings and change locking mode from TEMP).

Why it's important:

  • for users who get used to use locking mode feature, there is already well formed mental usability pattern that locking mode enabled and disabled through A+B click with visual feedback supplemented by LOCKED / UNLOCKED caption on display. And it misses !LOCKED! caption on +/A being short pressed while TEMP locking mode is enabled.

How it should be implemented:

  • user enables T as Locking mode in the settings;
  • user goes to soldering mode;
  • if user presses +/A button, it should show temp change "sub-window";
  • if user presses A+B, screen shows LOCKED and now +/A is locked;
  • if user presses +/A button after the previous step, screen shows !LOCKED! and nothing happens;
  • if user presses A+B, screen shows UNLOCKED and now +/A is unlocked;
  • probably preferably, although debatable: since -/B is not locked, so user can set an iron to standby mode and going to menu "resetting" soldering workflow, +/A should not be still locked on exit from standby or menu (TL;DR: if user sets TEMP locking mode and enables it with A+B, but then goes to standby and/or menu, then on exit from standby/menu +/A should be unlocked).

ia avatar Jan 26 '25 22:01 ia

The locking mode setting seemed most appropriate to add the setting to without adding a new settings option, despite the functionality differences.

In fact, I think that LockingMode routine is very suitable place for this feature since it locks one of the buttons in soldering mode.

Unfortunately, as I mentioned in my old comment, for many reasons it's pretty impossible to implement "do X with N button" actions for anything some users want.

ia avatar Jan 26 '25 22:01 ia

Looking through the ui logic, it seems that implementing locking mode properly would require a rewrite of the entire locking mode system. previously, the only exception in locking mode was boost mode, but doing the same for the full functionality would require many if statements nested in case blocks. The alternative is ignore the scratch state and just call the locked alert, but then there would be no way to disable locking mode. Not sure how to proceed.

nonokirby avatar Jan 29 '25 17:01 nonokirby

I think at this point it is probably wise that we break up the button handling and decode logic so that it doesn't grow into nested if statements.

I'll be back near my computer later this week and can comment more than (sorry for delay)

Ralim avatar Feb 02 '25 08:02 Ralim