flipperzero-firmware icon indicating copy to clipboard operation
flipperzero-firmware copied to clipboard

Gui: Submenu and VarItemList API additions and improvements

Open Willy-JL opened this issue 4 months ago • 5 comments

What's new

General improvements:

  • Can have header in Variable Item List
  • Can have a vertical Submenu
  • Can lock items in Variable Item List and Submenu
  • Variable Item List label and value text will scroll if too long
  • Variable Item List value arrows shrink if value is < 4 characters
  • More freedom and usefulness in Variable Item List API
  • Cleaned up some parts of the code and some magic numbers are now defined
  • Feature parity between submenu and variable item list, only differences now are that:
    • variable item list item ID is based on index of addition, submenu item ID is based on user-supplied 32bit value
    • variable item list can edit item values, submenu only allows picking one (by design of the widget purpose)
    • variable item list items can be edited after creation, submenu items cannot (by design of the widget purpose)

API additions (no removals, fully backwards compatible):

  • submenu_add_lockable_item():
    • based on #2289 which was rejected, CFWs merged it and improved it over time
    • theres a few apps that use it by now
    • example usecase: highlighting that some features are not available in current configuration / state
    • and even more useful in varitemlist version below
  • submenu_set_orientation():
    • can have vertical submenu (user's preference for ux)
    • same concept works great in infrared app, could be reused there to make more of the app vertical instead of having to keep switching
  • variable_item_list_get():
    • get and allow editing an item after creation
    • example usecase: editing one option changes state of other entries in the list, like if they are locked due to incompatible settings or if they have different labels or values
    • Sub-GHz app is already doing this (though in a very hacky way) here, to change frequency text to --- when hopping is enabled, by storing the item pointer in scene state, this is also quite error prone, with varitemlist get() it would be safer and clearer
    • for example i have a an app menu that edits an array, you select an index by scrolling on first item, second item removes that index when clicked, third item adds a new item when clicked, this needs to edit the first item to show what index is selected after remove/add
  • variable_item_list_set_header():
    • feature parity with submenu widget
    • example usecase: headers are pretty but want editable options not just a list
  • variable_item_set_item_label():
    • allow changing label after adding item
    • example usecase: want to show index of item, can have value shown and put (1/10) in label dynamically, like in my example above in get()
    • more too probably, allows more creativity
  • variable_item_set_locked():
    • same as above for submenu, based on same code but ported to varitemlist by me
    • also allows setting locked state after the fact, remembers locked message so after creation you can toggle with set_locked(item, false/true, NULL)
    • example usecase: some settings combinations in this screen are incompatible, like GPIO pins overlapping when configuring

Acknowlegments:

  • Original submenu lockable items code by @giacomoferretti in #2289
  • Vertical submenu and some code improvements by Unleashed CFW team
  • Most other edits by me @Willy-JL
  • Hope I'm not forgetting someone else that was involved

Verification

  • Check apps that use variable item list and submenu to be working correctly

  • No dedicated example app for additional APIs but some apps do use them, for example:

    • Magspoof is using both submenu/varitemlist lockable items and varitemlist get() to dynamically edit a config menu based on allowed combinations of GPIO pins
    • NFC Maker is using locked submenu items when choosing NTAG card type to show if payload is too big for some types
    • XRemote is using submenu vertical orientation in a similar fashion to infrared app
    • FindMy Flipper is using varitemlist header in its tag setup pages
    • BLE Spam is using varitemlist get() to edit which subarray is shown, one option chooses a type and next option chooses allowed values for that type, editing first one needs to update second one
    • An edited version of Mass Storage uses varitemlist header in disk image creation screen which allows setting image name and size
    • Some CFWs show hidden debug options as locked so the user knows they are available but they probably should not use them, keeping them secret could be detrimental to some advanced user who might need them (for example: lfrfid read raw)

Checklist (For Reviewer)

  • [ ] PR has description of feature/bug or link to Confluence/Jira task
  • [ ] Description contains actions to verify feature/bugfix
  • [ ] I've built this code, uploaded it to the device and verified feature/bugfix

Willy-JL avatar Apr 30 '24 05:04 Willy-JL

This is in response to developer frustration with inconsistent APIs first-hand from @zacharyweiss when recently updating his Magspoof app, as well as discussion with @skotopes about differences between forks' APIs. As the lead maintainer of one such fork I've developed a decent amount for Flipper in the last year, some of which resides in additional APIs which I deemed useful. My past with the great people of OFW has been rocky to say the least but I wish to help the community and this wonderful project in whatever way I can, and while backporting all of the custom features would mean my fork wouldn't have a purpose anymore, I think (and aku seems to agree) that atleast having a consistent API would be a great start. This is some of that, but I'm planning to go through the rest too. One step at a time )

That being said, I'm aware of the comments made for example in #2289 about wanting a small and simple toolkit, and some (if not all) the changes in this PR would go against that wish. I simply ported and cleaned up the most developed submenu and varitemlist implementations that I'm aware of, then we can discuss here which parts (if any) would fit OFW's philosophy.

I also just realized that there's a few more things that could be added, namely:

  • a way to remove items from varitemlist, this would allow full control without reset() and adding all the items back
  • same goes for submenu to remove an item
  • having submenu scroll the text for long labels too

Willy-JL avatar Apr 30 '24 05:04 Willy-JL

I noticed that I had ported without the centering for varitemlist value text, so it was messed up. Added the extra elements APIs, one of which was needed for centered scrolling text in varitemlist value text.

Again, this is to port over APIs in hopes of making them consistent across forks, but I totally understand if you don't think these belong in OFW

Willy-JL avatar May 03 '24 09:05 Willy-JL

Is it possible to split it into separate features?

I'm ready to merge some parts immediately and some I'd like to discuss more.

skotopes avatar May 16 '24 16:05 skotopes

@skotopes sure I can do that, which parts should I split?

Willy-JL avatar May 16 '24 20:05 Willy-JL