Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

whiteboard support #1480

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

whiteboard support #1480

wants to merge 14 commits into from

Conversation

tgrld
Copy link

@tgrld tgrld commented Feb 22, 2023

No description provided.

@tgrld
Copy link
Author

tgrld commented Feb 22, 2023

demo: https://meet.tugrul.dev/

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Thanks for getting this started!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
env.example Outdated Show resolved Hide resolved
env.example Outdated Show resolved Hide resolved
env.example Outdated Show resolved Hide resolved
env.example Outdated Show resolved Hide resolved
env.example Outdated Show resolved Hide resolved
@tgrld tgrld requested a review from saghul March 5, 2023 16:03
@tgrld
Copy link
Author

tgrld commented Mar 8, 2023

jitsi/web docker image must be rebuild

Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Left some comments!

@@ -214,3 +214,8 @@ JIBRI_XMPP_PASSWORD=

# Jitsi image version (useful for local development)
#JITSI_IMAGE_VERSION=latest

# whiteboard support
Copy link
Member

Choose a reason for hiding this comment

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

Please drop these from here and make a PR to the handbook.

@@ -5,6 +5,7 @@
{{ $ENABLE_SUBDOMAINS := .Env.ENABLE_SUBDOMAINS | default "true" | toBool -}}
{{ $XMPP_DOMAIN := .Env.XMPP_DOMAIN | default "meet.jitsi" -}}
{{ $XMPP_BOSH_URL_BASE := .Env.XMPP_BOSH_URL_BASE | default "http://xmpp.meet.jitsi:5280" -}}
{{ $WHITEBOARD_URL_BASE := .Env.WHITEBOARD_URL_BASE | default "http://whiteboard.meet.jitsi" -}}
Copy link
Member

Choose a reason for hiding this comment

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

Don't set a default, otherwise it will always be enabled.

At any rate you are not using this variable.

@@ -130,6 +131,20 @@ location ^~ /etherpad/ {
}
{{ end }}

