Support to turn off loop and access HTML5 events/common methods #128

rafa8626 · 2017-03-03T03:09:40Z

This PR solves #87

…sending play event and updated demo file

googlebot · 2017-03-03T03:09:44Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

rafa8626 · 2017-03-03T03:11:22Z

I signed it!

googlebot · 2017-03-03T03:11:23Z

CLAs look good, thanks!

rafa8626 · 2017-03-14T05:07:28Z

@borismus Any updates on this?

rafa8626 · 2017-03-24T01:56:47Z

Is there anything I need to do for this to get merged? I'd like to use this with a more robust player

ArthurvS · 2017-03-30T11:37:12Z

Hi Ron,

I download your changes to have the video duration checker in my own project but i noticed 2 minor errors when using a 360 image instead of video.
Your duration function doesn't check if the video-proxy has been initiated.
In the world-render.js on line 75 you can add an if(this.scene.video)
And in the main.js you just have to place your is ready check inside the if(event.videoElement)

If you want i can make a pull request for these 2 minor bugg fixes. Thanks for the feature it works really nice and is extremely helpfull

rafa8626 · 2017-03-30T11:47:53Z

Yes please feel free to create a pull request and I will be happy to review it and merge it

tommytee · 2017-03-31T12:32:39Z

hi ron666, i forked your fork. added a scrubber bar (and some other stuff).
https://github.com/tommytee/vrview

rafa8626 · 2017-03-31T12:54:21Z

@tommytee Great! I'll take a look at it soon.

rafa8626 · 2017-04-10T04:03:51Z

Hi @ArthurvS any updates on the PR to fix both things you mentioned me above?

ArthurvS · 2017-04-10T08:08:58Z

Hi @rafa8626,
Sorry for the delay, had a busy week! It is coming your way right now.
Cheers
Arthur

…luded, better to do this locally at merge, cheers

…into ArthurvS-360Image_bugfix

googlebot · 2017-04-10T10:17:16Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

rafa8626 · 2017-04-10T10:19:29Z

@ArthurvS can you just sign the CLA and confirm you are OK with these changes being contributed to Google, please? Otherwise if they decide to merge this with master, the cannot merge it. Use this link Please visit https://cla.developers.google.com/ to sign.

ArthurvS · 2017-04-10T11:01:45Z

I signed it!

… not included, better to do this locally at merge, cheers" This reverts commit 03a2c7c.

rafa8626 · 2017-04-10T11:08:11Z

@borismus @ArthurvS signed the CLA but I still see that the commit is marked as failure. What can I do to fix this?

googlebot · 2017-04-10T11:08:14Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

rafa8626 · 2017-04-10T11:10:47Z

@ArthurvS sorry to bother you once more; just comment one more time including this text: I'm okay with my commits being contributed to this project.

ArthurvS · 2017-04-10T11:24:49Z

@rafa8626, np! happy to help!
@googlebot:
I'm okay with my commits being contributed to this project.,

rafa8626 · 2017-04-10T11:34:19Z

Thanks!

kanaabe · 2017-04-17T00:36:03Z

Thanks for your work on this! I just pulled down your fork to play around with your changes but I'm seeing a console error when I test locally. Is there something I'm missing?

The error points to:

rafa8626 · 2017-04-17T03:17:41Z

@kanaabe Thanks for pointing this. I had a little merge error but now it's fixed; if you download the latest master branch from my fork you'll see this is solved

rafa8626 · 2017-04-27T23:44:04Z

@borismus Any chances of this getting merged in the master branch soon?

borismus · 2017-04-29T00:38:16Z

examples/video/index.js

@@ -45,6 +61,9 @@ function onVRViewReady() {
  } else {
    playButton.classList.remove('paused');
  }
+
+  // Set media on 00:02
+  vrView.setCurrentTime(2);


Why not start at the beginning?

Because it was to demonstrate the use of the method at a different time

borismus

Left a few comments.

borismus · 2017-04-29T00:38:39Z

examples/video/index.js

@@ -67,4 +86,20 @@ function onToggleMute() {
  muteButton.classList.toggle('muted');
 }

