Menu class refactor, created class MenuItem

[tadeubas]

Description

Menu items were tuples without the possibility of checks and reusable code, now the MenuItem class is used for the Menu.

Now when you access a menu with a disabled item, a dynamic check can be performed when clicking on the item. This is useful in case you are without an SD card and when accessing a menu with a disabled item that uses the SD card, you can insert it and then click on the disabled item and it will work as expected.

What is the purpose of this pull request?

• [ ] Bug fix
• [ ] New Feature
• [ ] Documentation update
• [x] Other

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 96.80851% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.20%. Comparing base (8166a9e) to head (bf2c0eb). Report is 125 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/krux/pages/__init__.py	93.33%	3 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #449 +/- ## =========================================== - Coverage 94.21% 94.20% -0.01% =========================================== Files 59 59 Lines 7259 7288 +29 =========================================== + Hits 6839 6866 +27 - Misses 420 422 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[odudex]

In my opinion, Jeff's method of structuring menus with tuples as entries is very clear, objective, and concise. Every developer has their own background and preferences. Perhaps the debate here is between objectivity and sophistication? Personally, I prioritize RAM performance, and I'm concerned that a trend towards wrapping things in classes could lead to excessive overhead. However, naturally, each case must have its trade-offs considered. That's why I would like to have more opinions on this. @jdlcdl , what's your take? (You also have the right to not having one too :grin: )

[tadeubas]

Would you like to hear from Jeff or even another developer? I think I've seen this opinion-based behavior before https://github.com/selfcustody/krux/pull/383#issuecomment-2096175652

The point I see is that your low-level programming experience makes you look for optimizations, often pointless ones. Like here, where you complained about a possible "HW cost" and again about "additional lines of code" https://github.com/selfcustody/krux/pull/395#issuecomment-2123153417 and here when you forced me to write "optimized" code for rounding (just to avoid using standard libraries, which are already written in an optimized way at low level) https://github.com/selfcustody/krux/pull/390#discussion_r1600231592

Now, regarding the menus, what is the biggest one we have, does it have 15 items? If it is discarded before the feature uses the memory, what is the logic in "saving some bytes"? Is it better for a programmer to know that he will create a Menu with tuples (go figure out in the code what the tuples need and do), or with MenuItems that accepts a label, does an action and can be enabled? We could program in C, but why are we using Python?

Again, I emphasize, it seems to be more a matter of opinion and belief than logic, programming, and computer science, to the point where it seems personal.

[odudex]

To me, it appears contradictory that seeking others' opinions is considered a sign of taking things personally.

[jdlcdl]

It's hard for me to decide on whether a 2-tuple as menu-items is more clean than a MenuItem instance that is also clean and a bit more clear. This is more about an expressed "enabled/disabled" property than having a method of "< Back"???(btw: I like that this is now a part of the menu itself and not another item to add to each menu (even if we've added that param to each Menu() call)). I love the UI of disabled items still being present/visible and not otherwise changing what I expect to see (other than I cannot get to it); things I expect to be there are always in the same place.

I hope that 32 extra bytes required in ram, even if 12% more costly, will never stop us from making a code improvement of readability/maintainability/clarity. On this thought, thinking of the sdcard explorer where a folder might have hundreds or thousands of entries -- so my opinion might bite in that case. I'm not sure how that works currently; do all of the MenuItem instances get created up front and each page "pages" thru that list of MenuItems... or could it just be the list from listdir() held and the MenuItems added to each page? If not already done that way, it seems possible for large menus not to be a problem and 32 extra bytes per item per page is negligible (how many items per page is reasonable... likely not more than 7-10 at the extreme high-end for UI that is probably too busy already???)

"None" as the second item in a menu item tuple seems already clear that its disabled, but the "False" enabled param is even more clear. While there are a few more lines in this pr, it's just the definition of MenuItem and MenuItemSD which are more lines... where they are used, it's just an added "MenuItem" prefix and sometimes an added enable/disable argument which feels clean and expressive, and in a few cases, using Menu and MenuItem results in fewer lines and cleaner code.

I'm going to lean ever-so-slightly to the side of "this refactor pr is a net benefit", even if it requires optimizing very large menus like large folders on sdcards that we cannot possibly predict (if that indeed could be a problem).

[tadeubas]

Managed to reduce the size for the MenuItem instance using defaults. That way it is smaller than the tuple always defining two lambda functions.

Results: 10 tuples = 2784 bytes 10 MenuItem = 2464 bytes

Now MenuItem is 32 bytes LESS than a tuple.

Tested with the latest commit and the code below:

# TESTING MEMORY USAGE
from krux.pages import MenuItem

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [MenuItem("label " + str(i))]

print("a10 m, ", gc.mem_free())

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [("label " + str(i), lambda: None, lambda: True)]

print("a10 t, ", gc.mem_free())

l = []
gc.collect()
print("b, ", gc.mem_free()) 

[odudex]

Thank you @jdlcdl for the deep analysis! We're heading towards a consensus here. @tadeubas , please don't feel offended by my questions. Their aim is to reduce my ignorance, not attack you. While writing, I now saw you managed to improve efficiency beyond original. That's awesome! Congratulations! And I believe there's nothing more to question!

[tadeubas]

Just wait a little to merge this PR, I will make it draft to do some other changes/optimizations

[odudex]

Sure, when you say it's ready, I'll do some tests and then merge!

[tadeubas]

Results: *tuple(2) is a tuple with 2 items, and so on...

I've noticed that we are talking about so little ram space that the memory chip behaves differently depending on the actual state of RAM (that depends on what was executed before).

And the latest test was not a fair comparison, because for the most time we used tuple(2), and now the equivalent MenuItem. I've tested again (with latest commit) and found we didn't won or lose any byte

[tadeubas]

Results (first test differs from the rest):

Test code:

# TESTING MEMORY USAGE
from krux.pages import MenuItem, MenuItemSwitch

print("TEST")
print("A1- MenuItem x tuple(2)")

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [MenuItem("label " + str(i))]

print("10 menuitem, ", gc.mem_free())

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [("label " + str(i), lambda: None)]

print("10 tuple, ", gc.mem_free())

print("TEST")
print("A2- MenuItem x tuple(2)")

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [MenuItem("label " + str(i))]

print("10 menuitem, ", gc.mem_free())

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [("label " + str(i), lambda: None)]

print("10 tuple, ", gc.mem_free())

print("TEST")
print("B1- tuple(2) x MenuItem")

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [("label " + str(i), lambda: None)]

print("10 tuple, ", gc.mem_free())

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [MenuItem("label " + str(i))]

print("10 menuitem, ", gc.mem_free())

print("TEST")
print("B2- tuple(2) x MenuItem")

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [("label " + str(i), lambda: None)]

print("10 tuple, ", gc.mem_free())

l = []
gc.collect()
print("b, ", gc.mem_free())

for i in range(10):
    l += [MenuItem("label " + str(i))]

print("10 menuitem, ", gc.mem_free())

[tadeubas]

I think this PR is done, fell free to suggest and/or do some changes and optimizations

[tadeubas]

And @jdlcdl is right. We create a list of all the items in the current folder on the SD card and pass it to the Menu, which creates a ListView with all of them. There is plenty of room for optimizations in filemanager.py

[odudex]

Guys, I did my own tests here and would like to share with you.

As inputs, I used 3 branches: develop (selfcustody) menu_item (by Tadeu) efficient_memory_translations (by Tadeu)

Method: Changed language to Portuguese Uncommented my memory gauge on page/init.py

        self.draw_ram_indicator()

    def draw_ram_indicator(self):
        """Draws the amount of free RAM in the status bar"""
        gc.collect()
        ram_text = "RAM: " + str(gc.mem_free())
        self.ctx.display.draw_string(12, 0, ram_text, GREEN)

Verified available RAM at boot and at "home", after login in with manual TinySeed, 12 words, untouched bits.

And here are the results:

Before drawing any conclusions, I would appreciate it if you could review my testing method to check for any potential flaws.

[tadeubas]

In my opinion your test is good because it used the entire application and was a real use case, but it is too broad to draw conclusions. I noticed, as I mentioned above, that running the same test twice can yield different results depending on the previous memory state, even in small and simplified code.

I was going to test the efficient_memory_translations PR in controlled code to see what happens. I think it could be improved as well.

[odudex]

Yes, memory fluctuates, also because MicroPython doesn't fully "unload" modules. So if you'd like to reproduce the numbers, the first measurement should be taken directly after boot, and the second after loading the modules exactly as I did, without any other random navigation.

[jdlcdl]

I like this ram_indicator. I'll use this regularly to temporarily-merge into any pr/branch that I'm playing with.

[tadeubas]

Hey guys, I think this PR be merged, right?

[odudex]

Refactoring that compromises performance, such as a reduction of 1.5KB in free memory at boot that escalates with the creation of additional menu instances, is a direction I am unwilling to follow. But if you and @jdlcdl agree it pays of, it will be merged.

[tadeubas]

I didn't noticed the increase in memory, I thought it was a decrease! I will check this and make some other tests

[odudex]

Right, please let me know if you can't reproduce the test I did where available RAM is reduced by 1.5KB right after boot, and this delta(using develop branch as reference) is increased upon using the device.

[tadeubas]

RAM at the menu screen, sequence executed from left to right in one boot only – Yahboom (locale was pt-BR)

	Login (after boot)	Tools	Tools > generate QR code - str: a	Settings	Settings > HW > Printer > CNC	Load mnemonic	Load mnemonic > Manual > Tiny Seed	Load mnemonic > Manual > Tiny Seed > 12w > Load Wallet	Home (right after load)	Backup	Backup > Other formats > Number
develop	1429728	1425088	1421056	1417664	1409920	1420640	1413248	1375168	1378400	1375520	1371840
menu_item	1428224	1423264	1419072	1415936	1407648	1419072	1411072	1373024	1376288	1373376	1369408
diff	1504	1824	1984	1728	2272	1568	2176	2144	2112	2144	2432

[jdlcdl]

1.5k seems like negligible, but to think it's all because of menu items (and not even 10 in most cases), it seems considerable for what is gained. I too thought that we'd see a reduction with latest changes to this pr.

To be honest, I have not paid such close attention to memory usage... ever. I'll be merging the following branch... https://github.com/selfcustody/krux/compare/develop...jdlcdl:krux:debug-RAM ... into whatever I'm testing from now on (then git reset --hard to get rid of it) so that I can get a feel for what to expect. But it's less than ideal and I'd hardly notice anything... it's mostly showing me 3 digits of precision + whatever was recently collected (because I want to see what tools are using while they do their work). That branch also does away with any gc.collect() calls (until I see that something breaks) so that I can actually see what was used.

Im afraid I don't have such a strong opinion FOR this branch while the ram cost seems so noticeable, but I await Tadeubas' next commit that might resolve to a ram savings (as I recall it did recently).

[odudex]

To be honest, I have not paid such close attention to memory usage... ever. I'll be merging the following branch... develop...jdlcdl:krux:debug-RAM ...

I liked your RAM indicator! Please do a PR. We could leave it commented (as we don't have #ifdef in python)

[jdlcdl]

Please do a PR

ok, I've submitted a PR that includes my ram indicator, which was actually yours (with a change that diminished precision).

I'm always happy to issue a PR, but please...

LET IT BE KNOWN,

That cutting/pasting from any of my public repos, or pulling a commit if that works, or re-using any of it with our without attribution is hereby permitted forevermore without limitations.

[tadeubas]

While this code adds a few KB (which is negligible), it adds a lot more positive things, improving the design, structure, readability of the code, and reducing the complexity of the Krux code. We have more quality code with this PR than before. So I think it should be merged. We've pointed out the PSBT functionality as the only one that needs a lot of RAM, and we'll isolate the signing process to have the most RAM (which will exclude any negligible KB that this PR might add).

[tadeubas]

I see I'm just a voice crying out in the desert, I'll close the issue