Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Push-to-Talk Support for Jitsi #2280

Closed
wants to merge 30 commits into from
Closed

Push-to-Talk Support for Jitsi #2280

wants to merge 30 commits into from

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Nov 15, 2018

Implements the matrix-react-sdk portion of Push-to-Talk support for Jitsi voice and video calls. The riot-web portion is element-hq/element-web#10369.

Push-to-Talk allows someone to temporarily unmute their microphone during a Jitsi call with just the push of a button. Releasing the button causes their microphone to become muted again. This is exceptionally useful during calls with 3+ people. Toggling the microphone with the shortcut is also supported.

The pipeline involved to make this work stretches quite far, from a new native node module in Riot's electron_app, called iohook, that can handle listening for whitelisted shortcuts even when Riot is minimized, to Scalar/Dimension that is hosting the Jitsi session within its own iframe.

Currently any combination of keyboard keys can be used for a shortcut. The functionality of the setting is currently complete.

This has currently been tested on MacOS/Linux. Windows is currently unsupported due to build issues with iohook.

TODOs that are out of scope for this PR, but that I would like to see if the future:

  • Audio cue for when mic is un/muted, so people know their shortcut is working and their mic is active.
  • Mute Jitsi by default on load if Push-to-Talk enabled

@anoadragon453
Copy link
Member Author

Current UI:

image

Anyone know how to separate the Set button from the Shortcut: ... div? I want the Set on the right and Shortcut on the left.

* develop: (74 commits)
  Send Field label pointer events to input
  Fix backup button in logout dialog
  Keep registration spinner inside the auth modal
  Rename Feather icon directory to `feather-customised`
  Make sure direct chat invites are treated as invites
  move canSendMessages into state so that it will re-render the composer
  Try to clarify that "Show read receipts" is just for visibility
  Move logos out of feather dir
  Move E2E icons out of feather dir
  Make the settings documentation fit within 120 characters per line
  Use a global WatchManager for settings
  Clarify finding first non-null field error
  fix lint
  make it i18n friendly
  Apply PR feedback, don't change room to go to its settings
  make better use of space
  set membership in case we don't get the ev
  Fix textareas
  Support custom tags in the new algorithm
  Fix variable reference
  ...
if (!ActiveWidgetStore.getWidgetMessaging(this.props.id)) {
this._setupWidgetMessaging();
}
this._setupWidgetMessaging();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note to reviewers: this was causing Jitsi to stop receiving messages after going to a different room and back. I'm not sure if removing this check has any other adverse effects but so far I've noticed none.

* develop: (315 commits)
  Support CI for matching branches on forks
  Don't show calculated room name in room settings name input field
  Disable big emoji for m.emote messages as it looks weird
  Update yarn.lock
  Remove Edge from browser support statements
  v1.0.4
  Prepare changelog for v1.0.4
  Released js-sdk
  Translated using Weblate (Slovak)
  Translated using Weblate (Russian)
  Translated using Weblate (Russian)
  Translated using Weblate (Russian)
  Translated using Weblate (Portuguese (Brazil))
  Translated using Weblate (Finnish)
  Translated using Weblate (Basque)
  Translated using Weblate (Arabic)
  Translated using Weblate (Albanian)
  Translated using Weblate (Russian)
  Translated using Weblate (Russian)
  Discard old sticker picker when the URL changes
  ...
toggleJitsiAudio() {
return this.messageToWidget({
api: OUTBOUND_API_NAME,
action: "audioToggle",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these 3 actions should require a bump in API version, as shown here: https://github.com/matrix-org/matrix-react-sdk/pull/2781/files#diff-48acd843c021678a317564b0cf6d8648R24

Also, if you need to figure out if the widget will even support it, a toWidget API request for supported_api_versions should reveal that.

Copy link
Member Author

@anoadragon453 anoadragon453 Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So doing this instead of checking for "jitsi" may be better?

We don't check this way anymore.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sending to an older version is alright though, as it will just ignore the command?

* develop: (269 commits)
  Fix fixed width dialogs
  Fix settings dialog layout
  Translated using Weblate (Swedish)
  Translated using Weblate (Romanian)
  Translated using Weblate (Italian)
  Translated using Weblate (Hungarian)
  Translated using Weblate (French)
  Translated using Weblate (Finnish)
  Translated using Weblate (Dutch)
  Translated using Weblate (Chinese (Traditional))
  Translated using Weblate (Chinese (Simplified))
  Translated using Weblate (Basque)
  Added translation using Weblate (Slovenian)
  Prevent the permalink creator from causing cascading failure
  Remove dead Login.loginAsGuest()
  Don't include all networks by default in the room directory
  Remove 'try the app' link from login
  Bind the onWheel listener directly to props
  Only update analytics when there's a change
  Fix a few bugs introduced in file upload rework
  ...
* develop: (1212 commits)
  Translated using Weblate (French)
  Adjust spelling on debug log screen
  Translated using Weblate (French)
  adjust jsdoc
  Translated using Weblate (Hungarian)
  Appease the linter
  Remove unused identityEnabled property from ValidatedServerConfig
  Finally fix tests
  Close settings after deactivating
  Hopefully actually make the tests happy
  Fix tests to use UIA proper
  Send the correct UIA alongside the wrong UIA for backwards comaptibility
  update copy
  Break out into sections
  Fix i18n
  Verify i18n in CI
  prevent inserting parts at index -1 for empty documents
  prevent autocomplete when doing bulk insertion (paste, drop text)
  manually attach input event handler, as React doesn't pass inputType
  Remove misleading text about admins logging people out from soft logout
  ...
* develop: (261 commits)
  Use options object
  Tweak control flow to avoid duplicate terms prompts
  Tweak import
  describe caret nodes
  describe all reasons why we need a custom textify algorithm
  add line breaks
  Don't load guest sessions on post-registration login link
  document editor
  Silence unnecessary warning
  add undo steps after word boundary (or capped) when typing or removing
  Show terms modal when inviting by email
  put max step length in constant
  Fix dialog button border colours
  Check IS v2 account tokens for validity
  fix bug that prevented a line from being removed when undoing a newline
  pass caret to history manager upon initial render
  add mod+z/y shortcuts, set editor state to what history manager returns
  push changes to history manager
  HistoryManager + unit tests
  Upgrade dependencies
  ...
@t3chguy
Copy link
Member

t3chguy commented Jul 10, 2021

@anoadragon453 what's the status here? Should it be set as a draft until you're ready for more review?

@anoadragon453
Copy link
Member Author

@anoadragon453 what's the status here? Should it be set as a draft until you're ready for more review?

@dbkr has been kind enough to adapt building iohook to the new hak native node build system, and the plan is to rebuild the PR on top of that support.

Though note that iohook currently doesn't support Electron 13, so this should probably remain a Labs flag for now until the project stabilises a bit (they've just switched over maintainership as well).

@anoadragon453 anoadragon453 marked this pull request as draft July 12, 2021 11:04
@anoadragon453
Copy link
Member Author

This is still relevant, but the PR doesn't need to be sitting open in the meantime. The code is quite old, and a new implementation will likely be in a new branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants