fix: Add Beeep action to protobuf definitions and UI components#679
fix: Add Beeep action to protobuf definitions and UI components#679johan-scriptdrift wants to merge 3 commits intogarethgeorge:mainfrom
Conversation
- Add Beeep action to the Hook message in config.proto - Modify HooksFormList component to include Beeep action - Update go.mod with beeep and related dependencies
|
|
proto/v1/config.proto
Outdated
| string template = 2 [json_name="template"]; | ||
| } | ||
|
|
||
| message Beeep { |
There was a problem hiding this comment.
Naming note: it may make more sense to name this after the capability e.g. OsNotification or such rather than the library beeep on the off chance that we need to migrate in the future.
There was a problem hiding this comment.
Agree, I will update.
| style={{ width: "100%" }} | ||
| placeholder={"Choose icon"} | ||
| options={[ | ||
| { label: "Information", value: "assets/information.png" }, |
There was a problem hiding this comment.
Where is this asset defined? Is this something backrest needs to bundle?
There was a problem hiding this comment.
The icon is not available on macOS so I wasn't able to test it easily and thought it would work on Windows and Linux. There is an asset folder in the beeep package and I assumed the icons would be loaded from there.
I've now tried this on windows and the icons does not seem to work their either. So maybe we should remove this input?
proto/v1/config.proto
Outdated
| message Beeep { | ||
| string template = 1 [json_name="template"]; | ||
| string title_template = 2 [json_name="titleTemplate"]; | ||
| double frequency = 3 [json_name="frequency"]; |
There was a problem hiding this comment.
Im somewhat ambivalent about accepting a frequency and duration , I wonder if most users mind or would rather have some opinionated default e.g. simply an enum.
I could see something like
enum DeliveryMode {
SILENT,
LOW_PITCH_CHIME,
HIGH_PITCH_CHIME,
}
.
WDYT? are you finding the range of frequency control useful?
There was a problem hiding this comment.
It seems like setting the frequency and duration does not work. At leat not on macOS. Maybe we should reduce this to just SILENT / CHIME ?
|
Really nice contribution, thanks for the interest! And I think quite high value given that most approaches to OS notifications are not cross platform and involve powershell or action script items today. Scripting this is something I see users do frequently. Using a cross platform library / adding native support makes a lot of sense to me and beeep is a good candidate library. Added a few notes but mostly opinions weakly held :). |
garethgeorge
force-pushed
the
main
branch
4 times, most recently
from
810daa4 to
5bc63d4
Compare
4 participants
Only tested on macOS!