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

Gui: Submenu and VarItemList API additions and improvements #3621

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

Willy-JL
Copy link
Contributor

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 Add lockable capability to Submenu module #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:

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

API additions:
- `submenu_add_lockable_item()`: based on flipperdevices#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)
- `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)
- `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, also probably more, 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)

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 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

Acknowlegments:
- Original submenu lockable items code by @giacomoferretti in flipperdevices#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
@Willy-JL
Copy link
Contributor Author

Willy-JL commented Apr 30, 2024

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
Copy link
Contributor Author

Willy-JL commented May 3, 2024

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

@hedger hedger added UI Affects UI New Feature Contains an IMPLEMENTATION of a new feature Core+Services HAL, furi & core system services labels May 8, 2024
@skotopes
Copy link
Member

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.

@Willy-JL
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core+Services HAL, furi & core system services New Feature Contains an IMPLEMENTATION of a new feature UI Affects UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants