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

Why Long Press to bring up model menu in Select Model #2510

Closed
1 task done
ducatiJake opened this issue Oct 10, 2022 · 21 comments · Fixed by #2533
Closed
1 task done

Why Long Press to bring up model menu in Select Model #2510

ducatiJake opened this issue Oct 10, 2022 · 21 comments · Fixed by #2533
Labels
enhancement ✨ New feature or request

Comments

@ducatiJake
Copy link

Is there an existing issue for this feature request?

  • I have searched the existing issues

Is your feature request related to a problem?

In 2.7.1 you just had to single tap a model image in the select model dialog to bring up it's menu from which you could select, delete, etc the model. In 2.8 you have to long press to get the same behavior. This seems inconsistent to me as I can just single tap the New Label button or the alphabetize button or actually all of the other UI elements. Also, in 2.7.1 if I deleted a model, the next model in the list would become highlighted. In 2.8 it goes back to the top which seems confusing.

Describe the solution you'd like

Please go back to the 2.7.1 single tap on a model to bring up its menu behavior.
Also please go back to the 2.7.1 highlight next model when deleting a model.

Describe alternatives you've considered

No response

Additional context

No response

@ducatiJake ducatiJake added the enhancement ✨ New feature or request label Oct 10, 2022
@raphaelcoeffic
Copy link
Member

I think this has already been discussed somewhere... @pfeerick @dlktdr do you guys remember where?

@jtaylor2
Copy link
Contributor

I don't remember where it was discussed, but if I remember correctly it was done this way to avoid the possibility of bringing up the menu when scrolling and also a discussion of maybe double tap to select later on.

I too think the current implementation is not ideal as it now takes 3 actions to select a model (tap to select the icon, long press to bring up the menu, and finally tap select from the menu). As someone who alternates planes pretty much every flight that along with the delay bringing up the icons (which hopefully labels will help) seems like it will become annoying after 12 or 15 flights.

I compile my own software and have changed the model select to just select the model on the first tap and bring up the menu on a long press (which is the way I think it should work) and tested in simu and the simulator and don't really see any down side to doing it that way, but I haven't tried it on the transmitter yet. That's the nice thing about open source, if you don't agree with the decisions made by the project you can have it your way.

Jim

@ducatiJake
Copy link
Author

I notice that in 2.7.1 when I tap drag a model it does not highlight that model. In 2.8 it does which I think is what you were trying to avoid. If we could go back to that behavior, I think we could then go back to single tap to bring up the menu. Jim's suggestion is interesting. Might have to brush up my C++ skills.

@pfeerick
Copy link
Member

I think it was originally long press for menu, but I reserve the right be proved wrong :-P #1374 (comment)

I brought up the idea of "double tap to load" for discussion, although I don't think it is a good idea either. #2266 (comment)

Any code you can submit as a PR @jtaylor2? More hands the merrier ;)

@dlktdr
Copy link
Collaborator

dlktdr commented Oct 11, 2022

I don't remember my self either. @kevinkoenig maybe will, he did a lot of the initial code for the GUI portion. I thought it was something to do with dragging and mis-clicks but since PR #1467 added the warning if still connected to a model I don't really see any reason it really needs a long press. Delete has an extra warning and all other mis-clicks are reversible.

Since @jtaylor2 already did the changes I would be willing to test that PR too 😄

@Eldenroot
Copy link
Contributor

Safety reasons?

@jtaylor2
Copy link
Contributor

I can do a pull request, but I haven't tested my change on the radio yet and I was under the impression that the project didn't want a direct select.

My idea is normal press on a model selects it and long press brings up the current menu (which still contains the select model entry). The change is simple and just duplicates the select Model() from the long press handler to the press handler.

I'll put the diff here and if the consensus is it has merit and doesn't create problems I can do a pull request or someone can just cherry pick it.

