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.

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

1.0.0 Refactor (Issue #36) #54

Merged
merged 174 commits into from
Feb 9, 2020

Conversation

Lindsay-Needs-Sleep
Copy link
Collaborator

@Lindsay-Needs-Sleep Lindsay-Needs-Sleep commented Nov 18, 2019

I am excited to present to you the 1.0.0 re-write of the plugin! The plugin now follows the chrome.cast api much more closely! And (as far as I am aware) works without issue!

I say re-write because I estimated that ~80% of Android was changed. And iOS was moved to objc to fit the cordova plugin standard (so a ~100% rewrite).

There should be very few changes required to migrate from 0.x to 1.0.0. Mostly if you depended on getRouteListElement you will have to switch to scanForRoutes/stopScan, or if you depended on behavior not found on chrome desktop's api implementation.

These changes have been tested on:

  • Android 4.4, 7.x
  • iOS 9.3 (iPad 3), 10.3 (iPhone 5), 12.4.2 (iPad mini 2), 13.2.2 (emulator iPad 7, emulator iPhone 8)
  • Chrome desktop browser 78.0 (the tests pass on this version of chrome, which demonstrates that the same code will work on all 3 platforms [basically])

The largest changes of this refactor are:

  • iOS, Android, and Chrome Desktop all work the same now! (With a few caveats.)
    All 3 will pass the same set of tests included in the plugin.

  • iOS and Android have scanForRoutes, stopScan, and selectRoute to replace getRouteListElement

  • iOS and Android still do not implement the full chromecast API, but a few more have been added, and existing ones have been made to match chrome desktop behavior.

Bug Fixes:

Improvements:

  • Upgraded to CAF v3, this got rid of a huge amount of deprecation notices.
  • Added eslint tests to ensure uniform js code stlying
  • Added a style checker for java
  • Updated documentation on: how to run style checkers, how to run tests.
  • Added a pull request template which instructions prospective pull requesters to run eslint and the tests
  • Repaired and added to the existing tests enforcing chrome desktop SDK behavior.
  • On app initialize you will receive any previously started+active session. (This includes navigating to a new page, or force killing the app and re-loading.)
  • Implemented startRouteScan and stopRouteScan to replace getRoutes (see why)
  • fixed selectRoute for Android 4.4.2
  • Allow native error responses to send a json object "code" and "description" parameters in addition to just the error code string.
  • Implement basic queue functionality
  • Tests can run on chrome desktop browser as well (with a couple stubs to enable missing functionality)
    • This is super useful because you can write tests and check that they work correctly on chrome first. Which is very important when chrome's implementation is what we are implementing.
  • Added a basic code example
  • Updated readme with all kinds of instructions (differences between chrome, api list, plugin-specific api list, api usage example, support+quirks, setup, how to test, how to run formatting test)
  • Added pull request template
  • Added scripts to assist in enforcing code formatting for JS and Java

Auto-tests:

  • initialize
    • receiverListener
    • sessionListener
  • leaveSession
  • stopSession
  • loadMedia Video
  • media.setVolume
  • media.play
  • media.pause
  • media.stop
  • session.setReceiverVolumeLevel
  • session.setReceiverMuted
  • requestSession
  • cordova.startRouteScan
  • cordova.stopRouteScan
  • cordova.selectRoute
  • MediaMetadata (All types with most of their special params)
  • loadMedia Audio (Did not test playback controls)
  • loadMedia Image
  • load queue
  • jump to queue item

Manual Tests:

  • Start session, reload page, call initialize, do we get session again?
  • Start session, force kill app, restart app, do we get session again?
  • Start session, join session with additional device, leave session on original, session still continues?, force kill and restart original device, original device should not rejoin the session automatically.
  • for session Leave: probably need to join the session from an additional device,then leave the session, restart app, does it rejoin the session? (it shouldn't) (pre-req rejoin on restart must work)
  • for loadmedia from external sender: start session on device, join session on other device, loadmedia from other device, is session.addMediaListener triggered?
  • for external exit: Start session, kill session with other device, requestSession should show the the join dialog, not stopCasting

In addition to all the issues listed above, this probably fixes these as well:

Sel-en-ium and others added 30 commits August 2, 2019 07:36
…tyle) (Will satisfy the eslint test in another commit.)
# Conflicts:
#	tests/tests.js
#	www/chrome.cast.js
…ork version. (Most tests fail. Will try to fix in another commit.)

# Conflicts:
#	README.md
#	tests/tests.js
…cess. (Chromecast.java)

We shouldn't prevent requestSession from going through if there is no receiver.  It handles this fine.  It just shows that it is searching for a chromecast.  This follows the behavior of chrome browser cast SDK. (chrome.cast.js)
Remove unused imports. (Chromecast.java)
Try to be a bit more safe with the threads. (Chromecast.java)
Just pass the Chromecast instance to the callback on initialization.  (Chromecast.java and ChromecastMediaRouterCallback.java)
…f power and annoying. Use the cordova log utility instead.
…n though.

Updated the README.md with instructions on how to run these tests.
We shouldn't load the tests.js file when the user is using the plugin for production use.
We shouldn't include the test framework as a dependency.  (Even a dependency of the test plugin.)
Added a pull_request_template.md
Managed to match the desktop chrome SDK behavior regarding receiver updates on start up.
Added some additional tests to cover more of the basic situations with receiver updates.
Simplified all the get route stuff (will fix it up properly later)
capture routeUnselected reason
…good stack trace. So remove them and check error as soon as possible to the criminal code.
…chrome desktop chromecast behavior. We are supposed to receive a callback to the error handler. Not have it silenced. This is important if you have started connection animations. If you never receive a callback you don't know when to stop the animations, and whether to transition back to the unconnected state or to a connected stated. If you wish to ignore the cancel error, just filter it out in your error callback. eg. if (err.code === 'cancel') { //ignore }
…pendencies is not great. Though, we are on our way to a monolith test this way. :/ Pick our poison I guess.
Session is automatically "rejoined" when the webview goes to a new page and calls initialize (this follows chromecast desktop sdk)
Do not call sessionListener on requestSession success (this does not follow chromecast sdk desktop behavior)
(So I changed all the tabs to spaces in the java files.  Sorry commit history. o.o)
It was a lot of work, so I disable a couple of the java checkstyle rules.  They are listed in the first comment in check_style.xml.
Issue jellyfin-archive#36 progress
…ent item index is i, you load items [i-1, i, i+1] *exclude i-1 if it is < 0, and exclude i+1 if it is >= queue.length. (aka. don't wrap the items.) This is to match chrome desktop behavior.

Fixes a bug where larger queues (maybe 20+) result in a loop and queueLoad never returns.  This is also safer because it should not result in a crash due to eating all the memory for extremely large queues.
…alse for successfull preChecks (chrome.cast.js)
…vered this case was effectively skipped. So the test now works properly and you can control the media after an app restart.
…t before the client has received the session. (Can happen on occaision when resuming a session on page reload or app restart).
ios does not have mediaStatus on didResumeCastSession if initialize is called immediately after app restart.  So retry for 2 seconds to see if we can get any media.  If not, we will assume the session does not have any media loaded.
@Lindsay-Needs-Sleep
Copy link
Collaborator Author

@dkanada Sure. I mean, I won't be able to dedicate much more time to this project (but I think it is working pretty good now). I don't mind if we "officially" transfer it. I should be able to answer questions/reviewing PR's. I'm not sure what else ownership entails?

@phpfile
Copy link

phpfile commented Jan 24, 2020

Figure this out because right now when one searches for "cordova chromecast plugin" it's not easy to get to Lindsay's plugin and it should.

@dkanada
Copy link
Member

dkanada commented Jan 25, 2020

Hmmm since your fork is hard to find it might be easier to just handle the issue here. If you consider your work almost finished we could make sure the existing master is tagged with a release, then pull in your changes. If you plan on making more changes in the future we could also give you write access to this repository so you can push to master.

@Lindsay-Needs-Sleep
Copy link
Collaborator Author

Lindsay-Needs-Sleep commented Jan 26, 2020

Haha, yep. I thought it was finished basically every commit since I submitted the pull request, but a couple of users kept finding bugs. Thankfully, the last one was closed yesterday, and no new ones have been reported, so I really, really, think it is done now. (fingers crossed >.<)

Yep, that sounds like a fine plan!

I don't have any plans for the immediate future.
At some point I may come back and do:

  • Support __onGCastApiAvailable #51 (but this one should be pretty easy, so my hope is someelse might do it. :p)
  • Maybe adding support for additional functions (eg. chrome.cast.media.QueueInsertItemsRequest because there seems to limit on message size, so you can't create really large queues [~70+] in one go.)
  • Also, maybe improving the documentation (divide it up between user documentation and developer documentation).

@th3hamm0r
Copy link

th3hamm0r commented Feb 3, 2020

@Lindsay-Needs-Sleep fyi: I've tested your latest master and the crash on iOS (#53) is still happening

edit: I've now created a new issue in your fork with some more information, I hope this helps a bit

@anthonylavado
Copy link
Member

@Lindsay-Needs-Sleep @dkanada Since the current master that we're using is already tagged as a release, I'm willing to:

Let me know what you think.

@Lindsay-Needs-Sleep
Copy link
Collaborator Author

Yep, I am fine with that.
I will write a test for PR 12 in a few days (hopefully) and merge that in as well.

We can wait for that, or increment the version (1.0.1), or whatever the preferred method is. :)

@anthonylavado anthonylavado merged commit 9ab40c2 into jellyfin-archive:master Feb 9, 2020
@Lindsay-Needs-Sleep
Copy link
Collaborator Author

Lindsay-Needs-Sleep commented Feb 9, 2020

@anthonylavado Thanks for the merge and invitation! :D

I was wondering if it would be possible to un-squash the commits in the merge?

I was quite meticulous with writing detailed commit messages so that git blame would be useful in determining exactly why and what importance each piece of code does/holds. I would really hate to lose all of that! (I find it super useful!)

dkanada added a commit that referenced this pull request Feb 9, 2020
@anthonylavado
Copy link
Member

@Lindsay-Needs-Sleep Sorry for the delay - I'm not very good at Git, and I forgot about my GitHub default for squash.

@dkanada Here's a reminder 💯

@dkanada
Copy link
Member

dkanada commented Feb 15, 2020

@Lindsay-Needs-Sleep I reset our master branch to just before your pull request was merged. If you open a new PR with the same changes we can merge it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment