Cleanup cordova modules - webpack fix #197
Conversation
| @@ -99,15 +105,26 @@ define(['playbackManager', 'nowPlayingHelper', 'events', 'connectionManager'], f | |||
| return; | |||
| } | |||
|
|
|||
| // dummy this up | |||
There was a problem hiding this comment.
can you explain the comment?
There was a problem hiding this comment.
well if you are asking if i wrote it, no. The actual meaning.. searching for it in google, i found this: https://www.urbandictionary.com/define.php?term=Dummy-up
There is an implementation to prevent notifications like hell to upade mediaSession info. So this should belong to that implementation. I don't know. I could remove the comment.
There was a problem hiding this comment.
I was wondering what this thing does and what this comment means...
There was a problem hiding this comment.
Well the code transforms init events into timeupdate for the function context. Then what i see is if event type if timeupdate and there as an update with an interval lesser than 5 seconds, then ignore the player state update. This code was in jf-android initially, but not in jf-web, but i found traces of it in jf-web, like the lastUpdateTime defined, but never used. I think it makes sense to not update very frequently the mediasession if event is timeupdate. If we find some issue with the refresh rate of the sticky notification, we should investigate further, and try to decrease the time between updates.
There was a problem hiding this comment.
I got the code about time updates.
So after your explanation I propose to remove // dummy this up and add a comment like // transform "init" event into "timeupdate" within this function context inside the if().
There was a problem hiding this comment.
sure np, will do
|
Take note that the other PR associated with this one, in jf-android, needs this one to properly work. |
There was a problem hiding this comment.
LGTM.
Would also be good if you could fix my suggestion #197 (comment)
|
LGTM too, can you fix up the conflict? |
|
Only when I get in front of my pc, later today. You will have to wait a couple of hours |
|
I detected a bug related with this PR that affects audio play. Please hold the merge till i resolve the issue. |
|
The problem is not in this PR but it's in my already approved PR in jf-android. I made a new PR to address this issue: jellyfin/jellyfin-android#112 |
This PR removes all cordova requires from site.js. This is in sync with jellyfin/jellyfin-android#109. This will solve all naming resolution problems for webpack compilation related with cordova.
The main points that changed are: