feat(color): Quick Model Select, keep active model in view when changing labels #2895

philmoz · 2022-12-19T02:14:23Z

On a number of occasions I have accidentally duplicated the current model from the popup menu.
This happens when I tap on the currently selected model and do not notice that the first entry is 'Duplicate Model'.

This change keeps the 'Select Model' entry as the first option even for the currently active model.
Selecting the menu option simply returns to the main view.

I think this makes for a more consistent UX.

pfeerick · 2022-12-19T03:11:20Z

I'm actually going to push a PR soon that will add a confirmation dialog to this (for the exact same reason), and a couple other more 'destructive' operations, so I'm not sure yet if this the right way to go.

philmoz · 2022-12-19T03:15:02Z

That's fine, it was just an idea.

Personally I think having consistent menus and behaviour is a better experience especially for new users.

pfeerick · 2022-12-19T05:08:45Z

That is true... but having menu entries that effectively do nothing can also be frustrating and clutter the UI... i.e. if there was a disabled state for menu entries that would probably be better. But then again, context-sensitive menus really should only offer up valid actions, rather than make the user have to guess which options will work or not work.

ParkerEde · 2022-12-19T14:15:22Z

@pfeerick @philmoz Please have a look to PR #2626. That would solve this issue too. What do you think about it? Some of my EdgeTX User friends and I use this, and we are happy with it.

philmoz · 2022-12-19T22:46:29Z

@ParkerEde - thanks for the heads up. Personally I like your solution, I think for most people changing the active model is the primary use for the model select page, so that should be the easiest / quickest thing to do.

I like the idea of moving the 'delete model' button to the bottom of the list as well. It keeps the popup menu more consistent.

I had a play with your code and simplified it a bit. If you're ok with it I will add my version of your solution to this PR.

Having said all that, this does significantly change the way the model select page works which could be disruptive for users who are familiar with the current UX.

philmoz · 2022-12-20T03:15:30Z

Now with my adaptation of @ParkerEde ideas for model selection.

ParkerEde · 2022-12-20T08:03:43Z

I have tested this. It's great.

What if we make a switch in radiosettings "Model quick select" False/True?
True = short ENTER selects marked Model immediately, long ENTER shows Menu
False = short/long ENTER shows Menu
default is false.
So we would be maximally compatible and on the other hand it is possible to switch to the fast model select behavior

ParkerEde · 2022-12-20T14:51:49Z

@philmoz
After opening the model selection screen and selecting another model, the first ENTER keystroke is not executed. Only the second press of ENTER will select the model.

modelselect.mp4

ducatiJake · 2022-12-20T17:35:02Z

Hi Guys, I made the code changes that @ParkerEde has commented on from PR #2626. Basically pretty much the same functional changes, but your code changes are more straight forward than mine were, so I am happy with your changes. I do agree with @ParkerEde on his above video that I also would prefer a single tap on a model icon box would select and open the tapped or clicked model.

pfeerick · 2022-12-21T01:30:50Z

Thanks for the video @ParkerEde

So the only thing really missing here is some form of visual feedback as to that first button press/tap having been actioned? As I think there should be some visual/audible feedback to any input, especially one like this one... i.e. consider the situation where you don't realise the first press has happened (since there is no feedback of that having happened?) Perhaps a change in the border color? Is a beep/haptic if sound is enabled warranted?

Or is this in reference to the #2626 behavioural change? If so, I agree... that would be better behind an toggle option - then you get the best of both. But that will guarantee that this will not make it into 2.8.1 (not that I'm saying that PR will make it into 2.8.1 anyway at this point) as it would require companion and storage changes (nothing major, just means more work is needed for that).

philmoz · 2022-12-21T02:08:43Z

I've fixed the issue with the scroll wheel / enter button selection.
If you are just using the touch screen you still need to select the model twice - once to get focus, and then to select it as active. I found it was too sensitive otherwise, sometimes interpreting a scroll as a selection.

I also added a 'Model quick select' setting as @ParkerEde suggested. Defaults to existing behaviour, turning it on selects new behaviour. Added this setting to companion as well.

Not sure I did the translation stuff correctly - I manually added the english string to all the language files.

pfeerick · 2022-12-21T02:15:55Z

At a glance, it looks fine... I don't think there is any need for the COLORLCD conditional - as there is no B&W alternative, but it won't do any harm. Very nice, thank you! I'll give it a try when I get back near a computer later. If that is ready to go then, will just ping the translators to give the non-english texts and that should be it.

ParkerEde · 2022-12-21T07:56:40Z

Translation DE
#define TR_MODEL_QUICK_SELECT "schnelle Modellauswahl"

ParkerEde · 2022-12-21T08:03:15Z

Wow.
I am very impressed. This looks super.
Also tested it in Companion and Simulator for X10 and TX16S. Looks perfect to me.
Thanks a lot
@pfeerick should we start a translator call?

pfeerick · 2022-12-21T09:36:31Z

@ParkerEde Yes, once there are no further changes needed to the PR. i.e. there is little point calling for translations if the initial string is wrong, or a second translation is needed ;)

Is the behavioural change that "model quick select" off introduces intentional? As this is not consistent with the rest of the UI. For touch, first tap normally selects, second tap brings up the menu - here the first tap selects and brings up the menu. And for hard key navigation, first ENT to select, second ENT for menu. Check the file browser for comparison (I thought the theme manager was the same, but it seems not 🤷 - that probably needs tweaking). Quick mode touch and hard key navigation is working fine though. Companion read/write working on TX16S.

philmoz · 2022-12-21T09:53:26Z

Behaviour change when quick select is off was unintentional - should be fixed now.

pfeerick · 2022-12-21T11:06:34Z

Thanks for that. One final request, and I think it's wrap. When selecting and unselecting a label, the active model doesn't always get brought into focus/shown... any chance of catching that or is it just an annoying edge case? Perhaps a video will explain it better... notice the model that has selection focus after the test label is unselected.

https://drive.google.com/file/d/124jVpDwAq9mOSSLPajKvU2BA-IvopMuj/view?usp=sharing

pfeerick · 2022-12-21T11:10:38Z

Can we get some translations for this please 😃

#define TR_MODEL_QUICK_SELECT "Model quick select"

@HThuren DA
~~@ParkerEde DE~~
@Eldenroot CZ
@ulfhedlund SE
@robustini IT
@zyren CN / TW
@ajjjjjjjj PL
@ToshihiroMakuuchi JP

robustini · 2022-12-21T11:14:01Z

IT
#define TR_MODEL_QUICK_SELECT "Selezione veloce modello"

Great implementation, I am tired of seeing duplicate models! :P

Eldenroot · 2022-12-21T11:20:31Z

CZ: Rychlý výběr modelu

ulfhedlund · 2022-12-21T12:02:29Z

SE
#define TR_MODEL_QUICK_SELECT "Snabbval av modell"

rotorman · 2022-12-21T13:47:54Z

FI: "Mallin pikavalinta"

zyren · 2022-12-21T13:50:19Z

CN
#define TR_MODEL_QUICK_SELECT "快速选择模型"

TW
#define TR_MODEL_QUICK_SELECT "快速選擇模型"

ToshihiroMakuuchi · 2022-12-21T13:59:58Z

JP
#define TR_MODEL_QUICK_SELECT "モデルクイックセレクト"

ajjjjjjjj · 2022-12-21T16:47:35Z

PL
#define TR_MODEL_QUICK_SELECT "Szybki wybór modelu"

HThuren · 2022-12-21T16:51:09Z

DK
#define TR_MODEL_QUICK_SELECT "Hurtigvalg af model"

philmoz · 2022-12-21T21:39:36Z

@pfeerick The code tries to always have a model selected. On first load that will be the active model. If you change to a label that does not include the active model it selects the first model in the list. When you change back to other label, the selected model remains as the first model from the previous set, not the active model.

Thinking about it, this may not be the best UX. It might be better to always remember the last model selected by the user. If it is in the list when changing labels then it gets re-selected, otherwise no model will be selected until the user selects one.

ducatiJake · 2022-12-21T22:09:34Z

@philmoz That is the way my implementation worked also. If the user has selected a model, then changed selected label, that included the currently selected model then that model stayed selected. If the model was not in the new labels list, the first model in the list was selected. Seemed right to me. My code also tried to always have a selected model. And by selected, I mean highlighted, not necessarily the active model.

pfeerick · 2022-12-21T23:18:00Z

Thinking about it, this may not be the best UX. It might be better to always remember the last model selected by the user. If it is in the list when changing labels then it gets re-selected, otherwise no model will be selected until the user selects one.

No, as @ducatiJake suggested, I think this code actually behaves now is correct in that respect - there should always be a select model, especially for hard key navigation. I was just surprised it prioritised keeping the last selected model in view and selected, since it's not like you need it selected to drag it or do something else with it. More I'm trying to identify the practical use / benefit to the end user for that - it's not like you need to toggle into a different label to access some other context-sensitive menu or drag and drop or anything. Perhaps so that you can see if the model is in a different label or not (without actually checking the labels assigned to the model)? Otherwise, I would have thought the priority was would be the active model, if it's in the current label/filter.

Add setting to enable quick model selection.

…2.8 logic.

…els changes.

Minor code cleanup.

pfeerick

Perfect timing - I was about to ask for a rebase ;) I agree re: "New Model" order... it for some reason would feel odd if it were changed from how it is... although I can also see the argument for it being changed. So let's leave it as it is for now until people start complaining it should be changed. 🤭 If everyone else is happy/thinks it makes sense that the focus should be the current model then this is ready to merge as far as I'm concerned.

ducatiJake · 2022-12-22T16:02:27Z

I built v2.8.1 this morning to check out these changes. I seem to have the Select Model changes, but not the Model Quick Select system setting. Not sure why. Anyhow, I have noticed one inconsistency. If I open Model Select, then use the roller to highlight another model, I have to click the roller twice to make it the active model and close the MS screen. If I open Model Select then highlight another model by tapping on that model which causes the same visual change as if I had used the roller to highlight it I only have to tap the model or press the roller 1 more time to make it the active model and close the MS screen. Shouldn't using the roller to highlight a model be the same as tapping on a model. Both give the same highlighted visual feedback. Seems to me that if a model is highlighted no matter how I got there a single tap or roller click should make that model active and close the MS screen. Other than this, I love these changes. I don't think the system setting is needed myself, but if most people believe it should be there, I have no issues with it being there. If it defaults off, it just makes it harder for users to discover these improvements.

ParkerEde · 2022-12-22T16:18:23Z

@ducatiJake
I suspect there is something wrong with the build with you. It is also included in Companion. However, I have only tested a Windows build. A model selected with the rotary encoder is opened with the first Enter keystroke. In case of touch operation, a model should be selected at the first touch and opened at the second touch. This is how it was agreed here. This is also how the current build behaves.
libedgetx-tx16s-simulator.zip
This libedgetx-tx16s-simulator.dll is based on actual main+this pr
does it work for you as expected?

ducatiJake · 2022-12-22T16:38:03Z

Thanks @ParkerEde I tried your .dll and see the same behavior as you. My build just got main plus everything labeled 2.8.1 which at least when I did it was not complete. Thanks for clarifying that.

pfeerick · 2022-12-22T23:49:36Z

This PR branch hasn't been merged yet, so unless you checked out this PR, you must have built against the wrong branch. Milestone is only a tracker of where it is planned this PR will first be in a release.

If it were just me (or a small group of people), I would also agree the setting would not be needed. However, there are too many people using this to make that sort of rash decision - safety is always the priority, so until 2.9 off should be the default state IMO. Then it could be changed to on, as people expect changes in behaviour on a major/minor release. I for one do not want to be responsible for an injury or damage caused due to an unexpected model change (given you can't rely exclusively on telemetry to indicate model active) if someone was unaware of the behaviour change.

HThuren · 2023-01-01T14:15:36Z

hi @ParkerEde / @pfeerick @zyren
Notice, this define is missing in tw.h
#define TR_MODEL_QUICK_SELECT "快速選擇模型"

Was missed in #2895

pfeerick · 2023-01-02T00:46:36Z

Thanks @HThuren ... fixed in 4b838c7

@ParkerEde

…ing labels (#2895) * Fix rebase merge issue. * Incorporate solution from @ParkerEde in PR #2626 * Fix selection logic for models. * Select model on first press. * Fix quick model selection when using scroll wheel & enter button. Add setting to enable quick model selection. * German translation. * Fix behaviour when 'model quick select' is off to work like existing 2.8 logic. * Add translations. * Prioritise active model as selected model when the set of visible models changes. * Fix selection ordering when using scroll wheel. Minor code cleanup.

Was missed in #2895

pfeerick · 2023-03-16T06:27:39Z

radio/src/gui/colorlcd/model_select.cpp

+    bool modelConnected =
+        TELEMETRY_STREAMING() && !g_eeGeneral.disableRssiPoweroffAlarm;
+    if (modelConnected) {
+      AUDIO_ERROR_MESSAGE(AU_MODEL_STILL_POWERED);


@philmoz I finally found a bug from this PR, which is quite nasty as it can completely lock the radio up. With the Quick Model select option on (it only triggers in the ON state), and you have a telemetry receiver active, when trying to switch model, you will get this warning, but it will keep repeating the message, and the haptic vibration continuously rather than a single time, and the radio will need to be hard powered off even if you do power off the receiver (and the prompts stop). This was with 2.8.1 - main does not seem to do this.

I think that was fixed in PR #3061 with the changes to the full screen dialog.
One of the commits in that PR mentions fixing the infinite loop issue.

Thanks - I'll see if 96cfe35 fixes it then and push it in for 2.8.2 if it behaves. :)

Now that you mention that PR, that reminds me... would you be able to double check if my memory is correct/why some of the alerts from storage/sdcard_yaml.cpp : const char * loadRadioSettingsYaml(bool checks) don't have any touch buttons? I think it's the ones that come up if there aren't any radio settings present.

There are three variation for the full screen popup:

confirmation dialog (with Enter and RTN buttons), generated with confirmationDialog() call

alert dialog with one action button defined by caller, generated with raiseAlert() and non-empty 3rd param

alert dialog with no buttons, generated by ALERT macro or raiseAlert() with empty 3rd param

sdcard_yaml.cpp uses the ALERT macro.

philmoz force-pushed the select-model-menu branch from 97642ec to f9061d6 Compare December 21, 2022 23:05

Phil Mitchell added 9 commits December 22, 2022 20:40

Incorporate solution from @ParkerEde in PR EdgeTX#2626

5bd5e7f

Fix selection logic for models.

378da7d

Select model on first press.

105f64d

Fix quick model selection when using scroll wheel & enter button.

2380bf1

Add setting to enable quick model selection.

German translation.

3be7e4f

Fix behaviour when 'model quick select' is off to work like existing …

bd8a6c7

…2.8 logic.

Add translations.

cc4a356

Prioritise active model as selected model when the set of visible mod…

ca3773e

…els changes.

Fix selection ordering when using scroll wheel.

037b788

Minor code cleanup.

philmoz force-pushed the select-model-menu branch from 80f9121 to 037b788 Compare December 22, 2022 09:47

pfeerick approved these changes Dec 22, 2022

View reviewed changes

pfeerick changed the title ~~Suggestion - keep 'Select Model' menu option in popup menu when tapping on current model~~ feat(color): Quick Model Select, keep active model in view when changing labels Dec 22, 2022

pfeerick added enhancement ✨ New feature or request color Related generally to color LCD radios UX-UI Related to user experience (UX) or user interface (UI) behaviour labels Dec 22, 2022

pfeerick mentioned this pull request Dec 22, 2022

Model select #2626

Closed

pfeerick added this to the 2.8.1 milestone Dec 22, 2022

pfeerick merged commit 3f08af6 into EdgeTX:main Dec 22, 2022

pfeerick added a commit that referenced this pull request Jan 2, 2023

fix: TW missing TR_MODEL_QUICK_SELECT

4b838c7

Was missed in #2895

pfeerick added a commit that referenced this pull request Jan 25, 2023

fix: TW missing TR_MODEL_QUICK_SELECT

3c68d45

Was missed in #2895

pfeerick reviewed Mar 16, 2023

View reviewed changes

pfeerick mentioned this pull request Apr 1, 2023

Failsafe not set warning do not have "Press any key to skip" message #3421

Closed

1 task

LevYas mentioned this pull request Apr 4, 2023

Radio Settings -> Radio Setup -> Model quick select EdgeTX/edgetx-user-manual#12

Closed

feat(color): Quick Model Select, keep active model in view when changing labels #2895

feat(color): Quick Model Select, keep active model in view when changing labels #2895

Conversation

philmoz commented Dec 19, 2022 • edited by pfeerick Loading

pfeerick commented Dec 19, 2022

philmoz commented Dec 19, 2022

pfeerick commented Dec 19, 2022

ParkerEde commented Dec 19, 2022 • edited Loading

philmoz commented Dec 19, 2022

philmoz commented Dec 20, 2022

ParkerEde commented Dec 20, 2022 • edited Loading

ParkerEde commented Dec 20, 2022 • edited Loading

ducatiJake commented Dec 20, 2022

pfeerick commented Dec 21, 2022 • edited Loading

philmoz commented Dec 21, 2022 • edited Loading

pfeerick commented Dec 21, 2022 • edited Loading

ParkerEde commented Dec 21, 2022

ParkerEde commented Dec 21, 2022

pfeerick commented Dec 21, 2022

philmoz commented Dec 21, 2022

pfeerick commented Dec 21, 2022

pfeerick commented Dec 21, 2022

robustini commented Dec 21, 2022 • edited Loading

Eldenroot commented Dec 21, 2022

ulfhedlund commented Dec 21, 2022

rotorman commented Dec 21, 2022

zyren commented Dec 21, 2022

ToshihiroMakuuchi commented Dec 21, 2022

ajjjjjjjj commented Dec 21, 2022

HThuren commented Dec 21, 2022

philmoz commented Dec 21, 2022

ducatiJake commented Dec 21, 2022

pfeerick commented Dec 21, 2022

pfeerick left a comment • edited Loading

Choose a reason for hiding this comment

ducatiJake commented Dec 22, 2022

ParkerEde commented Dec 22, 2022 • edited Loading

ducatiJake commented Dec 22, 2022

pfeerick commented Dec 22, 2022 • edited Loading

HThuren commented Jan 1, 2023 • edited by rotorman Loading

pfeerick commented Jan 2, 2023

pfeerick Mar 16, 2023

Choose a reason for hiding this comment

philmoz Mar 16, 2023

Choose a reason for hiding this comment

pfeerick Mar 16, 2023

Choose a reason for hiding this comment

pfeerick Mar 16, 2023

Choose a reason for hiding this comment

philmoz Mar 16, 2023

Choose a reason for hiding this comment

philmoz commented Dec 19, 2022 •

edited by pfeerick

Loading

ParkerEde commented Dec 19, 2022 •

edited

Loading

ParkerEde commented Dec 20, 2022 •

edited

Loading

ParkerEde commented Dec 20, 2022 •

edited

Loading

pfeerick commented Dec 21, 2022 •

edited

Loading

philmoz commented Dec 21, 2022 •

edited

Loading

pfeerick commented Dec 21, 2022 •

edited

Loading

robustini commented Dec 21, 2022 •

edited

Loading

pfeerick left a comment •

edited

Loading

ParkerEde commented Dec 22, 2022 •

edited

Loading

pfeerick commented Dec 22, 2022 •

edited

Loading

HThuren commented Jan 1, 2023 •

edited by rotorman

Loading