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

Added option to Swap A and B buttons in menu #2059

Open
wants to merge 51 commits into
base: dev
Choose a base branch
from

Conversation

resistancelion
Copy link
Contributor

@resistancelion resistancelion commented Jan 26, 2025

  • Please check if the PR fulfills these requirements
  • [OK, compiles & functions for TS100 3.45/TS101 3.50] The changes have been tested locally
  • [Forces to go to default settings after update] There are no breaking changes
  • What kind of change does this PR introduce?
    Added menu option to swap A and B key functionality in menus (doesn't affect the Temperature regulation buttons)
    Note: Effect is immediate, so if you double tap A, you'll apply it and go back to previous menu

#2034

  • Other information:
    UK, BG, BE translations doesn't require re-visiting
    New compiler definition: REVERSE_NAV_EVERYWHERE to make it reverse buttons not only in menu (if needed)

Unified statement to one type of translation ("Sleep mode" instead of "Waiting mode")
Corrected "blink" translation
Shortened phrases for statements ("t° у сек"/"градусів на секунду"; "сек"/"секунд","секунди"; "Хв"/"Хвилин","Хвилини")
Traslated soldering tip type selection menu
Spelling corrected + replaced "temperature" with "t°" in few places
Unified a few more sentences to be suitable for a context

Fixed text size to better fit TS101/TS100
getButtonState() now have an option int/bool argument to swap the result via XOR
@discip discip enabled auto-merge (squash) January 26, 2025 12:07
@discip discip disabled auto-merge January 26, 2025 12:07
@ia ia added the Usability UX, Usability and/or Design. label Jan 26, 2025
@ia ia self-requested a review January 26, 2025 18:35
@ia
Copy link
Collaborator

ia commented Jan 26, 2025

Hello. Since I'm interested in this feature myself, I would like to test everything myself before merge, but I will try to give feedbck in 2-3 days tops.

@Ralim
Copy link
Owner

Ralim commented Jan 27, 2025

This code change seems fine, @ia if you can test this would be great, feel free to merge if it works in testing

Copy link
Collaborator

@ia ia left a comment

Choose a reason for hiding this comment

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

Just a few rants about code style practices.

Copy link
Collaborator

@ia ia left a comment

Choose a reason for hiding this comment

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

Sorry for bad news again. :/
Just really would like to know how much do you need this REVERSE_NAV_EVERYWHERE feature for yourself.

@@ -43,8 +43,15 @@ guiContext context; // Conte
OperatingMode handle_post_init_state();
OperatingMode guiHandleDraw(void) {
OLED::clearScreen(); // Clear ready for render pass
bool d = getSettingValue(SettingsOptions::ReverseButtonNavEnabled);
bool e = getSettingValue(SettingsOptions::ReverseButtonTempChangeEnabled);
#ifdef REVERSE_NAV_EVERYWHERE
Copy link
Collaborator

Choose a reason for hiding this comment

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

New compiler definition: REVERSE_NAV_EVERYWHERE to make it reverse buttons not only in menu (if needed)

Are you going to use this feature yourself? I really would like to know. And if you're not, then could this be removed, please?

My concern here is very simple: there are already two separate files to handle different screens, and now there will be two additional "combinatorial varieties", so basically when next time there will be some changes there, we have to think about basically four different use-case code paths, which may turn into nightmare from the perspective of code management & maintaining in the future.

Copy link
Contributor Author

@resistancelion resistancelion Feb 6, 2025

Choose a reason for hiding this comment

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

Yes, i've thought that i've sent a comment mentioning it, but apparently i've forgot and had sent only this one

Copy link
Contributor Author

@resistancelion resistancelion Feb 6, 2025

Choose a reason for hiding this comment

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

may turn into nightmare from the perspective of code management & maintaining in the future.

no request for or work towards - my mindset about such

Copy link
Collaborator

Choose a reason for hiding this comment

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

Pardon, I'm a bit confused with your replies.

I have a simple question (two, actually): are you really going to use REVERSE_NAV_EVERYWHERE code paths yourself on a daily basis? Is that really more comfortable for you to use boost mode with the back button while soldering and going to standby/settings with the front button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

are you really going to use REVERSE_NAV_EVERYWHERE code paths yourself on a daily basis? Is that really more comfortable for you to use boost mode with the back button while soldering and going to standby/settings with the front button?

No, i use both +/- + A/B reverse, so yeah, Just a leak of conceptual analysis from my side, will it suit more to modify existing swapping setting to vary from 0 to 3, where 0 is neither and 3 is both?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Anyway, then let's keep for now REVERSE_NAV_EVERYWHERE as macro, and the swap of buttons for the settings menu as a setting, just like the original feature request in the issue was with the comments there. And I thought it was pretty clear there, so that's why I'm very confused how any misunderstanding about this feature could happen in the first place, since there is no any mention nor request about changing buttons on the main screens for Soldering mode and for Standy mode.

will it suit more to modify existing swapping setting to vary from 0 to 3 [...]

With high probability this turn some parts of the code base into even more hard-to-maintain spaghetti, than OLED section here now. Please, try to understand, that it's not a problem to staff as many features & options as some may would like to have, and then use it for themselves, but about for maintainers to keep it, well, maintain & support in the future.

But if there will be at least ~3 more users requesting an option to swap buttons on the main screen modes in the future as well, with heavy grounded reasoning, we may consider to add this as another boolean option, i.e. "swap buttons on main screen".

@ia ia self-assigned this Feb 8, 2025
@ia ia linked an issue Feb 8, 2025 that may be closed by this pull request
Copy link
Collaborator

@ia ia left a comment

Choose a reason for hiding this comment

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

@resistancelion , as of for the review, these are a few code style notices:

  • first of all, could, you, please, fix this;
  • and somehow some files have been modified, while they shouldn't, so could, you, please, add/remove extra spaces at these places, like it used to be, because there were proper alignments originally, but now these changes only "spam" diff-view with noise, and not required:

@@ -495,7 +495,7 @@ static __INLINE q63_t mult32x64(q63_t x, q31_t y) { return ((((q63_t)(x & 0x0000
/*
#if defined (ARM_MATH_CM0_FAMILY) && defined ( __CC_ARM )
#define __CLZ __clz
#endif
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

Comment on lines 39 to 41
* #else
* #error "We need NMSIS 1.1.5 or later!"
* #endif
* #endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

@@ -206,7 +206,7 @@ typedef enum IRQn {
#ifdef NMSIS_ECLIC_VIRTUAL
#ifndef NMSIS_ECLIC_VIRTUAL_HEADER_FILE
#define NMSIS_ECLIC_VIRTUAL_HEADER_FILE "nmsis_eclic_virtual.h"
#endif
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

@@ -238,7 +238,7 @@ typedef enum IRQn {
#ifdef NMSIS_VECTAB_VIRTUAL
#ifndef NMSIS_VECTAB_VIRTUAL_HEADER_FILE
#define NMSIS_VECTAB_VIRTUAL_HEADER_FILE "nmsis_vectab_virtual.h"
#endif
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

Comment on lines 39 to 41
* #else
* #error "We need NMSIS 1.1.5 or later!"
* #endif
* #endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

@@ -495,7 +495,7 @@ static __INLINE q63_t mult32x64(q63_t x, q31_t y) { return ((((q63_t)(x & 0x0000
/*
#if defined (ARM_MATH_CM0_FAMILY) && defined ( __CC_ARM )
#define __CLZ __clz
#endif
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

@@ -168,7 +168,7 @@
0x03, 0x07, 0x0e, 0x1c, 0x38, 0x70, 0xe0, 0xc0, 0x80, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x06, 0x07, 0x03, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x03, 0x07, 0x06, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
#endif
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

@@ -356,7 +356,7 @@
0x00, 0x00, 0x3e, 0x00, 0x00, 0x00, 0x07, 0x0f, 0x2b, 0x09, 0x07, 0x00, 0x01, 0x02, 0x04, 0x04, 0x04, 0x04, 0x04, 0x02, 0x01,
}
};
#else
#else
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

@@ -401,7 +401,7 @@
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x80, 0x80, 0x40, 0x40, 0x40, 0x40, 0x20, 0x20, 0x20, 0x20, 0x10, 0x10, 0xd0, 0xc8, 0x08, 0x10, 0x10, 0x10, 0x10,
0x20, 0x20, 0x20, 0x40, 0xc0, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x03, 0x04, 0x04, 0x38, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x37, 0x37, 0x00, 0x00, 0x00, 0x00, 0x00, 0x38, 0x04, 0x04, 0x02, 0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
#endif
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

Comment on lines 582 to 583
};
#endif
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • here

Copy link
Collaborator

@ia ia left a comment

Choose a reason for hiding this comment

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

UPD: Oops! I was heavily confused, because I got lost in macro names myself. I did make a heavy confusion between REVERSE_NAV_EVERYWHERE and REVERSE_BUTTON_NAV, obviously. :/

About macro management... These definitions with REVERSE_NAV_EVERYWHERE should be commented, i.e.:

// #define REVERSE_BUTTON_NAV // Uncomment to swap buttons completely in _Soldering mode_ and in _Standby mode_

in these places:

@CakeIsYummy21
Copy link

Very cool to see it getting done! : )

@ia
Copy link
Collaborator

ia commented Feb 13, 2025

Very cool to see it getting done! : )

Thanks, @CakeIsYummy21! I only would like to refactor a couple of things myself, so I hope it will be over by this weekend.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Usability UX, Usability and/or Design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to switch A and B button for settings
4 participants