Additional configuration to customize streaming dialog #9539

skolmer · 2021-07-13T22:31:28Z

The current streaming feature already allows us to stream to other streaming providers than youtube by submitting a rtmp-url instead of a youtube stream key. This is great but there is currently no way to change the UI to include details about the streaming service we want to use.
We extended the dialog with config parameters that allow us to change the default terms and condition link, the privacy policy link and also the regex that is used to validate the input.
The help link was already part of the interface config so we added the new params accordingly.
This PR will fix #9524

skolmer · 2021-08-17T19:31:21Z

@saghul To support your current effort in deprecating the interface_config.js, should we move this to config.js including the already existing LIVE_STREAMING_HELP_LINK? Any preferences how this should be structured? Multiple config values or one config object that contains everything liveStreaming related?

saghul · 2021-08-18T07:26:00Z

To support your current effort in deprecating the interface_config.js, should we move this to config.js including the already existing LIVE_STREAMING_HELP_LINK?

THANK YOU! Much appreciated. Yes, let's do that.

Any preferences how this should be structured? Multiple config values or one config object that contains everything liveStreaming related?

The latter, a liveStreaming object which contains everything.

Now, that would break backwards compatibility, so you can add a small "translation" piece of code here:

jitsi-meet/react/features/base/config/reducer.js

Line 189 in fbf9d48

