Skip to content

[WIP] Add screensharing support.#227

Merged
Ivansss merged 14 commits into
masterfrom
screensharing
Mar 14, 2017
Merged

[WIP] Add screensharing support.#227
Ivansss merged 14 commits into
masterfrom
screensharing

Conversation

@fancycode

@fancycode fancycode commented Jan 20, 2017

Copy link
Copy Markdown
Member

This PR adds support for screensharing (#31).

Currently more a tech demo 🙈 various parts still need implementation:

cc @jancborchardt for ui, @nickvergessen @LukasReschke, @Ivansss for general review

Comment thread extensions/chrome/manifest.json Outdated
},
"content_scripts": [ {
"js": [ "content.js" ],
"matches": [ "https://replace-with-your-hostname/*" ]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

😒

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This one actually would allow wildcard hostnames, it's the matches below that doesn't allow it.
I'll look if there is another way to communicate between website and the extension, or if we have to deal with the "one extension per hostname".

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Updated extension to work with all domains 🎉

@fancycode

Copy link
Copy Markdown
Member Author

I think we should develop the extensions in separate repositories as their release cycle (and versioning) will be different from the app itself. What do you think?

Comment thread extensions/chrome/manifest.json Outdated
{
"name": "Nextcloud Screensharing",
"description": "Screensharing utility for the Nextcloud video calls app.",
"author": "Joachim Bauch <bauch@struktur.de>"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

missing ,

@nickvergessen

Copy link
Copy Markdown
Member

chrome-screensharing-extension ?

@codecov-io

codecov-io commented Jan 23, 2017

Copy link
Copy Markdown

Codecov Report

Merging #227 into master will decrease coverage by 0.39%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #227     +/-   ##
===========================================
- Coverage     17.31%   16.92%   -0.4%     
- Complexity      240      291     +51     
===========================================
  Files            16       16             
  Lines          1155     1312    +157     
===========================================
+ Hits            200      222     +22     
- Misses          955     1090    +135
Impacted Files Coverage Δ Complexity Δ
lib/Controller/ApiController.php 0% <0%> (ø) 113% <0%> (+41%)
lib/Notification/Notifier.php 95.19% <0%> (+1.28%) 27% <0%> (+10%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c29495a...ae9d60d. Read the comment docs.

@nickvergessen

Copy link
Copy Markdown
Member

@jancborchardt

Copy link
Copy Markdown
Member

Interface-wise what about:

  • »Share screen« and the current »Fullscreen« function should be in a (to be created) popover menu of a 3-dot-menu next to the audio and video buttons of your own video.
  • If someone shares a screen then that screen should be shown as the promoted (big) video permanently as long as it’s shared. Sized so that it’s not overlayed by the participant videos below though.
  • Should we limit screensharing to one screen at a time? Otherwise we need to reconsider the interface.
  • Anything else?

@fancycode

Copy link
Copy Markdown
Member Author

Firefox support is now blocked on #234 (no PeerConnection is being established, so also no screensharing possible).

fancycode added a commit to nextcloud/spreed-screensharing-chrome-extension that referenced this pull request Jan 27, 2017
This is required to support screensharing in Chrome for the Nextcloud Spreed
app (see nextcloud/spreed#227).

Signed-off-by: Joachim Bauch <bauch@struktur.de>
fancycode added a commit to nextcloud/spreed-screensharing-firefox-addon that referenced this pull request Jan 27, 2017
This is required to support screensharing in Firefox for the Nextcloud Spreed
app (see nextcloud/spreed#227).

Signed-off-by: Joachim Bauch <bauch@struktur.de>
Comment thread js/app.js Outdated

var screensharingStopped = function() {
console.log("Screensharing now stopped");
// No need to notify the user.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

log is only done in debug mode, I guess it would be fine to keep.

@fancycode

Copy link
Copy Markdown
Member Author

@Ivansss did you make sure to keep my screensharing-related changes (and any other changes that might have happened in the past) in your simplewebrtc upgrade? Maybe it would make sense to upgrade in a separate PR.

@Ivansss

Ivansss commented Feb 9, 2017

Copy link
Copy Markdown
Member

@fancycode Yes, I applied all the changes we did in simplewebrtc.js since the initial commit on top.

@Ivansss

Ivansss commented Feb 9, 2017

Copy link
Copy Markdown
Member

Reverted SimpleWebRTC update, doing it in a different PR #242 .

@nickvergessen

Copy link
Copy Markdown
Member

Rebased and simply dropped the commit and the revert.

@fancycode

Copy link
Copy Markdown
Member Author

FYI, you could have rebased only up to my last commit to avoid getting new hashes for all old commits.

@jancborchardt

jancborchardt commented Feb 13, 2017

Copy link
Copy Markdown
Member

Talked more with @Ivansss how to proceed:

  • clicking the shared screen increases the size and breaks it
  • when sharing a screen, we need to change the layout to have the screenshare max height not be overlapped by the videos of the other people. Also both videos need to be visible. So we need to switch to the 3-person-layout as soon as screensharing begins, with your own video staying in the bottom right and the video of the other person in the bottom left.
  • screensharing will be a third icon next to audio and video
  • by default the icon should be showed slightly transparent, just like the audio and video icons when muted / disabled
  • fullscreen will move to the top right of the #app-content because it relates to the whole call, not only to your view or your things. Shouldn’t be shown in the emptycontent view because then the sidebar isn’t shown and you can’t join a call.
  • I’ll do a proper icon for screensharing and add it to core
  • the icons should get tooltips (the dark JS ones) saying – depending on state: »Mute audio« / »Unmute audio«, »Disable video« / »Enable video«, »Share screen« / »Unshare screen« and »Fullscreen« / »Exit fullscreen«
  • when clicking the screensharing icon and the extension is not installed yet, show a popover prompting to install the extension, with a link fitting for the browser. If it’s Safari or something else, have the popover say »For screensharing, please use Firefox or Chrome and install the Nextcloud screensharing extension.« – with Firefox and Chrome being linked to the respective pages. This should not be a showNotification which is very far away from the icons but rather an arrowed popover over the icon, just like it looks for the 3-dot-menu
  • in a similar vein, the error message »Screensharing is not supported by your browser.« shows when using Firefox and a HTTP instance. Correct would be »Screensharing is only possible over https«. Again, as a popover over the icon instead of a showNotification on the other edge of the screen.
  • publish the extensions of course, and give it a Nextcloud icon (like the favicon)

@jancborchardt

Copy link
Copy Markdown
Member

Icon PR at nextcloud/server#3480 and stable11 backport at nextcloud/server#3481

Let me know if you need anything else here. :)

fancycode and others added 6 commits February 16, 2017 16:37
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Firefox needs the domain to be whitelisted, so this is done by the addon.

Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Joachim Bauch <bauch@struktur.de>
Signed-off-by: Ivan Sein <ivan@nextcloud.com>
Signed-off-by: Ivan Sein <ivan@nextcloud.com>
@Ivansss

Ivansss commented Feb 16, 2017

Copy link
Copy Markdown
Member

Rebased to use SimpleWebRTC v2.2.3.

Signed-off-by: Ivan Sein <ivan@nextcloud.com>
Signed-off-by: Ivan Sein <ivan@nextcloud.com>
Signed-off-by: Ivan Sein <ivan@nextcloud.com>
@Ivansss

Ivansss commented Feb 24, 2017

Copy link
Copy Markdown
Member

@jancborchardt Could you please take a look to the current state? I would like to know your opinion about the layout when a screen is being shared and discuss how to improve it 😄

Ivansss added 2 commits March 1, 2017 16:04
Signed-off-by: Ivan Sein <ivan@nextcloud.com>
Signed-off-by: Ivan Sein <ivan@nextcloud.com>
@Ivansss Ivansss requested a review from jancborchardt March 4, 2017 15:27
@nickvergessen

Copy link
Copy Markdown
Member

Addons are now published:

@jancborchardt

jancborchardt commented Mar 10, 2017

Copy link
Copy Markdown
Member

@Ivansss I’m using Google Chrome 55 with the extension on Ubuntu 16.04 and on clicking the »Share screen« it says »Screensharing is not supported by your browser«. :\

But generally judging from the screenshots it looks great! :) There’s just two things:

  • I would change the background of the content from this grey to black. (Below the shared screen)
  • When video of one participant is deactivated, the layout of the current speaker avatar is wrong:
    Master (correct):
    capture du 2017-03-10 15-25-52
    This branch (see the avatar on the bottom and a bit to the right):
    capture du 2017-03-10 15-27-08

I can look into both, but for that I have to get the actual screensharing working :\ we could also merge this first and polish it forward?

@nickvergessen

Copy link
Copy Markdown
Member

Works fine here, are you having your netflix-cheat in place again?

@jancborchardt

Copy link
Copy Markdown
Member

@nickvergessen no, I also just disabled all extensions and it’s the same issue. We are also trying on @gsambrotta’s setup and couldn’t get a video call to run at all yet :\ (@gsambrotta best open an issue about it).

I’ll try if it works with Firefox.

@jancborchardt

Copy link
Copy Markdown
Member

Oh and screensharing doesn’t work for public call guests, does it? Because the icon is shown and nothing happens on clicking it.

@Ivansss

Ivansss commented Mar 10, 2017

Copy link
Copy Markdown
Member

It should work for guests too, but I noticed that notifications are not shown for guests.

So probably in your case »Screensharing is not supported by your browser« notification is being triggered, but you can't see it because of the problem with notifications for guests.

@gsambrotta

Copy link
Copy Markdown
Member

no problem from my side, just wrong branch (oooops!)

@jancborchardt

Copy link
Copy Markdown
Member

@Ivansss ah ok, can you then make sure the notification – if it’s not supported – is also shown for guests?

Will need to see what’s wrong with my setup then 😕

Signed-off-by: Jan-Christoph Borchardt <hey@jancborchardt.net>
@jancborchardt

Copy link
Copy Markdown
Member

@Ivansss @nickvergessen ok, I changed the grey background to black in the CSS.

If there’s no layout issues for you, and the rest of the issues in my other comment at #227 (comment) are fixed (or should be postponed) and the notification about screensharing not being available is also shown for guests → we can merge this! :)

Signed-off-by: Ivan Sein <ivan@nextcloud.com>
@Ivansss Ivansss merged commit d884803 into master Mar 14, 2017
@Ivansss Ivansss deleted the screensharing branch March 14, 2017 19:33
@MariusBluem MariusBluem mentioned this pull request Mar 23, 2017
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.

6 participants