Improve menu items to allow templated strings

[seejdev]

Fixed #988

This PR introduces functionality to the menu which allows templated strings to be used within menu item text fields. This enables launcher to fill in data that k2 may not be able to get, and further customize the experience to the individual launcher instance.

I've limited the template data to a few simple fields. We can always add more to the list to be used in future menus.

type TemplateData struct {
	LauncherVersion  string
	LauncherRevision string
	GoVersion        string
	ServerHostname   string
}

The template data is generated by the desktop runner, and applied to either menu updates from the control server, or the default menu. The template expansion is performed on the raw JSON from the control server, so we are able to parse and expand any and all fields, rather than parsing as-needed and per-field. The default menu is created if no there is no existing menu.

[seejdev]

Moving this PR back to draft status while I refactor how template data is injected in to desktop.

[RebeccaMahany]

LGTM!

[directionless]

I'm happy to try this, though I think I have 2 questions.

First, I think this is doing the template expansion in the Desktop process, correct? That feels correct, in that it gets the desktop version, but I think it's going to be harder to pass highly dynamic data through. (Which is mostly only in the daemon) OTOH, maybe we'll drop a bunch of variables, and a template file. 🤷

Second, I think this is doing the templating expansion at each string? Does it make sense to expand once at the initial file read time?

[seejdev]

I'm happy to try this, though I think I have 2 questions.

First, I think this is doing the template expansion in the Desktop process, correct? That feels correct, in that it gets the desktop version, but I think it's going to be harder to pass highly dynamic data through. (Which is mostly only in the daemon) OTOH, maybe we'll drop a bunch of variables, and a template file. 🤷

Second, I think this is doing the templating expansion at each string? Does it make sense to expand once at the initial file read time?

Yes, you're correct the template expansion is handled by desktop process. I hadn't thought about doing that in launcher proper, but it could work and may be a better approach. Launcher proper can easily access more data and handle more dynamic data queries/templates, but the user process could be better suited to handle things like i18n and user-centric data.

I like the idea of expanding once - it simplifies some of the design. In order to do that, I'd have to take the JSON from k2 control server, feed the raw JSON bytes to the template parsing, and then unmarshal the expanded output back into JSON. One potential drawback is that any templating error in any field would cause all of the menu data to be treated as invalid, rather than the one field.

But I still kind of like desktop being more "dumb" and not having to worry about template expansion, etc. I'll see how easy it is to move some of this to launcher proper.

[directionless]

I like the idea of expanding once - it simplifies some of the design. In order to do that, I'd have to take the JSON from k2 control server, feed the raw JSON bytes to the template parsing, and then unmarshal the expanded output back into JSON. One potential drawback is that any templating error in any field would cause all of the menu data to be treated as invalid, rather than the one field.

Yes, I think that's right. Template expand before the json unmarshal. Template errors would produce bad content, but then again, so does bad json. So 🤷 server has to be careful either way

[seejdev]

I like the idea of expanding once - it simplifies some of the design. In order to do that, I'd have to take the JSON from k2 control server, feed the raw JSON bytes to the template parsing, and then unmarshal the expanded output back into JSON. One potential drawback is that any templating error in any field would cause all of the menu data to be treated as invalid, rather than the one field.

Yes, I think that's right. Template expand before the json unmarshal. Template errors would produce bad content, but then again, so does bad json. So 🤷 server has to be careful either way

So I tried going down this path, but it has some wrinkles. My biggest concern was this: performing the template parsing/expansion on raw JSON strings means that the replacement strings in the template data must be JSON-compliant, and any special characters must be escaped. I felt that was a weird restriction to put on the template data, and it made me worried that adding this sort of non-standard manipulation of JSON exposes us to various ways we could produce malformed JSON as a result.

Having launcher desktop to do the template expansion also enables the default/fallback menu to use templates too.

So if we're okay with doing the template expansion in launcher desktop, the second question is should we do it on-demand per-field (how it's working now), or all at once. It probably doesn't make a big difference either way, other than that doing it up front entirely simplifies some design parts where the textParser dependency doesn't need to be passed to as many things.

[directionless]

So if we're okay with doing the template expansion in launcher desktop, the second question is should we do it on-demand per-field (how it's working now), or all at once. It probably doesn't make a big difference either way, other than that doing it up front entirely simplifies some design parts where the textParser dependency doesn't need to be passed to as many things.

Of the 4 choices enumerated (launcher vs desktop and single shot vs per-field) I'm good with whatever you chose.

I suspect doing the parsing as a single shot is going to be both simpler, and more powerful, so I'd bias that way. But, it's pretty easy to iterate.

[seejdev]

So if we're okay with doing the template expansion in launcher desktop, the second question is should we do it on-demand per-field (how it's working now), or all at once. It probably doesn't make a big difference either way, other than that doing it up front entirely simplifies some design parts where the textParser dependency doesn't need to be passed to as many things.

Of the 4 choices enumerated (launcher vs desktop and single shot vs per-field) I'm good with whatever you chose.

I suspect doing the parsing as a single shot is going to be both simpler, and more powerful, so I'd bias that way. But, it's pretty easy to iterate.

I made the change to expand templates up-front and as a single-pass of the entire menu data. I'm happy with this approach, as it reduces complexity in the desktop process, reduces dependencies of the menu itself, and we get the nice property that any templated data added to menus in the future will automatically get parsed with out additional launcher changes (other than adding new templated fields).

[James-Pickett]

template dohicky is really cool and from stdlib ❤️

[directionless]

Looks really clean. Some nits.

[directionless]

I'm fine with this. But... Since Execute takes an io.Writer, I'd probably move that up to the method call. Then you get to do things like return t.Execute(w, tp.td). I suspect callers will get simpler too

[directionless]

Not totally sure what I think. I'll go with it, but it feels a little tangled using menu.InitialMenu here. I think to use that, we need to add in the defaults. Which was the purpose of calls like newInitialMenuData.

So this is going to write a partial menu, which then has to get read and have the defaults added in. Is it better to leave this blank and fall through later? I don't know. (And thus feel happy to roll with this)

[directionless]

I'd say lets followup, but I think this is all cleaner if Parse works off io readers and writers. There's a lot of extra buffering

[seejdev]

Cool, I'll add this to a follow up PR!