{{ if .Env.WHITEBOARD_URL_BASE }}
# whiteboard (excalidraw-backend)
location = /socket.io/ {
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some extra path here like excalidraw-collab/socket.io sinilar to what we do for Etherpad.

config.whiteboard.collabServerBaseUrl = '{{ $WHITEBOARD_COLLAB_SERVER_PUBLIC_URL }}';
{{ else if .Env.WHITEBOARD_URL_BASE -}}
config.whiteboard.collabServerBaseUrl = '{{ $PUBLIC_URL }}';
Copy link
Member

Choose a reason for hiding this comment

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

Adjust the URL as mentioned above.

networks:
meet.jitsi:
aliases:
- ${WHITEBOARD_URL_BASE:-whiteboard.meet.jitsi}
Copy link
Member

Choose a reason for hiding this comment

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

Just "hardcode" the alias, like we do for Etherpad.

There is not much of a point in changing it since it's just the internal network alias.

Copy link

Choose a reason for hiding this comment

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

Is this still ongoing? Or is this now supported natively already?

Copy link
Member

Choose a reason for hiding this comment

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

Still ongoing

@loli10K
Copy link

loli10K commented Mar 23, 2024

I would like to have this integrated as well, any news on this @tgrld?

Does this require some sort of official jitsi/excalidraw-backend docker image published by you guys @saghul?

@saghul
Copy link
Member

saghul commented Mar 23, 2024

Yes, we need to publish that image indeed.

I plan to do that and come back to this PR soon.

@loli10K
Copy link

loli10K commented Mar 23, 2024

Thanks. I am currently running the following patch locally and it seems to work without issues (jitsi/excalidraw-backend is a local build)

diff --git a/web/rootfs/defaults/meet.conf b/web/rootfs/defaults/meet.conf
index 6ec7c03..986e9da 100644
--- a/web/rootfs/defaults/meet.conf
+++ b/web/rootfs/defaults/meet.conf
@@ -8,6 +8,7 @@
 {{ $ENABLE_SUBDOMAINS := .Env.ENABLE_SUBDOMAINS | default "true" | toBool -}}
 {{ $XMPP_DOMAIN := .Env.XMPP_DOMAIN | default "meet.jitsi" -}}
 {{ $XMPP_BOSH_URL_BASE := .Env.XMPP_BOSH_URL_BASE | default "http://xmpp.meet.jitsi:5280" -}}
+{{ $WHITEBOARD_ENABLED := .Env.WHITEBOARD_ENABLED | default "0" | toBool }}
 
 server_name _;
 
@@ -133,6 +134,20 @@ location ^~ /etherpad/ {
 }
 {{ end }}
 
+{{ if $WHITEBOARD_ENABLED }}
+# whiteboard (excalidraw-backend)
+location = /socket.io/ {
+    tcp_nodelay on;
+
+    proxy_http_version 1.1;
+    proxy_set_header Upgrade $http_upgrade;
+    proxy_set_header Connection "upgrade";
+    proxy_set_header Host $http_host;
+
+    proxy_pass http://whiteboard.meet.jitsi/socket.io/?$args;
+}
+{{ end }}
+
 location ~ ^/([^/?&:'"]+)$ {
     try_files $uri @root_path;
 }
diff --git a/web/rootfs/defaults/settings-config.js b/web/rootfs/defaults/settings-config.js
index 4a5fbb1..9fae33a 100644
--- a/web/rootfs/defaults/settings-config.js
+++ b/web/rootfs/defaults/settings-config.js
@@ -554,7 +554,11 @@ config.e2eping.maxMessagePerSecond = {{ .Env.E2EPING_MAX_MESSAGE_PER_SECOND }};
 // Settings for the Excalidraw whiteboard integration.
 config.whiteboard = {
     enabled: {{ $WHITEBOARD_ENABLED }},
-    collabServerBaseUrl: '{{ $WHITEBOARD_COLLAB_SERVER_PUBLIC_URL }}'
+{{ if .Env.WHITEBOARD_COLLAB_SERVER_PUBLIC_URL -}}
+    config.whiteboard.collabServerBaseUrl = '{{ $WHITEBOARD_COLLAB_SERVER_PUBLIC_URL }}';
+{{ else }}
+    collabServerBaseUrl: '{{ $PUBLIC_URL }}'
+{{ end }}
 };
 
 // Testing
diff --git a/whiteboard.yml b/whiteboard.yml
new file mode 100644
index 0000000..d7817ae
--- /dev/null
+++ b/whiteboard.yml
@@ -0,0 +1,12 @@
+version: '3.5'
+
+services:
+  whiteboard:
+        image: jitsi/excalidraw-backend:${JITSI_IMAGE_VERSION:-unstable}
+        restart: ${RESTART_POLICY:-unless-stopped}
+        depends_on:
+            - web
+        networks:
+            meet.jitsi:
+              aliases:
+                    - whiteboard.meet.jitsi

However as soon as i move the excalidraw location to a different path as requested in the code review things start to break:

diff --git a/web/rootfs/defaults/meet.conf b/web/rootfs/defaults/meet.conf
index 986e9da..6ade033 100644
--- a/web/rootfs/defaults/meet.conf
+++ b/web/rootfs/defaults/meet.conf
@@ -136,7 +136,7 @@ location ^~ /etherpad/ {
 
 {{ if $WHITEBOARD_ENABLED }}
 # whiteboard (excalidraw-backend)
-location = /socket.io/ {
+location = /excalidraw-collab/socket.io/ {
     tcp_nodelay on;
 
     proxy_http_version 1.1;
diff --git a/web/rootfs/defaults/settings-config.js b/web/rootfs/defaults/settings-config.js
index 9fae33a..a828a2d 100644
--- a/web/rootfs/defaults/settings-config.js
+++ b/web/rootfs/defaults/settings-config.js
@@ -557,7 +557,7 @@ config.whiteboard = {
 {{ if .Env.WHITEBOARD_COLLAB_SERVER_PUBLIC_URL -}}
     config.whiteboard.collabServerBaseUrl = '{{ $WHITEBOARD_COLLAB_SERVER_PUBLIC_URL }}';
 {{ else }}
-    collabServerBaseUrl: '{{ $PUBLIC_URL }}'
+    collabServerBaseUrl: '{{ $PUBLIC_URL }}/excalidraw-collab/'
 {{ end }}
 };
 

For some reason my browser keeps trying to connect to $PUBLIC_URL/socket.io instead of $PUBLIC_URL/excalidraw-collab/socket.io.

@loli10K
Copy link

loli10K commented Apr 25, 2024

I'm sorry for being persistent here but how soon-ish can we expect this to move forward @saghul? I am having trouble keeping up with master while also manually applying patches for this functionality (plus #1737).

Also should we open a new PR for this? From his activity history it seems @tgrld is no longer active on GitHub.

@saghul
Copy link
Member

saghul commented Apr 25, 2024

I can't provide an ETA, sorry. Things are busy at the moment.

I'd say open a new PR.

(plus #1737).

How is that one related? AFAIS my feedback has yet to be addressed.

@loli10K
Copy link

loli10K commented Apr 25, 2024

How is that one related? AFAIS my feedback has yet to be addressed.

It is indeed not related to this one, i was just saying that since i also need that functionality i am required to apply both patches manually every time master gets updated which is kind of troublesome, so at least having this merged would really help. I will open a new PR.

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

Successfully merging this pull request may close these issues.

None yet

4 participants