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

Add support for the Handy APIv2 #2193

Merged
merged 7 commits into from
Apr 3, 2022

Conversation

chetbrinko
Copy link
Contributor

@chetbrinko chetbrinko commented Jan 1, 2022

Docs: https://staging.handyfeeling.com/api/handy/v2/docs/
Ref: https://github.com/defucilis/thehandy

Update axios to 0.24.0 due to a security update

Progress on #1376

I've updated https://github.com/defucilis/thehandy with the change defucilis/thehandy#2 because the class HandyUtils wasn't exported, which is why this is a Draft PR. In order to test/verify the change you will need to use my branch until it is merged upstream

Also I added support for the following funscript features:

  • inverted flag
  • range argument

@chetbrinko
Copy link
Contributor Author

@UnluckyChemical765 btw thank you for your earlier work on which this is based

@kermieisinthehouse kermieisinthehouse added feature Pull requests that add a new feature improvement Something needed tweaking. javascript labels Jan 1, 2022
@inventiv3919
Copy link

Do you have a docker container of this by any chance?

@chetbrinko
Copy link
Contributor Author

@kermieisinthehouse
Copy link
Collaborator

Testing is welcome, as none of the devs have the hardware. We can only review the code, not the functionality

@inventiv3919
Copy link

Trying to launch the docker container gives error: standard_init_linux.go:230: exec user process caused: exec format

@chetbrinko
Copy link
Contributor Author

@inventiv3919 sorry, the docker image was built for arm - i forced it to amd64 for you so if you repull it should work - i can't verify easily as i'm on arm

@chetbrinko
Copy link
Contributor Author

@kermieisinthehouse understood - all I can say is "works on my machine/hardware" from since I opened the PR - hopefully @inventiv3919 can provide external validation.

@inventiv3919
Copy link

@chetbrinko thanks :) I'll try test it out later today

@inventiv3919
Copy link

inventiv3919 commented Jan 8, 2022

Just fired it up and watched a few videos, seems to work perfectly so far. I was able to change my funscript offset from -655 down to -50, definitely a big improvement

Edit: been using it for a few days now and it's still working great, sync is better and I haven't noticed any bugs.

@inventiv3919
Copy link

@chetbrinko @kermieisinthehouse I've been using it all week with no issues :)

@kermieisinthehouse
Copy link
Collaborator

Given how simple the bindings are, we could probably host the repo under the stashapp org.

@chetbrinko
Copy link
Contributor Author

chetbrinko commented Feb 7, 2022

@kermieisinthehouse feel free to fork my fork into the org, if you please. Happy to be added as a contributor to maintain it.

Once that's done I'll mark the PR as non-draft and we can continue with getting it merged~

@inventiv3919
Copy link

@kermieisinthehouse any chance you could fork this pr now? So many new features have been added to stash and I can't update to use any of them until this is added to the main stash branch

@WithoutPants
Copy link
Collaborator

Is there any reason we can't use the fork as-is until it is patched upstream?

@inventiv3919
Copy link

@WithoutPants I've been using @chetbrinko 's docker container since January and haven't noticed any bugs/glitches. I'm just a bit impatient and would like to use some of the other new features I see have been added since then

@WithoutPants WithoutPants marked this pull request as ready for review April 1, 2022 01:47
@WithoutPants
Copy link
Collaborator

I have update to sync with the develop branch. I've changed it to use thehandy 1.0.3 package and re-implemented uploadCsv so that HandyUtils does not need to be exported for this. Once the pending PR is merged upstream, we can change the code to reference that instead.

This will need a retest from @chetbrinko, @inventiv3919 or someone else with the necessary hardware. Be aware that the shift to videojs may have introduced issues with interactive functionality, so if bugs do occur it may be worth comparing it with the current development build.

@WithoutPants WithoutPants added this to the Version 0.14.0 milestone Apr 1, 2022
@inventiv3919
Copy link

I'll try it out once @chetbrinko updates the docker container

@WithoutPants
Copy link
Collaborator

@inventiv3919 I made a docker image for you to test with: withoutpants/stash:handyv2
(repo here: https://hub.docker.com/repository/docker/withoutpants/stash)

@inventiv3919
Copy link

Thanks for making the docker image @WithoutPants

The shift to videojs has indeed introduced an issue: the handy connects and syncs fine and responds accordingly when the stash player is paused/resumed however seeking ahead/back does not update the handy and there doesn't appear to be a way to resync it without reloading the page and not skipping through at all.

@chetbrinko may need to update the PR so that videojs exposes the current timepoint to the handyapi

@WithoutPants
Copy link
Collaborator

Thanks for making the docker image @WithoutPants

The shift to videojs has indeed introduced an issue: the handy connects and syncs fine and responds accordingly when the stash player is paused/resumed however seeking ahead/back does not update the handy and there doesn't appear to be a way to resync it without reloading the page and not skipping through at all.

@chetbrinko may need to update the PR so that videojs exposes the current timepoint to the handyapi

Pausing the video and then playing again does not sync it again?

@inventiv3919
Copy link

It does not. It pauses it but when I click play again the handy continues without resyncing

@WithoutPants
Copy link
Collaborator

Are you able to test while looking at the browser development console?

Under the network tab, there should be requests for hssp/play and hssp/stop. When the video starts or seeks, I expect a call to hssp/play with estimatedServerTime and startTime filled in.

Can you verify what requests are sent when starting, pausing, seeking and pausing after seeking?

@inventiv3919
Copy link

inventiv3919 commented Apr 3, 2022

Ok I think it's a problem with Samsung internet when I used chrome on my laptop everything except the quick skip buttons worked (and then it resynced by pausing then resuming). Seems to also be working on chrome for Android so I'll switch to that browser. Sorry for wasting your time

(Oh and I checked the network tab and yes it's sending the requests with estimatedservertime & starttime filled in)

@WithoutPants
Copy link
Collaborator

So this PR is good to go? Not sure why the skip buttons aren't working correctly.

@inventiv3919
Copy link

I'd say so yeah, the skip buttons aren't a big deal

@WithoutPants WithoutPants merged commit 6ebf3dd into stashapp:develop Apr 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull requests that add a new feature improvement Something needed tweaking.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants