Generate embed codes for Tobira directly instead of for the Opencast player #557
Conversation
JulianKniephoff
force-pushed
the
embed-tobira-player
branch
from
01e254a
to
f699326
Compare
|
馃殌 This PR was deployed at https://pr557.tobira.opencast.org. The deployment will be updated whenever someone pushes onto this PR's branch. |
There was a problem hiding this comment.
Overall the approach makes sense to me and I don't think its too much code duplication. The refactorings also make sense to me. Good jobs keeping atomic and understandable commits, made the review a lot easier I think.
I have not tested it yet, that will be in an additional comment.
I did find a few things that should still be fixed tho. (This review does not include the first three commits which are part of a different PR)
frontend/src/routes/Video.tsx
Outdated
| @@ -233,7 +233,6 @@ type Props = { | |||
|
|
|||
| const VideoPage: React.FC<Props> = ({ eventRef, realmRef, basePath }) => { | |||
| const { t } = useTranslation(); | |||
| const rerender = useForceRerender(); | |||
There was a problem hiding this comment.
That change aint good. I put it here specifically to redraw more than just the player. It's also about the time/date information below the player (i.e. in the <Metadata> component). For the video block it's probably fine to do it only in the Player, but here we specifically need to rerender the whole player page.
There was a problem hiding this comment.
Makes sense. I pushed another commit.
|
Ok, I tested briefly. I didn't check every possible video state but I assume you did. Thanks for including the test page commit, that was helpful! I only got one new thought: Forbid embedding all other pages... I like the idea but maybe we take too much freedom away from people? Like if they want to embed a specific page and it works for them... why not? |
github-actions
bot
added
the
status:conflicts
This comment was marked as resolved.
This comment was marked as resolved.
Technically this also extends them a bit, but that should be fine.
This way routes not using the `RootLoader` (<cough>embeds</cough>) can define their own loading fallbacks.
This was always used together anyway and it will be so for the embedded player as well. This also makes the comment in the `Player` code make more sense. ;P
This avoids potential race conditions.
This change seems larger than it is mostly because I had to move some stuff around to get rid of a dependency cycle in our module structure which we unfortunately have lots of. This was causing load order inconsistencies and made the app crash.
JulianKniephoff
force-pushed
the
embed-tobira-player
branch
from
f699326
to
f8a1cb5
Compare
github-actions
bot
removed
the
status:conflicts
My thinking was mostly: Don't give people crazy ideas. I could imagine this leading to unintended use cases and absurd requirements. 馃し |
Ok sure, then let's just disallow it for now, it's fine 馃し |
JulianKniephoff
force-pushed
the
embed-tobira-player
branch
from
8c7c1e2
to
871346d
Compare
JulianKniephoff
force-pushed
the
embed-tobira-player
branch
from
871346d
to
abeda17
Compare
Wow, this took way too long. I got super hung up on how exactly to factor this. Turns out: Suddenly being embedded in an iframe breaks a lot of assumptions throughout all our components. In the end, after a long and painful deliberation process, I opted to mostly make the embedded stuff independent of all the rest. I made some obvious refactorings to reuse at least some stuff, but there is also a lot of (additional) duplication, now. (And looking through a lot of the code for this again, that's already somewhat true before this PR, so maybe it's not actually that bad.)
One argument for why this might be okay is that this is a rather ad hoc feature: We only want this to have the nice "stream will start whenever" view when you embed videos from Tobira. However, I think that's actually Paellas responsibility, so maybe we can even take out this code again at some point. On the other hand I also thought about extending the feature to being able to embedding other things, but if we really want that, I think we will have to rethink some of the choices I made here, so I'm kinda betting on us not wanting that. 馃
So what this means concretely is that there is exactly one route now that can be embedded. And in fact, trying to embed other routes will elicit an error. Components don't know about being embedded or not. The whole thing works by the embed-route just pasting together preexisting or factored-out components, or just flat out duplicating some.
The only exception to this is the global error boundary, which lives outside of the router. So if we want that to look nice in an iframe as well, it has to know whether it's embedded or not, instead of just relying on routing logic.
Anyway, I'm glad this is (mostly) over with, now. 馃槱
Meta
Closes #524.
Based on #554.
Can and should be reviewed commit by commit, which should also make clear when/how/what the whole thing does.
Note that the last commit is for demo/testing purposes only and should be removed before this is merged.