fpv-wtf / wtfos-configurator

Configurator for wtfos, with built in margerine
GNU Affero General Public License v3.0
42 stars 16 forks source link

Add package config support for ui-schema #342

Closed benlumley closed 1 year ago

benlumley commented 1 year ago

This allows the use of the ui schema part of react json schema form - it's like an extra bit of schema to define the rendering of the form (the main schema is about the data structure) - https://react-jsonschema-form.readthedocs.io/en/v1.8.1/form-customization/

My specific use case is to use the ui:help element to add these help texts underneath - as underlined here:

Screenshot 2023-04-14 at 15 18 11

I want to use them to add some explainers for msp-osd config.

Questions/decisions:

I've added the stripUISchema method; which strips out any properties prefixed with ui: from the regular schemaV2.json; allowing both ui:schema and regular schema to coexist in one file - but be split out and fed in to rjsf separately.

This felt like the simplest way initially - avoids burdenning package authors with managing two separate schema files.

Not that I've done it and understood the complexity of stripping this out (owing to the various ways fields can be nested/dependent) - I'm not super sure that this is the best idea, and am wondering whether sticking with the separate file might not be simpler....

Thoughts on that appreciated!

Sample json (see the ui:help keys) ```` let conf = { "title": "MSP OSD", "description": "Configure the OSD", "type": "object", "units": [ "msp-osd-goggles" ], "required": [], "properties": { "show_waiting": { "type": "boolean", "title": "Show Waiting Message", "ui:help": "here ere", "description": "", "enum": [ true, false ] }, "show_au_data": { "type": "boolean", "title": "Show VTx Temperature and Voltage", "description": "", "enum": [ true, false ] }, "rec_enabled": { "type": "boolean", "title": "Enable OSD recording", "description": "", "enum": [ true, false ] }, "rec_pb_enabled": { "type": "boolean", "title": "Enable OSD playback", "description": "", "enum": [ true, false ] }, "hide_diagnostics": { "type": "boolean", "title": "Hide diagnostic information", "description": "", "enum": [ true, false ] }, "fakehd_enable": { "type": "boolean", "title": "Enable FakeHD Mode", "description": "", "enum": [ true, false ] } }, "dependencies": { "fakehd_enable": { "oneOf": [ { "properties": { "fakehd_enable": { "enum": [ false ] } } }, { "properties": { "fakehd_enable": { "enum": [ true ] }, "fakehd_hide_throttle_element": { "type": "boolean", "ui:help": "here ere2", "title": "FakeHD hide throttle element", "description": "", "enum": [ true, false ] }, "fakehd_lock_center": { "type": "boolean", "title": "Lock FakeHD to centered mode", "description": "", "enum": [ true, false ] }, "fakehd_menu_switch": { "type": "number", "title": "FakeHD menu switch character", "description": "", "minimum": 1, "maximum": 512 }, "fakehd_hide_menu_switch": { "type": "boolean", "title": "FakeHD hide the menu switch", "description": "", "enum": [ true, false ] }, "fakehd_columns": { "type": "string", "title": "FakeHD column config", "description": "", "oneOf": [ { "const": "T", "title": "Top" }, { "const": "M", "title": "Middle" }, { "const": "B", "title": "Bottom" }, { "const": "S", "title": "Split" } ] }, "fakehd_rows": { "type": "string", "title": "FakeHD row config", "description": "16 characters; each one of LCRWTFD", "maxLength": 16, "minLength": 16, "pattern": "^[LCRWTFD]{16}$" }, "fakehd_layout_debug": { "type": "boolean", "title": "FakeHD layout debug", "description": "Undocumented feature to fill the grid with numbers for layout debugging; remember to disable", "ui:help": "bob", "enum": [ true, false ] } } } ] } } } ````

Draft for now... thoughts on the above point re integrated vs separate files appreciated.

And then I also want to add a way to put links in (so can link directly to full docs where useful from the help texts).

stylesuxx commented 1 year ago

I think having a separate file with a proper structure will prevent parsing headaches further down the line.

We have to think about the Links though. I would prefer them to be an extra field since I do not want to render random HTML provided by package maintainers. So maybe we could add a custom widget to render Links where one can provide a URL and a link text and then append those Links after the help text.

Alternatively we could parse the help text as Markdown - at least it would be a bit more limiting than just rendering unknown HTML.

benlumley commented 1 year ago

I think having a separate file with a proper structure will prevent parsing headaches further down the line.

The more i think; the more i agree. making a support issue for myself here. so i will rework that.

And re the other bit.... I've looked at how it works / what args are available - I'm pretty sure I can create a new renderer for the help, and it can then also pluck out and render a ui:helplinkurl / ui:helplinktext property to append the optional link. I also don't wanna be dangerouslysettinginnerhtml with stuff from a package json file 😨 (or somehow allowing JSX, which is what the internet suggests). Relevant stuff: https://rjsf-team.github.io/react-jsonschema-form/docs/advanced-customization/custom-templates/#fieldhelptemplate

So I'll get amends made and report back.

j005u commented 1 year ago

i'm all for Markdown support where it makes sense, including rendering the package README on the details page.

a) markdown should be safe given a proper renderer b) what can a package dev do with code injection into fpv.wtf that they can't already do with root code execution on the goggles? it's not like we have any secret keys or session ids to steal?

i'm not at all dismissing the concern, nor am I saying b) is an excuse to just give them a way to load custom JS or something, that's just the view i've come to after considering this topic a few times already.

stylesuxx commented 1 year ago

b) Yes, you are right in regards to having full root access anyway. Still I think giving people full access to HTML means great potential for all kind of formatting fuckups (intended or not). So providing the ability to use Markdown we can apply this to multiple parts - some of the translations for example also include links and I would prefer to have that as Markdown instead as to for example splitting the string in 3 parts or rendering HTML.

j005u commented 1 year ago

I actually removed the tag from Brian, he's already answered it's fine once.

And I just realized, we know of another website that lets anyone upload arbitary markdown and shows it all over the place. While at the same time having extremely sensitive secrets in it.

This one lol.

benlumley commented 1 year ago

Sample uiSchemaV2.json for testing against msp-osd (/opt/etc/package-config/msp-osd/uiSchemaV2.json)

{
  "show_waiting": {
    "ui:help": "test"
  },
  "show_au_data": {
    "ui:help": "test2"
  },
  "fakehd_rows": {
    "ui:help": "row help"
  }
}
benlumley commented 1 year ago

A uiSchemaV2 with markdown in:

{
  "show_waiting": {
    "ui:help": "test"
  },
  "show_au_data": {
    "ui:help": "# A title\n\n## a subtitle\n\n[and a link](https://www.google.co.uk)"
  },
  "fakehd_rows": {
    "ui:help": "row help"
  }
}
Screenshot 2023-04-20 at 00 28 51
benlumley commented 1 year ago

Thank you, looking very good - just a couple of nit-picks and a question.

Thanks - nice to get a code review, mostly work my by myself - thanks for taking the time. I'll get those all actioned.