search

trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.36k stars 661 forks source link

Menu item text gets hidden if it is too long #4019

Closed matejcik closed 1 month ago

matejcik commented 4 months ago

Describe the bug problem: image

after a local hacky fix: image

offending code: https://github.com/trezor/trezor-firmware/blob/700512901b822742759e677ae59fbaef04417842/core/embed/rust/src/ui/model_mercury/component/button.rs#L624-L635

Why do we even have this? ISTM we should default to showing both icon and text in all cases.

To reproduce

trezorctl btc get-address -n m/44h/0h/0h/0/0 -d then click the menu

matejcik commented 4 months ago

UI tests pass with the local hack, which just unconditionally runs the first branch in the above code. This tells me that the other branches are dead code.

obrusvit commented 3 months ago

Why do we even have this? ISTM we should default to showing both icon and text in all cases.

Taken from model_t UI.

UI tests pass with the local hack, which just unconditionally runs the first branch in the above code. This tells me that the other branches are dead code.

Or rather the menu is not walked-through in UI tests?

ibz commented 1 month ago

Indeed, not all menus are tested.

But what is worse? I don't see a case for a menu item with no text - a menu item with incomplete text is certainly better.

So, I see 3 issues here:

1) Is there a case against showing the text all the time? If not, maybe we should bite the bullet and just do it? 2) What should we do if we detect that text does not fit? (can we even detect that - easily - in code?) I would argue that - if possible - we should probably just somehow crash (only in testing / emulator?) - that would make the CI pipeline fail, and would make us shorten the strings. Probably a pain, but we would do it once and know that it is done. 3) Increasing test coverage is something we should be constantly striving for, but unrelated to this issue - so that should not hold us back from actually fixing this issue which is quite bad.

matejcik commented 1 month ago

Is there a case against showing the text all the time? If not, maybe we should bite the bullet and just do it?

agreed, or @mmilata @obrusvit do you know of a case where we want to render icon without text?

we should probably just somehow crash (only in testing / emulator?) - that would make the CI pipeline fail, and would make us shorten the strings. Probably a pain, but we would do it once and know that it is done.

we can crash in debug mode, if detecting this is easy at run-time. i'm not sure about that, @obrusvit @mmilata can you advise?

mmilata commented 1 month ago

do you know of a case where we want to render icon without text?

nope

we can crash in debug mode, if detecting this is easy at run-time

Seems to be easy in this particular case since we already have the widths in variables? For general case we'd probably have to extend the text renderer. Or perhaps graphics driver to detect drawing off-screen, but that sounds like it will have its own problems.

ibz commented 1 month ago

Seems to be easy in this particular case since we already have the widths in variables?

Right, it was a silly question from my side to start with. That's what the if in question does - detect when the text does not fit and hide the text. So it knows it does not fit.

ibz commented 1 month ago

On the other hand, I am thinking what is the gain of crashing if the menus are not walked-through by the tests / CI anyway.

There is a very slim chance somebody will manually get to that point while testing, and if they do, they will still notice the menu looks bad, no? Crashing at that point will just annoy somebody that is trying to do something unrelated.

So maybe the best way is to simply show the incomplete text?

Hannsek commented 1 month ago

So maybe the best way is to simply show the incomplete text?

Most probably, as on every other places.

matejcik commented 1 month ago

On the other hand, I am thinking what is the gain of crashing if the menus are not walked-through by the tests / CI anyway.

a lot of tests actually do involve menus

the point of this is to crash if a translation causes a menu item to not render; we don't really check all languages screen by screen, so the crash will indicate that some translation string doesn't fit and we need to change it

ibz commented 1 month ago

a lot of tests actually do involve menus

Yes, but then shouldn't you have seen some UI tests diffs when you disabled the other branches?

matejcik commented 1 month ago

Yes, but then shouldn't you have seen some UI tests diffs when you disabled the other branches?

i didn't run all the languages

ibz commented 1 month ago

OK, I'll do that then and see what happens. The way it is now is very bad, anyway.

obrusvit commented 1 month ago

Alternative is implementing multi-line texts. As was also desired by Figma designs. See e.g. image

I think the combination of approaches would be the best if feasible:

  1. try to fit on one line
  2. if fails, try to fit on two lines
  3. if fails, crash in debug, show incomplete text in prod
Hannsek commented 1 month ago

@lapohoda I think we can do the multiline items, right?