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

feat(color): Single select option for model labels plus label AND/OR filtering logic. #4532

Merged
merged 8 commits into from
Jan 26, 2024

Conversation

philmoz
Copy link
Collaborator

@philmoz philmoz commented Jan 11, 2024

Fixes #4188

This is my take on a better way to handle labels. Feedback welcome.

TODO:

Adds new options to control how the label selection and filtering work. A new 'Manage Models' setup view has been added to the Radio Setup page with the options (Model quick select has also been moved to this new page).

There are three new options:

  • 'Label select' - 'Multi select' or 'Single select' (Multi select is the default). If Single select is chosen then only a single label can be selected.
  • 'Label matching' - 'Match all' or 'Match any' (Match all is the default). Match all is the current logic - only models having all selected labels are shown. Match any will show models with any of the selected labels.
  • 'Favorites matching' - Only available when 'Match any' is selected for Label matching. Options are 'Must match' and 'Optional match' (Must match is the default). Only applies when 'Favorites' is in one of the selected labels. If 'Must match' is selected then only shows models that have Favorites AND the other selections. If 'Optional match' is selected then models that match Favorites OR any of the other labels are shown.

screenshot_tx16s_24-01-11_15-23-01
screenshot_tx16s_24-01-11_15-23-15
screenshot_tx16s_24-01-11_15-23-31
screenshot_tx16s_24-01-11_15-23-37
screenshot_tx16s_24-01-11_15-23-45

I've also tried to show the logic iin the labels list:

Screenshot 2024-01-11 at 3 30 51 pm Screenshot 2024-01-11 at 3 31 28 pm Screenshot 2024-01-11 at 3 31 52 pm

@jtaylor2
Copy link
Contributor

Compiled and tested this PR in SIMU for tx16s. PR goes far beyond what I envisioned in #4188. Thank you.

@ParkerEde
Copy link
Contributor

ParkerEde commented Jan 11, 2024

I have tested the PR. Very good idea. "Single select" in particular is very good. I only noticed that if a label is selected, the check mark is also displayed. If I now select a model, it is loaded. If I now go back to the model manager, the last label is also displayed correctly, but the check mark is no longer visible.

@philmoz
Copy link
Collaborator Author

philmoz commented Jan 12, 2024

I have tested the PR. Very good idea. "Single select" in particular is very good. I only noticed that if a label is selected, the check mark is also displayed. If I now select a model, it is loaded. If I now go back to the model manager, the last label is also displayed correctly, but the check mark is no longer visible.

Thanks for testing - the single select check mark should work now.

@ParkerEde
Copy link
Contributor

yes, nice. Thx

@ParkerEde
Copy link
Contributor

Now I found out during testing that the PageUp/down no longer work. Please also note that the Horus x10 with page longpress for down must work. It was the same before this PR. The checkmark must also be displayed.

@philmoz
Copy link
Collaborator Author

philmoz commented Jan 13, 2024

Now I found out during testing that the PageUp/down no longer work. Please also note that the Horus x10 with page longpress for down must work. It was the same before this PR. The checkmark must also be displayed.

The PGUP/PGDN keys should now work in single select mode.

@ParkerEde
Copy link
Contributor

ParkerEde commented Jan 13, 2024

PgUp/Down is not quite perfect yet. The wrap around should jump directly to the first label and not to none. This drastically increases the loading time if you want to jump from the last to the first label. Before this PR, it was also the case that the wrap around jumped directly from the last to the first label. This affects both modes (single/multi select)

I have noticed that the list position jumps when going through the labels with the PgUp/Dn key, which looks very strange. It jumps to the correct next label, but within the list the entries jump up and down strangely. I have one label more than can be displayed at the same time (6 labels). By the way, in 2.9.2 6 labels could be displayed at the same time, now only 5, why did they change that?

@ulfhedlund
Copy link
Contributor

Great effort!
SE translations:
#define TR_LABELS_SELECT "Etikettval"
#define TR_LABELS_MATCH "Etikettmatchning"
#define TR_FAV_MATCH "Matcha favoriter"
#define TR_LABELS_SELECT_MODE "Flerval","Enskilt val"
#define TR_LABELS_MATCH_MODE "Matcha alla","Matcha någon"
#define TR_FAV_MATCH_MODE "Måste matcha","Alternativt matcha"

@pfeerick
Copy link
Member

By the way, in 2.9.2 6 labels could be displayed at the same time, now only 5, why did they change that?

Looks like the line height for each entry has increased slightly.

image

image

@philmoz
Copy link
Collaborator Author

philmoz commented Jan 14, 2024

