[MM-57964] Implement openThread and openStopRecordingModal
#3073
Conversation
api-types/index.ts
Outdated
| @@ -70,6 +70,12 @@ export type DesktopAPI = { | |||
|
|
|||
| focusPopout: () => void; | |||
|
|
|||
| openThread: (threadID: string) => void; | |||
There was a problem hiding this comment.
Can we rename these so that we know they're specific to calls?
Something like openThreadForCalls.
I know it's a bit "smurf naming" but it's important in this context to make sure the API is only used for that.
There was a problem hiding this comment.
Sure. This functionality was generic enough that I thought it could be used by something else in the future, but we can rename it for now since Calls is currently the only user.
There was a problem hiding this comment.
@devinbinnie Done
| @@ -504,6 +508,14 @@ export class CallsWidgetWindow { | |||
| }; | |||
| }; | |||
|
|
|||
| private handleCallsOpenThread = (event: IpcMainEvent, threadID: string) => { | |||
| this.forwardToMainApp(CALLS_WIDGET_OPEN_THREAD)(event, threadID); | |||
There was a problem hiding this comment.
Can you link me where in (I assume the calls plugin) code this is being received and what it's doing? We'll need to make sure that it's not something exploitable.
A quick look seems like nothing in the Desktop App should be an issue with this (since it's just forwarding through)
There was a problem hiding this comment.
| openStopRecordingModal: (channelID) => ipcRenderer.send(CALLS_WIDGET_OPEN_STOP_RECORDING_MODAL, channelID), | ||
| onOpenStopRecordingModal: (listener) => createListener(CALLS_WIDGET_OPEN_STOP_RECORDING_MODAL, listener), |
There was a problem hiding this comment.
@devinbinnie @cpoile Since we are here, I was considering making this a bit more generic (e.g. openCallsModal) so we could pass a modal ID (string or enum) and at least the channel/call ID which is hopefully sufficient in most cases. That would avoid the need to make desktop changes every time we want to add a modal interaction from the widget.
Thoughts?
There was a problem hiding this comment.
I'd be okay with that, but I think we'd need to lock down the types of modals that could be opened somewhere, and this should never be used to open a modal that's native to the desktop app. If we're just using it as a relay to the calls plugin from the widget (or vice-versa) then I'm okay with it.
There was a problem hiding this comment.
Okay. Of course we'd still be handling this event only for Calls windows.
There was a problem hiding this comment.
I'll be deferring this improvement to the next time we'll need this sort of functionality.
devinbinnie
added
4: Reviews Complete
|
@devinbinnie Would you be able to publish a new API types NPM version? Thanks! |
|
@streamer45 #3075 |
Summary
We are extending the API to support the functionality in mattermost/mattermost-plugin-calls#779, namely:
Ticket Link
https://mattermost.atlassian.net/browse/MM-57964
Release Note