search

EdgeTX / edgetx

EdgeTX is the cutting edge open source firmware for your R/C radio
https://edgetx.org
GNU General Public License v2.0
1.61k stars 339 forks source link

Fix proper focus #3901

Closed JimB40 closed 11 months ago

JimB40 commented 1 year ago

Is there an existing issue for this problem?

What part of EdgeTX is the focus of this bug?

Transmitter firmware

Current Behavior

screen-2023-08-04-083224

Focus on variuos UI interactive elements is now tiny frame. @philmoz have you changed anything in global LVGL UI elements style definitions?

Expected Behavior

232181445-be0970ee-2042-42e0-898a-bccc71aa2c87

Steps To Reproduce

Flash 2.10 nightly

Version

Nightly (Please give date/commit below)

Transmitter

Radiomaster TX16S / TX16SMK2

Operating System (OS)

No response

OS Version

No response

Anything else?

No response

philmoz commented 1 year ago

Focus was changed in the libopenui refactor PR to be more consistent (along with size, border and rounded corners).

Controls that can be both active and focused used the border color to show focus. That meant there were two types of focus - border or fill.

philmoz commented 1 year ago

Keep in mind one of the main reasons for the libopenui refactor is to set up a better framework for 3.0.

JimB40 commented 1 year ago

Bit confused Phil. There are style-sheets for UI elements prepared that include NORMAL, FOCUSED, ACTIVE and ACTIVE-FOCUSED states. in case of above mentioned elements. https://github.com/EdgeTX/edgetx/issues/3225

As principal UX rule consistency can't override visibility. Focus as inversed field is well defined and visible while tiny frame is not. If there are tougher cases like Lists items or groups that uses tiny frame we shoud find a way to change those cases and not to equalize everything to worse case.

AFAIU libopenui now is under our full control so I can't see the point.

If there are implementation problems (because of how code is written) I'm ready to discuss it to find best solution but not to sacrifice principals.

pfeerick commented 1 year ago

Before we could even contemplate looking at the stylesheets, the code needed to be unified so things weren't been formatted and modified on every other page. Now that that has been done, starting to implement the stylesheet and progressing on to the 3.0 UI should be easier.

Your job @JimB40 is to be starting to pull out these details, and pointing to the stylesheet to show how you want it to look, and it'll either happen or you'll be told it's not feasible. ;)

JimB40 commented 1 year ago

Hmm... Never seen case in my UX/UI career that "code unification" made styling not feasible. But then okay. Wait and see.

At least EDIT state was reclutant to "refactor" or whatever and is no lookin like this Input Edit packed

pfeerick commented 1 year ago

Hmm... Never seen case in my UX/UI career that "code unification" made styling not feasible.

🤣 And who said that, other than you? 😛 You'll be told it's not feasible because of resource constraints like memory, processing time, ease of implementation, etc. But remember, baby steps... can't get to the style stuff, until the foundations are ready to actually do it. ;)

At least EDIT state was reclutant to "refactor" or whatever and is no lookin like this

It depends on what you call "EDIT state" - it helps if you're just going to pull screenshots out to highlight a specific area of interest - do you mean the entire dialog? The input field for "input name" seems to be in focus, but not being edited? If you mean the latter, then I thought that was almost exactly what it looks like...

image

If you mean the entire edit dialog, no, the refactor was not related to that at all. For that, you need to work on trying to get https://github.com/EdgeTX/edgetx/pull/2939 how you want it. I stress try ... you will not necessarily get it exactly how you want it.

JimB40 commented 1 year ago

EDITED state is how UI element looks when user does editing. It's desctibed in UI element styling sheet (if UI element has EDITED state) like #3223 So FOCUSED in now tiny frame around while EDITED remained green inversion after "refactor" or whater it was done.

philmoz commented 1 year ago

Border focus indicator is not something new I have added. I've just made it more consistent.

I'm not sure why this is causing so much angst.

Examples from 2.8: Screenshot 2023-08-04 at 9 08 36 pm Screenshot 2023-08-04 at 9 08 51 pm Screenshot 2023-08-04 at 9 09 09 pm Screenshot 2023-08-04 at 9 09 15 pm Screenshot 2023-08-04 at 9 09 27 pm Screenshot 2023-08-04 at 9 09 40 pm Screenshot 2023-08-04 at 9 09 54 pm Screenshot 2023-08-04 at 9 10 05 pm Screenshot 2023-08-04 at 9 10 11 pm Screenshot 2023-08-04 at 9 10 20 pm

JimB40 commented 1 year ago

Because inversion is far better visible in direct sun, and that is where I often use radio. If that was used on compuer screen only I woultn't even bother mysef to write a single line.

pfeerick commented 1 year ago

OK, so you meant "Text Input field EDIT state" ... gotcha. image

So, if we focus on just the one input field type... what you are calling "Text Input field", it currently looks mostly like the example ENABLED state.

But when it's in the edited state it's filled with EDIT color (already is), border is EDIT color (isn't) and text color is PRIMARY2 (already is). And when it's FOCUSED, rather than it just being an outline, you want the entire control filled with the FOCUS color (isn't), border also FOCUS color (already is) and TEXT PRIMARY2 (isn't)? Correct?

JimB40 commented 1 year ago

Yep. Have idea that would be good to have half and hour in next dev-hour. So I can explain EdgeTX UI System. So if I write later just a name from UI world we both know meaning. So I don't have to explain what UI Atom is :) Screenshot 2023-08-04 at 13 10 55

pfeerick commented 1 year ago

Or perhaps you could learn the names of the UI controls that we use all the time, so we don't need to know two lots of terminology 🤪 But yes, that would be a good idea for the next one, as things are getting to a state where this can start happening.

It's more... keep in mind... how would you explain it if you were face to face... wouldn't you point at what you are talking about? :wink: After all, pulling up a screenshot of the whole screen, and then babbling about EDIT state when in fact you're talking about a particular control isn't quite conveying the message you intended.

JimB40 commented 1 year ago

Somehow to communicate well in ETX project I've learned what is behind "commit" or "rebase" to understand, not asking to use "my language". Learned also how to use GitHub even it was not on my "fun to do" list. Ussualy worked with front-end devs who knew terminology, so it wasn't my task to check if property for padding is actually named "padding" in CSS or whatever they used.

If this is going to be boring-half-an-hour-I-dont-need scratching idea now. After 50 "price" of hour of my life skyrocketed. So I try not make any waste. :)

PS. Have to think over attitude, maybe @philmoz is right I'm causing too much. This is RC hobby project, not commercial one for big brand. Cheers :)

pfeerick commented 1 year ago

I'm causing too much

That I can't agree with at all... as much as I would like to get hold of you and shake you at times (still sure you want that face-to-face meet? 🤣 ) ... it's just about trying to find that common terminology that we all understand. As you say, this is hobby/open source, not a commercial project, so gotta be more flexible ;)

pfeerick commented 11 months ago

I'mma gonna stomp on this issue for now. Bring it up again as part of the 3.0 UI if still want it changed. IMO, this made things more consistent though...

i.e. Before https://github.com/EdgeTX/edgetx/assets/5500713/1e20074e-6a5f-4462-86bd-680708e7c895

After https://github.com/EdgeTX/edgetx/assets/5500713/732e5e1f-f5d9-429e-a980-3c4a785fae9e