PgUp/Down is not quite perfect yet. The wrap around should jump directly to the first label and not to none. This drastically increases the loading time if you want to jump from the last to the first label. Before this PR, it was also the case that the wrap around jumped directly from the last to the first label. This affects both modes (single/multi select)

I assumed that this was an oversight in the previous code - if it jumps directly to the first/last entry on wrap around then there is no way to clear the selection and show all models (on radios without a touch screen).

@ParkerEde
Copy link
Contributor

I think that since there have been no complaints from Horus users about this, the behavior should not be changed. There is also no need to jump to no label in the wrap arround, because why should I be interested in a complete list if I have taken the trouble to organize my model within labels. Please, please do it the way it was in the previous versions. And if you want to have an unsorted complete list, you can go to the list of labels with the focus, press Enter and then select one or more labels or even no label, as it has always been in 2.9.2.

@ParkerEde
Copy link
Contributor

DE translations:
#define TR_LABELS_SELECT " Labelauswahl"
#define TR_LABELS_MATCH "Labelvergleich"
#define TR_FAV_MATCH "Favoriten vergleichen"
#define TR_LABELS_SELECT_MODE "Mehrfachauswahl", "Einfachauswahl"
#define TR_LABELS_MATCH_MODE "Alle", "Beliebig"
#define TR_FAV_MATCH_MODE "Muss übereinstimmen", "Alternative Übereinstimmung"

@philmoz
Copy link
Collaborator Author

philmoz commented Jan 15, 2024

I have reverted the PGUP/DN wrap around logic.

@ParkerEde
Copy link
Contributor

Thx. Now it's fine to me.

@ParkerEde
Copy link
Contributor

Shouldn't we also make the labeling of this button translatable?
image

@philmoz
Copy link
Collaborator Author

philmoz commented Jan 15, 2024

Shouldn't we also make the labeling of this button translatable?

TR_MANAGE_MODELS

@ParkerEde
Copy link
Contributor

ParkerEde commented Jan 15, 2024

DE:
#define TR_MANAGE_MODELS "Modellmanager"

@philmoz philmoz force-pushed the feat-label-select-logic branch from fa96f61 to 76ffa4e Compare January 19, 2024 20:28
@philmoz
Copy link
Collaborator Author

philmoz commented Jan 19, 2024

DE: #define TR_MANAGE_MODELS "Modellmanager"

This is also used for the title on the manage models page where all caps is the current standard.
Screenshot 2024-01-20 at 7 30 18 am

@ParkerEde
Copy link
Contributor

But on the buttons, all capital letters don't look very good. All other burtons have normal lettering.

@philmoz
Copy link
Collaborator Author

philmoz commented Jan 19, 2024

But on the buttons, all capital letters don't look very good. All other burtons have normal lettering.

I agree. I also think that the all caps on the menus looks bad and should be changed; but that's a topic for a separate PR.

@philmoz
Copy link
Collaborator Author

philmoz commented Jan 19, 2024

I have updated this to use the quick menu string (mixed case) for the button title instead of the all caps menu title string.

@pfeerick
Copy link
Member

@philmoz Do you have any problems with #4524 and this PR getting merged, and a followup PR to catch Companion up?

@pfeerick pfeerick added color Related generally to color LCD radios UX-UI Related to user experience (UX) or user interface (UI) behaviour labels Jan 21, 2024
@pfeerick pfeerick added this to the 2.10 milestone Jan 21, 2024
@philmoz
Copy link
Collaborator Author

philmoz commented Jan 21, 2024

Do you have any problems with #4524 and this PR getting merged, and a followup PR to catch Companion up?

Ok with me; but this one needs translations.

@pfeerick
Copy link
Member

pfeerick commented Jan 21, 2024

I think that will probably be it's own PR as well, as there are several translations needed for each language now, across multiple PRs, so (overdue) time to pull them all together...

@pfeerick pfeerick added the user manual change Will impact on the user manual in some respect label Jan 26, 2024
@pfeerick
Copy link
Member

Looks great on TX16. I love how you added the AND/OR to the label list to help inform why it's matching the way it does... should help with people still confused how labels behave by default.

@pfeerick pfeerick merged commit 2c01bee into EdgeTX:main Jan 26, 2024
41 checks passed
@pfeerick pfeerick mentioned this pull request Mar 11, 2024
13 tasks
@philmoz philmoz deleted the feat-label-select-logic branch April 29, 2024 02:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
color Related generally to color LCD radios user manual change Will impact on the user manual in some respect UX-UI Related to user experience (UX) or user interface (UI) behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatically uncheck a label when checking a second label results in no models
5 participants