function _translateLegacyConfig(oldValue: Object) {

like we do for toolbarButtons.

Thanks for doing this!

skolmer · 2021-08-26T08:46:41Z

@saghul We have changed the PR to the new config.js format and added the migration script for LIVE_STREAMING_HELP_LINK

stale · 2022-01-08T22:33:54Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

skolmer · 2022-01-11T18:44:04Z

@saghul I see there are some conflicts and the bot marked it as wontfix. Is this PR obsolete now or are there still plans to merge it. If so please let us know and we try to fix the conflicts.

saghul · 2022-01-12T16:13:53Z

Sure thing, we'd love to have it. Sorry it fell through the cracks, can you please rebase it and fix the conflicts?

skolmer · 2022-01-13T17:38:25Z

Sure thing, we'd love to have it. Sorry it fell through the cracks, can you please rebase it and fix the conflicts?

Perfect, we just fixed the merge conflict so the PR is now ready again to be merged.

skolmer · 2022-01-27T11:26:41Z

@saghul Can you give us a note when the right time is to get this merged? We can than fix merge conflicts on short notice. Otherwise we will do it over and over again 😉

saghul · 2022-01-27T13:21:15Z

Sorry for the delay. I am planning to spend time on this PR tomorrow.

saghul

Left some comments, PTAL.

config.js

react/features/base/config/reducer.js

react/features/recording/components/LiveStream/AbstractStreamKeyForm.js

react/features/recording/components/LiveStream/native/StreamKeyForm.js

skolmer · 2022-05-23T11:53:54Z

Hi @saghul could you take another look at this PR it looks like it is now ready to be merged?

saghul · 2022-05-23T20:02:56Z

Thanks for the poke. I'll be afk until next week, I'll take a look when I'm back.

skolmer · 2022-06-13T08:36:24Z

Thanks for the poke. I'll be afk until next week, I'll take a look when I'm back.

Just a friendly reminder 😊

saghul

Left some comments, PTAL!

saghul · 2022-06-13T12:43:19Z

config.js

@@ -676,6 +676,18 @@ var config = {
    // 'microphone', 'camera', 'select-background', 'invite', 'settings'
    // hiddenPremeetingButtons: [],

+    // Customize the Live Streaming dialog. Can be modified for a non-Youtube provider.


Can you please move these close to where the recording options are?

saghul · 2022-06-13T12:43:28Z

config.js

@@ -676,6 +676,18 @@ var config = {
    // 'microphone', 'camera', 'select-background', 'invite', 'settings'
    // hiddenPremeetingButtons: [],

+    // Customize the Live Streaming dialog. Can be modified for a non-Youtube provider.


Suggested change

// Customize the Live Streaming dialog. Can be modified for a non-Youtube provider.

// Customize the Live Streaming dialog. Can be modified for a non-YouTube provider.

saghul · 2022-06-13T12:43:40Z

config.js

@@ -676,6 +676,18 @@ var config = {
    // 'microphone', 'camera', 'select-background', 'invite', 'settings'
    // hiddenPremeetingButtons: [],

+    // Customize the Live Streaming dialog. Can be modified for a non-Youtube provider.
+    // liveStreaming: {


Add this to the whitelist please.

saghul · 2022-06-13T12:45:12Z

react/features/recording/components/LiveStream/AbstractStreamKeyForm.js

+ */
+export function _getLiveStreaming(state: Object) {
+    const { helpLink, termsLink, dataPrivacyLink, validatorRegExpString } = getLiveStreaming(state) || {};
+    const fourGroupsDashSeparated = /^(?:[a-zA-Z0-9]{4}(?:-(?!$)|$)){4}/;


make this a const at the top level?

saghul · 2022-06-13T12:46:35Z

react/features/recording/components/LiveStream/AbstractStreamKeyForm.js

-            || LIVE_STREAMING_HELP_LINK;
+        const { helpURL, termsURL, dataPrivacyURL, streamLinkRegexp } = this.props._liveStreaming;
+
+        this.helpURL = helpURL;


This is not a good idea since the props can chance. Just read it from props wherever you need it.

saghul · 2022-06-13T12:47:26Z

react/features/recording/components/LiveStream/AbstractStreamKeyForm.js

+ * @returns {LiveStreamingProps}
+ * @private
+ */
+export function _getLiveStreaming(state: Object) {


Make function called getLiveStreaming in functions.any.js actually do this and give you the nice pre-filled object.

saghul · 2022-06-13T12:47:47Z

react/features/recording/components/LiveStream/constants.js

@@ -13,3 +13,10 @@ export const YOUTUBE_LIVE_DASHBOARD_URL = 'https://www.youtube.com/live_dashboar
 * The URL for YouTube terms and conditions.
 */
 export const YOUTUBE_TERMS_URL = 'https://www.youtube.com/t/terms';
+
+/**
+ * The live streaming help link to display. On web it comes from


This comment no longer applies.

saghul · 2022-06-13T12:48:05Z

react/features/recording/components/LiveStream/constants.js

+ * The live streaming help link to display. On web it comes from
+ * interfaceConfig, but we don't have that on mobile.
+ */
+export const LIVE_STREAMING_HELP_LINK = 'https://jitsi.org/live';


Let's add a DEFAULT_ or JITSI_ in front please.

skolmer · 2022-06-28T13:49:05Z

Left some comments, PTAL!

Hey, sorry for the late response. We made the suggested changes. If you agree with them we should be ready to merge :)

saghul

Left a few comments, almost there!

saghul · 2022-07-04T14:01:18Z

config.js

@@ -295,6 +295,18 @@ var config = {
    // Whether to enable live streaming or not.
    // liveStreamingEnabled: false,


Please move this one into the new liveStreaming section and name it enabled.

saghul · 2022-07-04T14:01:42Z

config.js

@@ -295,6 +295,18 @@ var config = {
    // Whether to enable live streaming or not.
    // liveStreamingEnabled: false,

+    // Customize the Live Streaming dialog. Can be modified for a non-YouTube provider.


Please move this block of options right after localRecording, so all recording options are together.

saghul · 2022-07-04T14:02:19Z

react/features/base/config/reducer.js

@@ -397,6 +397,15 @@ function _translateLegacyConfig(oldValue: Object) {
        };
    }

+    // Migrate interfaceConfig.LIVE_STREAMING_HELP_LINK


When you move the enabled option, please add migration code here.

saghul · 2022-07-04T14:03:01Z

react/features/base/config/reducer.js

@@ -397,6 +397,15 @@ function _translateLegacyConfig(oldValue: Object) {
        };
    }

+    // Migrate interfaceConfig.LIVE_STREAMING_HELP_LINK
+    if (oldValue.liveStreaming === undefined


Generally speaking, it's good to always add the empty object here, before we add backwards compatible options.

saghul · 2022-07-04T14:03:33Z

react/features/recording/components/LiveStream/AbstractStreamKeyForm.js

+    /**
+     * The live streaming dialog properties.
+     */
+    _liveStreaming: LiveStreamingProps


We put these at the top, generally.

saghul · 2022-07-04T14:04:04Z

react/features/recording/components/LiveStream/AbstractStreamKeyForm.js

@@ -150,9 +157,22 @@ export default class AbstractStreamKeyForm<P: Props>
     */
    _validateStreamKey(streamKey = '') {
        const trimmedKey = streamKey.trim();
-        const fourGroupsDashSeparated = /^(?:[a-zA-Z0-9]{4}(?:-(?!$)|$)){4}/;
-        const match = fourGroupsDashSeparated.exec(trimmedKey);
+        const match = this.props._liveStreaming?.streamLinkRegexp.exec(trimmedKey);


Looks like streamLinkRegexp could be undefined?

saghul · 2022-07-04T14:04:32Z

react/features/recording/components/LiveStream/functions.js

@@ -0,0 +1,29 @@
+// @flow


Drop the flow anotation please, we are migrating to TS.

saghul · 2022-07-04T14:05:01Z

react/features/recording/components/LiveStream/index.js

@@ -1,3 +1,4 @@
 // @flow

 export * from './_';
+export * from './functions';


Don't export anything from a feature's index.js, just use the full path to /functions when importing from another feature.

…ialog; deprecated LIVE_STREAMING_HELP_LINK

…eaming.enabled

skolmer · 2022-07-20T10:05:52Z

@segnord Next time please do not force push while the PR is in review. It makes it really hard to follow up on review comments.
@saghul We took care about your latest review feedback, can you take another look?

saghul · 2022-07-20T15:05:18Z

webpack chokes on something so I cannot test it. Can you please make sure the linter is tamed? Apologies in advance because this is in part our fault due to the transition to TypeScript.

skolmer · 2022-07-20T17:44:04Z

webpack chokes on something so I cannot test it. Can you please make sure the linter is tamed? Apologies in advance because this is in part our fault due to the transition to TypeScript.

No worries, it should work now :)

saghul · 2022-07-21T13:33:25Z

You can run npm lint locally to be sure, since I seem to need to approve every GH action run :-/

skolmer · 2022-07-21T13:37:06Z

@segnord Can you take a look? There still seems to be an issue with the Config type:

Error: react/features/base/config/reducer.ts(423,14): error TS2339: Property 'liveStreaming' does not exist on type 'IConfig'.
Error: react/features/base/config/reducer.ts(423,39): error TS2339: Property 'liveStreaming' does not exist on type 'IConfig'.
Error: react/features/base/config/reducer.ts(427,18): error TS2339: Property 'liveStreaming' does not exist on type 'IConfig'.
Error: react/features/base/config/reducer.ts(428,25): error TS2339: Property 'liveStreaming' does not exist on type 'IConfig'.
Error: react/features/base/config/reducer.ts(434,18): error TS2339: Property 'liveStreaming' does not exist on type 'IConfig'.
Error: react/features/base/config/reducer.ts(437,18): error TS2339: Property 'liveStreaming' does not exist on type 'IConfig'.
Error: react/features/base/config/reducer.ts(438,25): error TS2339: Property 'liveStreaming' does not exist on type 'IConfig'.
Error: Process completed with exit code 2.

…-877-stream-dialog-parameter

segnord · 2022-07-21T15:20:12Z

@skolmer Sorry, I didn't notice that the linter in the main branch now scans typescript files, I've merged the master branch into this MR and the npm lint runs fine locally.

skolmer · 2022-08-02T08:14:11Z

@saghul The pipeline is now green, can you take another look if everything is fine now?

saghul · 2022-08-02T08:27:23Z

Cheers!

saghul self-requested a review September 15, 2021 16:02

stale bot added the wontfix Issue won't be fixed label Jan 8, 2022

stale bot removed the wontfix Issue won't be fixed label Jan 11, 2022

segnord force-pushed the nic/feat/PB-877-stream-dialog-parameter branch 2 times, most recently from eaf69ec to be1d655 Compare January 13, 2022 17:37

saghul requested changes Jan 28, 2022

View reviewed changes

skolmer requested a review from saghul March 10, 2022 12:16

saghul requested changes Jun 13, 2022

View reviewed changes

segnord force-pushed the nic/feat/PB-877-stream-dialog-parameter branch 2 times, most recently from a554758 to 5c68bb9 Compare June 28, 2022 13:47

saghul reviewed Jul 4, 2022

View reviewed changes

feat(config) liveStreaming object inroduced to modify the streaming d…

cde093d

…ialog; deprecated LIVE_STREAMING_HELP_LINK

segnord force-pushed the nic/feat/PB-877-stream-dialog-parameter branch from f981648 to e654dea Compare July 6, 2022 21:48

feat(config) config.liveStreamingEnabled replaced with config.liveStr…

8902a4b

…eaming.enabled

segnord force-pushed the nic/feat/PB-877-stream-dialog-parameter branch from e654dea to 8902a4b Compare July 6, 2022 21:52

skolmer requested a review from saghul July 16, 2022 20:12

feat(config) fixed eslint errors

64c680b

feat(config) fixed eslint errors

61d1b94

segnord added 2 commits July 21, 2022 17:16

Merge remote-tracking branch 'remotes/origin/master' into nic/feat/PB…

d56b34b

…-877-stream-dialog-parameter

feat(config) added liveStreaming type to the IConfig

becb37b

saghul approved these changes Aug 2, 2022

View reviewed changes

saghul merged commit 87f6d27 into jitsi:master Aug 2, 2022

	// Customize the Live Streaming dialog. Can be modified for a non-Youtube provider.
	// Customize the Live Streaming dialog. Can be modified for a non-YouTube provider.

		@@ -295,6 +295,18 @@ var config = {
		// Whether to enable live streaming or not.
		// liveStreamingEnabled: false,

Additional configuration to customize streaming dialog #9539

Additional configuration to customize streaming dialog #9539

Conversation

skolmer commented Jul 13, 2021

skolmer commented Aug 17, 2021

saghul commented Aug 18, 2021

skolmer commented Aug 26, 2021

stale bot commented Jan 8, 2022

skolmer commented Jan 11, 2022

saghul commented Jan 12, 2022

skolmer commented Jan 13, 2022

skolmer commented Jan 27, 2022

saghul commented Jan 27, 2022

saghul left a comment

Choose a reason for hiding this comment

skolmer commented May 23, 2022

saghul commented May 23, 2022

skolmer commented Jun 13, 2022

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skolmer commented Jun 28, 2022

saghul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skolmer commented Jul 20, 2022

saghul commented Jul 20, 2022

skolmer commented Jul 20, 2022

saghul commented Jul 21, 2022

skolmer commented Jul 21, 2022

segnord commented Jul 21, 2022

skolmer commented Aug 2, 2022

saghul commented Aug 2, 2022