MLFlexer / resurrect.wezterm

A plugin to restore windows, tabs and panes for Wezterm inspired by tmux-resurrect
MIT License
33 stars 1 forks source link

add event emitters #33

Closed joncrangle closed 1 month ago

joncrangle commented 1 month ago

https://github.com/MLFlexer/resurrect.wezterm/issues/27

This draft PR adds event emitters for:

Example: user config for sending a toast notification when events occur:

local resurrect_event_listeners = {
  "resurrect.error",
  "resurrect.save_state",
  "resurrect.load_state",
}
for _, event in ipairs(resurrect_event_listeners) do
  wezterm.on(event, function(msg, window)
    window:toast_notification("Wezterm", msg, nil, 4000)
  end)
end

It's marked as draft because the API could still use some workshopping, identify more places where we might want to emit resurrect.error. It also needs more testing to see whether there are any issues, particularly with user config functions now that a window object is required if the user wants a toast notification.

I started with the current state of the age encryption PR, since it's close to merge and I figured we would want to emit error events for encryption and json sanitization errors. I actually json sanitization error notifications in because I was getting errors related to an improperly escaped 0X1B character on Arch.

MLFlexer commented 1 month ago

First of all, thank you for taking the initiative on this! I have a few comments for the current state :smile:

I think that the event names should be descriptive, like you have made them with resurrect.load_state. If there could be need for multiple emits in a function, then it might be a good idea to name them like so: resurrect.load_state.start and resurrect.load_state.finished.

I also think it would be a good idea to include the relevant args in the emit, like name and type for the load_state function.

Furthermore I dont think it is necessary to include a description of the event like "State loaded" as this should be obvious from the event name and might only be relevant for the toast notification, but if it used for something else, then this argument is unnecessary in my opinion.

joncrangle commented 1 month ago

First of all, thank you for taking the initiative on this! I have a few comments for the current state 😄

I think that the event names should be descriptive, like you have made them with resurrect.load_state. If there could be need for multiple emits in a function, then it might be a good idea to name them like so: resurrect.load_state.start and resurrect.load_state.finished.

I also think it would be a good idea to include the relevant args in the emit, like name and type for the load_state function.

Furthermore I dont think it is necessary to include a description of the event like "State loaded" as this should be obvious from the event name and might only be relevant for the toast notification, but if it used for something else, then this argument is unnecessary in my opinion.

Thank you for your feedback. I've made updates to revert passing the window object, and added more emitters with relevant args. I provided an example in the docs of using the emitters for toast notifications using your snippet.

MLFlexer commented 1 month ago

For error events only, I feel like the resurrect.error and the error message as an argument to the callback was a good pattern, as I would imagine error events being mostly (only?) used for notifying the user via. a the toast.