diff --git a/radio/src/gui/colorlcd/model_select.cpp b/radio/src/gui/colorl
cd/model_select.cpp
index 606b9c52b..96ac8568e 100644
--- a/radio/src/gui/colorlcd/model_select.cpp
+++ b/radio/src/gui/colorlcd/model_select.cpp
@@ -469,7 +469,12 @@ void ModelsPageBody::update(int selected)
 
   for (auto &model : models) {
     auto button = new ModelButton(this, rect_t{}, model);
-    button->setPressHandler([=]() -> uint8_t { return 1; });
+    button->setPressHandler([=]() -> uint8_t {
+      if (model != modelslist.getCurrentModel()) {
+        selectModel(model);
+       }
+        return 0;
+     });
 
     // Long Press Handler for Models
     button->setLongPressHandler([=]() -> uint8_t {

model_select.diff.zip

@pfeerick
Copy link
Member

pfeerick commented Oct 11, 2022

How you describe it is exactly how I thought it should be (i.e. basically the same as 2.7) ... I don't see any purpose served by the need to tap to select, then tap'n'hold, and then finally "select model" - it's not like it's a multi-select mode for labeling or anything. So will be playing with this also. :) My comment earlier and before was in relation to double-tap - i.e. the same as in double-clicking the mouse on a PC to run a program - to change models. As I could see that would inherently be dangerous/annoying since it could be done inadvertently, but thought it was worth discussing, but this will probably neatly end that train of thought ;)

@ducatiJake
Copy link
Author

I have compiled Jim's change into my firmware and run it in my TX16S. I am quite happy with this change, it makes it much quicker, easier and more intuitive to select another model. It is different than 2.7.1 though. In 2.7.1 bring up Select Model. Tap a model and the model menu comes up. Tap on Select Model and dialog closes and new model is selected. 2.8 w Jim's change bring up Select Model. Tap a model and dialog closes and new model is selected. No menu displayed. I actually like this, it's quicker to do the thing you bring up Select Model for 95% of the time which is to select another model. A long tap still brings up the menu to delete or duplicate etc. I tried it just using controls and no touch screen and behavior is just as good. I'm all for this change, Thanks Jim

@eshifri
Copy link
Contributor

eshifri commented Oct 11, 2022

What happens with scrolling? Aren't there a big chance to select a new model while you were just intending to scroll through the list?

@ducatiJake
Copy link
Author

I tried the drag scrolling for quite a while and didn't mistakenly select once for what that's worth. I can def see it happening though. From my perspective that's easy to fix, just go back into Select Model and select the desired model.

@raphaelcoeffic
Copy link
Member

raphaelcoeffic commented Oct 12, 2022

What happens with scrolling? Aren't there a big chance to select a new model while you were just intending to scroll through the list?

We could use the same trick as with file preview: one tap selects, the next opens the menu. Normal short click with encoder would open the menu.

@jtaylor2
Copy link
Contributor

I don't think selecting a model by mistake while scrolling will be a significant problem, it hasn't been in my testing. And as @ducatiJake said if you do select a model by mistake just go back into model select and do it again. To my way of thinking selecting the wrong model while scrolling is less of a problem than hitting the wrong line in the menu and either creating a new model in 2.7 (which I seem to do often) or duplicating a model in 2.8. I think that most of the time people go into Select Model they want to select a model so make it as quick and easy as possible.

I'll submit a pull request based on my diff where press on an icon selects the model and long press brings up the menu and we can see what people think.

@eshifri
Copy link
Contributor

eshifri commented Oct 12, 2022

What happens with scrolling? Aren't there a big chance to select a new model while you were just intending to scroll through the list?

We could use the same trick as with file preview: one tap selects, the next opens the menu. Normal short click with encoder would open the menu.

I believe the whole point of this is to select on the first click and remove "select model" from the menu.

@Eldenroot
Copy link
Contributor

It will bring issues with scrolling and safety issues... not a good idea.

@eshifri
Copy link
Contributor

eshifri commented Oct 12, 2022

Maybe vice versa: long press to select a model - short press to open the menu?
Lower chance for mistake.
I kind of agree that selecting a model through the menu is not ideal.

@Eldenroot
Copy link
Contributor

There should be some confirmation screen on model selection... you wanna select another model -> confirmation page is displayed and then you have to hold "roller button"/"enter" button or touch the screen for lets say 5 second to make it confirmed.

@raphaelcoeffic
Copy link
Member

I created #2533 to reproduce the original behaviour, plus a small addition that should protect against accidentally triggering the menu when scrolling with touch control.

@jtaylor2
Copy link
Contributor

My idea was to make selections fast and easy by just clicking on the icon without going thru a menu with the inherent problem of finger checking the wrong menu entry and creating or duplicating a model by mistake.

Since it appears the consensus is worried about selecting a model by mistake and wants to do model selection from a menu and @raphaelcoeffic has a pull request to restore 2.7 behavior I'll close my pull request and revisit doing a direct select with Long Press after his is merged.

@ducatiJake
Copy link
Author

I have to agree with Jim, I built and tried this branch, and I don't really see it as an improvement. As confusing as it was to long press to make a selection, it may be even odder to have to tap twice to then get a menu that I have to tap a third time just to select a model. Really the default action of the Select Model dialog I suspect. There are not a lot of UI components that do one thing the first time you tap them then something different the second time tapped. Not complaining here, just disagreeing. You guys are doing great work and I am super grateful for you picking up the mantle of open source from the OpenTX guys. I was just trying to help by pointing out something I thought would confuse a lot of others out there.

@pfeerick
Copy link
Member

I just realised after using the normal main branch behaviour that it wasn't as bad as I thought before... You actually only needed to long press on a model to bring up the menu, you don't need to select/tap it first. So it's actually two presses to change model - long press it if you can see it, "select model". Not entirely sure how the PR Raphael did changes that as havnt tried it yet.

I think a later improvement could be optional press and hold to switch model, with configurable delay time (min one second), which also would have some sort of onscreen animation while the delay time was being served. I. E. Some touch UIs show an animation while press and hold to indicate a second function will trigger if you keep holding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants