-
Notifications
You must be signed in to change notification settings - Fork 342
Facilitate running custom javascript on the main JS process #382
base: master
Are you sure you want to change the base?
Conversation
EventNameAppErrorAccept = "app.error.accept" | ||
EventNameAppEventReady = "app.event.ready" | ||
EventNameAppEventSecondInstance = "app.event.second.instance" | ||
EventNameAppExecuteJavaScript = "app.execute.javascript" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you rename those based on what I've suggested in the JS PR ?
@@ -444,3 +447,35 @@ func (a *Astilectron) NewTray(o *TrayOptions) *Tray { | |||
func (a *Astilectron) NewNotification(o *NotificationOptions) *Notification { | |||
return newNotification(a.worker.Context(), o, a.supported != nil && a.supported.Notification != nil && *a.supported.Notification, a.dispatcher, a.identifier, a.writer) | |||
} | |||
|
|||
type eventProxy struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need this new struct, *Astilectron
already has an On()
method
dispatcher: a.dispatcher, | ||
logger: a.l, | ||
} | ||
event, err := synchronousEvent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use this instead:
synchronousEvent(a.worker.Context(), a, a.writer, Event{Name: EventNameAppCmdExecuteJavaScript, TargetID: targetIDApp, Code: code}, EventNameAppEventExecutedJavaScript)
@@ -28,6 +28,7 @@ type Event struct { | |||
Code string `json:"code,omitempty"` | |||
Displays *EventDisplays `json:"displays,omitempty"` | |||
Enable *bool `json:"enable,omitempty"` | |||
Error *string `json:"error,omitempty"` | |||
FilePath string `json:"filePath,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather you create this new field instead of using Reply
:
Success json.RawMessage `json:"success"`
if event.Error != nil && *event.Error != "" { | ||
return fmt.Errorf("ExecuteJavascript failed: %s", *event.Error) | ||
} | ||
if err := json.Unmarshal([]byte(event.Reply), response); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Could you wrap this in
if response != nil && len(event.Success) > 0
because some developers may not need a response here - If you use the
Success
field as I mentioned, you can usejson.Unmarshal(event.Success, response)
instead
Note this PR is dependent on asticode/astilectron#63
The rationale is explained in the PR above.
The
eventProxy
feels very awkward, I'm probably doing more than I need to. Happy to receive some pointers on how to improve this code.