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.

Sign up for GitHub

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

Add setSource method #153

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
+10 −0
Draft

Add setSource method #153

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09fe160
Add setSource method
rtritto Feb 18, 2025
95e1443
Merge branch 'main' into add-setSource
yoriiis Feb 20, 2025
bdbad05
Move setSource method from player core to html5 provider
rtritto Feb 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,7 @@ The player instance exposes the following methods, accessible when the player is
| `mute()` | - | - | Mute the volume |
| `unMute()` | - | - | Unmute the volume |
| `seekTo(time)` | `Number` | - | Seek to a current time in seconds |
| `setSource(source)` | `String` | - | Set the media source |
Copy link
Member

Choose a reason for hiding this comment

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

It should be added a mention to specify that it is only available for HTML5 video (and audio?)

You can get inspiration from similar examples in the README

https://github.com/vlitejs/vlite/blob/main/README.md?plain=1#L266-L270

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be implemented also for other non-HTML5 providers?

Copy link
Member

Choose a reason for hiding this comment

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

I've check and other providers have method to dynamically change the video source. Good news! And I prefer have common use case between providers.

The change can finally be in Player class but the same method needs to work with all providers. You can inspired from other method for the logic. Their method seems to not be a Promise.

The commun parameter seems to be videoId which contains the new video ID.

Example of implementation:

diff --git a/README.md b/README.md
index 669135a..aa3497d 100644
--- a/README.md
+++ b/README.md
@@ -295,6 +295,7 @@ The player instance exposes the following methods, accessible when the player is
 | `getDuration()`        |         -          | `Promise` | Get the duration                  |
 | `mute()`               |         -          |     -     | Mute the volume                   |
 | `unMute()`             |         -          |     -     | Unmute the volume                 |
+| `setSource()`          |      `String`      |     -     | Set the new source from video ID  |
 | `seekTo(time)`         |      `Number`      |     -     | Seek to a current time in seconds |
 | `requestFullscreen()`  |         -          |     -     | Request the fullscreen            |
 | `exitFullscreen()`     |         -          |     -     | Exit the fullscreen               |
diff --git a/src/core/player.ts b/src/core/player.ts
index 82598ab..e715ff5 100644
--- a/src/core/player.ts
+++ b/src/core/player.ts
@@ -168,6 +168,13 @@ export default abstract class Player {
 	 */
 	public abstract methodUnMute(): void
 
+	/**
+	 * methodSetSource
+	 * Extends by the provider
+	 * @abstract
+	 */
+	public abstract methodSetSource(videoId: string): void
+
 	/**
 	 * On the player is ready
 	 */
@@ -455,6 +462,14 @@ export default abstract class Player {
 		this.dispatchEvent('volumechange')
 	}
 
+	/**
+	 * Set the new source of the player
+	 * @param videoId Video ID
+	 */
+	setSource(videoId: string) {
+		this.methodSetSource(videoId)
+	}
+
 	/**
 	 * Update the current time of the media element
 	 * @param newTime New current time of the media element
diff --git a/src/providers/vimeo/vimeo.ts b/src/providers/vimeo/vimeo.ts
index ba59476..8c2ea7d 100644
--- a/src/providers/vimeo/vimeo.ts
+++ b/src/providers/vimeo/vimeo.ts
@@ -147,6 +147,10 @@ export default function VimeoProvider(Player: any) {
 			})
 		}
 
+		methodSetSource(id: string) {
+			this.instance.loadVideo(id)
+		}
+
 		/**
 		 * Play method of the player
 		 */

Copy link
Member

Choose a reason for hiding this comment

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

videoId parameter name is okay for Youtube, Vimeo and Dailymotion but not for HTML5 which have a video url. Needs to adapt

| `requestFullscreen()` | - | - | Request the fullscreen |
| `exitFullscreen()` | - | - | Exit the fullscreen |
| `getInstance()` | - | - | Get the player instance |
Expand Down
9 changes: 9 additions & 0 deletions src/core/player.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,6 +477,15 @@ export default class Player {
this.methodSeekTo(newTime)
}

/**
* Set the media source
* @param source New media source URL
*/
setSource(source: string) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if the method should be here, because the Player class is used by all providers with inheritance (Youtube, Dailymotion, HTML5, etc.) and this is a feature only for HTML5 video and audio. This is the first time this case has arisen I believe.

This must also be documented. I will check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe can help:
https://github.com/videojs/video.js/blob/8842d376ed3d834ad3ce0a5e662fe446513ea0bd/src/js/tech/html5.js#L1932-L1942
Related HTML5 doc: https://html.spec.whatwg.org/multipage/media.html#dom-media-src

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Video.js method is setSource

Copy link
Contributor Author

@rtritto rtritto Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe, if this is a feature only for HTML, it can not be added but the doc should at least be updated with related use cases on how can be done.
I wrote below my use case with HTML and where I have some problem.

Copy link
Member

Choose a reason for hiding this comment

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

It can be added, but the player file is a bridge between all provider. If the method is for only one provider, it should be is the dedicated provider (html5)

Copy link
Contributor Author

@rtritto rtritto Feb 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If the method is for only one provider, it should be is the dedicated provider (html5)

Agree.

Can the providers be used as modular plugin? (new feature)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a commit to move setSource method from player core to html5 provider.

this.media.src = source
this.media.load()
}

/**
* Request the fullscreen
*/
Expand Down