Skip to content

fix(color): Logical switches monitor fix#2410

Merged
pfeerick merged 4 commits intoEdgeTX:mainfrom
banyaszg:fix_2384
Oct 2, 2022
Merged

fix(color): Logical switches monitor fix#2410
pfeerick merged 4 commits intoEdgeTX:mainfrom
banyaszg:fix_2384

Conversation

@banyaszg
Copy link
Contributor

Fixes #2384

Summary of changes:
This patch uses styles for active and non active buttons. The style defines colors and fonts for each logical switch state.

@pfeerick pfeerick added color Related generally to color LCD radios bug/regression ↩️ A new version of EdgeTX broke something labels Sep 29, 2022
@pfeerick pfeerick added this to the 2.8 milestone Sep 29, 2022
@raphaelcoeffic
Copy link
Member

I would have strongly preferred the ButtonMatrix. Was there an issue with taking that route?

we should avoid having Styles created and applied in object constructors when possible, as it is rather slow. It this particular case it is also leaking memory, as the styles need to be destroyed after use (LVGL is not C++).

@banyaszg
Copy link
Contributor Author

banyaszg commented Sep 29, 2022

I implemented with ButtonMatrix first. It worked well except one behaviour. The focus/editing thing caused a lot of problems. The whole matrix was a widget, so normaly you need to push the Enter to start editing mode. Then you can scroll the focus with the scroll button, but if you press the Enter again, then you leave the editing mode and lose the selection. I spent hours to find an elegant solution, but it required so much hacking I gave up around 1am. Then I tried to fix the current code, and this way looked as an option. Now it works, but the implementation is not valid yet, so I thought I will sleep first and refine it after.

@banyaszg
Copy link
Contributor Author

Actually we can do with direct change too skipping the styles, just I thought it is a good was to create a complete setting for each state.

@banyaszg
Copy link
Contributor Author

@raphaelcoeffic What do you think about defining and using a custom state as it is in this version?

@pfeerick pfeerick linked an issue Sep 29, 2022 that may be closed by this pull request
1 task
@raphaelcoeffic
Copy link
Member

What do you think about defining and using a custom state as it is in this version?

This is the right way to go, for sure. However, this is basically the same as LV_STATE_CHECKED, so that you should not have to use a custom state for that.

The only difference I see here to the style that covers that state is:

  • Text color SECONDARY1 instead of PRIMARY2
  • Bold font

I understand the use of Bold, but we should probably keep the same colours for sake of consistency.

@banyaszg
Copy link
Contributor Author

banyaszg commented Oct 1, 2022

The style settings are from the orignal code to look the same as it was.
I saw the similarities with the LV_STATE_CHECKED style, but I thought the color differs too much. Actually the PRIMARY2 is white in the default theme and the white text on yellow background moderately readable. I think it is ok for one or two options, because I know what that thing is, but in middle of such a big matrix it counts.

@raphaelcoeffic
Copy link
Member

Actually the PRIMARY2 is white in the default theme and the white text on yellow background moderately readable.

This is something lots of people have complained about. So maybe we should change the generic style to use SECONDARY1 instead when background is ACTIVE? That would leave us with just the bold font here.

@banyaszg
Copy link
Contributor Author

banyaszg commented Oct 1, 2022

This is something lots of people have complained about. So maybe we should change the generic style to use SECONDARY1 instead when background is ACTIVE? That would leave us with just the bold font here.

I think it is a good idea to change that color. After a certain age we really appreciate the contrast. Also the bold font could be default for the checked state. However the PRIMARY1 color can be enough without bold.
And with the proper contrast the LV_STATE_CHECKED would be good in this part.

@pfeerick
Copy link
Member

pfeerick commented Oct 1, 2022

After a certain age we really appreciate the contrast.

Age has nothing to do with it... I'm not that old and I still appreciate good contrast... 😀

@banyaszg
Copy link
Contributor Author

banyaszg commented Oct 1, 2022

Age has nothing to do with it... I'm not that old and I still appreciate good contrast... grinning

I'm also not old, just my minimum focus distance is increasing.
But at 18 meters I still hit the 10. 😀

@gagarinlg
Copy link
Member

Age has nothing to do with it... I'm not that old and I still appreciate good contrast... grinning

I'm also not old, just my minimum focus distance is increasing.

I am slowly getting in the area where things are either too far away or too close, but there is no point, where I can look sharp.

@pfeerick
Copy link
Member

pfeerick commented Oct 1, 2022

Thankfully (or not) mine should never change now... had cataract surgery a couple of years ago so fixed focus vision now - unless the warranty runs out on the lenses 🙃

@banyaszg
Copy link
Contributor Author

banyaszg commented Oct 1, 2022

I assume the default LV_STATE_CHECKED style will be changed, so I modified the code to use it.
Also I modified the layout for the portrait mode to look better. (Tested in NV14 simulator.)

@banyaszg banyaszg marked this pull request as ready for review October 1, 2022 18:10
@pfeerick
Copy link
Member

pfeerick commented Oct 2, 2022

This generally looks a lot better, as well as on the NV14. Thanks for that!

I'll play with the styles in a separate PR, now that this sets the foundations for that.

@pfeerick pfeerick merged commit f223e49 into EdgeTX:main Oct 2, 2022
pfeerick pushed a commit that referenced this pull request Oct 2, 2022
* fix(color): Logical switches monitor fix

* Direct button style modification.

* Using a custom state

* Using the default CHECKED style. Adaptation to portrait mode.
@banyaszg banyaszg deleted the fix_2384 branch October 6, 2022 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug/regression ↩️ A new version of EdgeTX broke something color Related generally to color LCD radios

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Logical switch monitor 2.8.0-rc1 Logical switches monitor becomes unresponsive

4 participants