Custom App Icon from Plugin

charlespascoe commented 2 years ago

Context

This is mainly something to make the interface a bit prettier, particularly as more people create plugins for Serenade.

Goals

I'd like to be able to pass a custom app icon from a plugin, so that as new plugins for new editors/terminals/browsers/etc. are created, the plugin author can provide an icon rather than it defaulting to a generic icon.

Proposed Solution

The plugin can optionally pass an icon string in the initial active WebSocket message, which is a base64-encoded image (e.g. using a data:// url, so that it can be used directly with the src prop in img tags) to use as the icon when the associated app is focused

A few potentially helpful implementation details that I've figured out from exploring the source code:

Add customAppIcon property (with some falsey value, maybe null or an empty string) to initialState in client/src/renderer/state/reducer.ts
Update client/src/main/active.ts, specifically the Active.update() method, to include an extra value in the state (e.g. add customAppIcon: this.pluginManager.findApp(this.app)?.icon in the call to this.bridge.setState() on line 405)
Update client/src/renderer/components/indicators/active-app-indicator.tsx to take the new customAppIcon state property, and use it as the icon src if its value is truthy, otherwise defaulting to the current logic.

I may have missed some other edge cases (e.g. what to do when the plugin disconnects, though that may already be handled by the current logic - I'm still reading the code), so please provide any additional guidance if necessary.

I'd be happy to implement this if the above sounds reasonable - it would give me the opportunity to set up my dev environment for Serenade and start contributing to the project.

Alternatives

The alternative is that you have to keep updating Serenade with an ever-growing list of icons ;)

tmacwill commented 2 years ago

thanks for the suggestion! I like this idea a lot, and agree with your implementation. this also adds minimal complexity to the application, which is great.

a few things you might want to consider:

we can probably just call the field in the protocol icon rather than customAppIcon for brevity and clarity.
if we add the icon field to the active message, one cool property is that applications will be able to change their icons dynamically. for instance, if a plugin has an error, it could show a different icon. this could be some nice functionality we haven't had before.
we haven't tried passing a significant amount of data from the main process to the renderer process (i.e., via setState), so would just check that there aren't any performance issues there. I'd imagine there are optimizations we can make if there are, like only sending the icon when the process changes, caching it in the renderer process, and so on.
on a related note, we might want to limit the size of the icon, just to prevent anything crazy from happening?

will add the approved tag to this issue, and feel free to assign to yourself if you're interested in implementing! fwiw, since this is a client-only change, you won't have to worry about setting up all the dependencies for the speech engine, code engine, etc.

charlespascoe commented 2 years ago

Great! I'll start working on it, though I can't seem to assign this issue to myself.

The size of the icons should be fairly small compared to any limits the Electron IPC should impose, but I'll double check before submitting a pull request. It's also worth noting the icon wouldn't be sent frequently, only when the application is focused or the plugin changes the icon (which is a cool idea that I hadn't thought of).

I agree there should be some size limit; I created a 48x48 icon (which larger than what is displayed) that was 1.6KB raw or 2.2KB when encoded as a data: URL string, so maybe a 8KB or 16KB limit on the length of the icon string should limit any performance issues.

I'll also update the relevant documentation; it seems web/src/components/docs/protocol/messages-reference.tsx would be the right place to document this new field, but is there anywhere else I should update?

tmacwill commented 2 years ago

sounds great! just assigned to you. that's the right place to update in the documentation as well.

serenadeai / serenade