+function formatTime (time) {
+


Clean up random spaces.

borismus · 2017-04-29T00:38:52Z

examples/video/index.js

+
+  var minutes = Math.floor(time / 60) % 60,
+      seconds = Math.floor(time % 60)
+  ;


Style: random semicolon?

No it's the closing one but I tend to leave it separately in the event I need to add more variables

borismus · 2017-04-29T00:39:19Z

src/api/player.js

@@ -47,6 +47,11 @@ function Player(selector, contentInfo) {
  // Expose a public .isPaused attribute.
  this.isPaused = false;

+  // Other public attributes
+  this.time_ = {currentTime: 0, duration: 0};


Stick to the convention established in the rest of the file (this.time instead of this.time_)

Also simplify by having this.currentTime and this.duration.

Ok I'll separate them in 2 variables

borismus · 2017-04-29T00:39:30Z

src/api/player.js

@@ -75,6 +80,10 @@ Player.prototype.addHotspot = function(hotspotId, params) {
  this.sender.send({type: Message.ADD_HOTSPOT, data: data});
 };

+/**
+ * HTML5 API


Pointless comment?

I meant to expand the explanation on this but it can be removed

borismus · 2017-04-29T00:39:38Z

src/api/player.js

@@ -83,6 +92,10 @@ Player.prototype.pause = function() {
  this.sender.send({type: Message.PAUSE});
 };

+/**
+ * Equivalent of HTML5 setSrc()


Use periods at the end of sentences.

borismus · 2017-04-29T00:39:47Z

src/api/player.js

@@ -101,6 +114,29 @@ Player.prototype.setVolume = function(volumeLevel) {
  this.sender.send({type: Message.SET_VOLUME, data: data});
 };

+Player.prototype.getVolume = function() {
+	return this.volume_;


Use consistent spacing (2 spaces)

borismus · 2017-04-29T00:42:12Z

src/embed/main.js

@@ -36,6 +36,7 @@ receiver.on(Message.PAUSE, onPauseRequest);
 receiver.on(Message.ADD_HOTSPOT, onAddHotspot);
 receiver.on(Message.SET_CONTENT, onSetContent);
 receiver.on(Message.SET_VOLUME, onSetVolume);
+receiver.on(Message.SET_CURRENT_TIME, onUpdateTime);


should be updateCurrentTime

borismus · 2017-04-29T00:42:52Z

src/embed/main.js

-      type: 'ready'
+      type: 'ready',
+      data: {
+        duration: event.videoElement.duration


Why do we provide duration in the ready call?

Because I tried to make sure it's been set ahead of time. It's the behavior loadedmetadata will produce so the intention was to mimic it

borismus · 2017-04-29T00:43:49Z

src/embed/video-proxy.js

+VideoProxy.prototype.getCurrentTime = function() {
+  return {
+    currentTime:  Util.isIOS9OrLess() ? this.audioElement.currentTime : this.videoElement.currentTime,
+    duration: Util.isIOS9OrLess() ? this.audioElement.duration : this.videoElement.duration


Duration doesn't make sense to keep querying. It doesn't change.

Agree. I'll remove it

borismus · 2017-04-29T00:44:16Z

src/embed/adaptive-player.js

@@ -34,10 +34,12 @@ var DEFAULT_BITS_PER_SECOND = 1000000;
 *
 * To play/pause/seek/etc, please use the underlying video element.
 */
-function AdaptivePlayer() {
+function AdaptivePlayer(loop) {


This would be better as an opt_params with {loop: true}, for future proofness.

Ok I see your point. I'll turn it into params

rafa8626 · 2017-04-29T03:42:17Z

I'll make the changes soon so this can be merged

rafa8626 · 2017-04-29T03:42:30Z

Thanks for reviewing this

… expand configuration of VR player

rafa8626 · 2017-04-29T15:54:54Z

@borismus Please review this one more time. I have addressed all the issues you pointed out. Thanks.

rafa8626 · 2017-05-01T11:02:40Z

@borismus I just performed one last modification to the PR to solve some issues when using only images. Let me know if this is good to merge

rafa8626 · 2017-05-04T11:18:47Z

@borismus I added one last fix to display the hotspots since they were not showing up. Any updates on this?

rafa8626 · 2017-05-12T10:35:23Z

@borismus Any updates on this?

borismus

Some more nits.

borismus · 2017-05-12T11:06:25Z

examples/video/index.js

+    console.log('media paused');
+  });
+  vrView.on('timeupdate', function(e) {
+    document.querySelector('#time').innerText = formatTime(e.currentTime) + ' | ' + formatTime(e.duration);


Change done

borismus · 2017-05-12T11:06:40Z

examples/video/index.js

+function formatTime (time) {
+  time = !time || typeof time !== 'number' || time < 0 ? 0 : time;
+
+  var minutes = Math.floor(time / 60) % 60,


Use multiple var; declarations

Change done

rafa8626 · 2017-05-12T11:20:53Z

@borismus All changes requested have been completed. Is this good to be merged?

rafa8626 · 2017-05-12T11:25:06Z

Thanks @borismus

ArthurvS · 2017-05-12T11:27:27Z

Thanks @borismus & @rafa8626

Implemented time events and methods to mimic HTML5; fixed issue when …

67d2bc5

…sending play event and updated demo file

rafa8626 changed the title ~~Add VR View API to prevent video looping and access HTML5 events~~ Support to turn off loop and access HTML5 events/common methods Mar 3, 2017

This was referenced Mar 3, 2017

How to show video progress? #121

Closed

Add VR View API to prevent video looping and access HTML5 events #87

Closed

Fixed conflicts

f87ae50

rafa8626 mentioned this pull request Mar 29, 2017

[Request] onLoaded event #137

Open

ArthurvS and others added 4 commits April 10, 2017 10:45

Removes bug when using images with this change. Builded files not inc…

03a2c7c

…luded, better to do this locally at merge, cheers

Merge branch '360Image_bugfix' of https://github.com/ArthurvS/vrview …

0284396

…into ArthurvS-360Image_bugfix

Merge branch 'ArthurvS-360Image_bugfix'

3fd3bf9

Built files to reflect latest changed

78868c1

Revert "Removes bug when using images with this change. Builded files…

35f8924

… not included, better to do this locally at merge, cheers" This reverts commit 03a2c7c.

Fixed merge issues

0a295c4

borismus reviewed Apr 29, 2017

View reviewed changes

Addressed minor formatting issues and integrated argument player to…

45fbac6

… expand configuration of VR player

Rafael Miranda added 3 commits April 29, 2017 12:16

Enabled play button once media ended

284e708

Fixed typo with volume variable

aabd448

Fixed minor issue with encoding object as param

748d16b

Rafael Miranda added 2 commits May 1, 2017 10:01

Fixed issues when using VRView with images only

a1a7cd7

Added fix to show hotspots properly

9fd2f0a

borismus suggested changes May 12, 2017

View reviewed changes

Addressed minor formatting issues

b5639b9

borismus merged commit ead7d94 into googlearchive:master May 12, 2017

Support to turn off loop and access HTML5 events/common methods #128

Support to turn off loop and access HTML5 events/common methods #128

Conversation

rafa8626 commented Mar 3, 2017 • edited Loading

googlebot commented Mar 3, 2017

rafa8626 commented Mar 3, 2017

googlebot commented Mar 3, 2017

rafa8626 commented Mar 14, 2017

rafa8626 commented Mar 24, 2017

ArthurvS commented Mar 30, 2017

rafa8626 commented Mar 30, 2017

tommytee commented Mar 31, 2017

rafa8626 commented Mar 31, 2017

rafa8626 commented Apr 10, 2017

ArthurvS commented Apr 10, 2017

googlebot commented Apr 10, 2017

rafa8626 commented Apr 10, 2017 • edited Loading

ArthurvS commented Apr 10, 2017

rafa8626 commented Apr 10, 2017

googlebot commented Apr 10, 2017

rafa8626 commented Apr 10, 2017

ArthurvS commented Apr 10, 2017

rafa8626 commented Apr 10, 2017

kanaabe commented Apr 17, 2017

rafa8626 commented Apr 17, 2017

rafa8626 commented Apr 27, 2017

borismus Apr 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borismus 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

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

rafa8626 commented Apr 29, 2017

rafa8626 commented Apr 29, 2017

rafa8626 commented Apr 29, 2017

rafa8626 commented May 1, 2017 • edited Loading

rafa8626 commented May 4, 2017

rafa8626 commented May 12, 2017

borismus 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

rafa8626 commented May 12, 2017

rafa8626 commented May 12, 2017

ArthurvS commented May 12, 2017

rafa8626 commented Mar 3, 2017 •

edited

Loading

rafa8626 commented Apr 10, 2017 •

edited

Loading

borismus Apr 29, 2017 •

edited

Loading

rafa8626 commented May 1, 2017 •

edited

Loading