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

Prevent mobile browsers from sleeping during transfer #125

Merged
merged 4 commits into from
Jan 19, 2023
Merged

Conversation

JustusFT
Copy link
Contributor

Ref: #120

@JustusFT JustusFT requested a review from donpui January 12, 2023 08:51
@donpui
Copy link
Contributor

donpui commented Jan 12, 2023

I have tested on iOS (16.2), but it didn't work, phone still went sleep.

However, I see there is PR which is fixing iOS problem: richtr/NoSleep.js#138, I will try with it.
Tried with replace package:
https://www.npmjs.com/package/@uriopass/nosleep.js

Still not working with. JS error:

NotAllowedError
The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.

Seems repo https://github.com/richtr/NoSleep.js abandoned by owner and a lot of PR hanging without merge.

@donpui
Copy link
Contributor

donpui commented Jan 12, 2023

I have tested on iOS (16.2), but it didn't work, phone still went sleep.

However, I see there is PR which is fixing iOS problem: richtr/NoSleep.js#138, I will try with it. Tried with replace package: https://www.npmjs.com/package/@uriopass/nosleep.js

Still not working with. JS error:

NotAllowedError
The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.

Seems repo https://github.com/richtr/NoSleep.js abandoned by owner and a lot of PR hanging without merge.

Seems Safari (webkit) requires allowsInlineMediaPlayback="true" react-native-webview/react-native-webview#1251, https://developer.apple.com/documentation/webkit/wkwebviewconfiguration/1614793-allowsinlinemediaplayback?language=objc
@JustusFT can we add?

@donpui
Copy link
Contributor

donpui commented Jan 12, 2023

I have tested on iOS (16.2), but it didn't work, phone still went sleep.
However, I see there is PR which is fixing iOS problem: richtr/NoSleep.js#138, I will try with it. Tried with replace package: https://www.npmjs.com/package/@uriopass/nosleep.js
Still not working with. JS error:

NotAllowedError
The request is not allowed by the user agent or the platform in the current context, possibly because the user denied permission.

Seems repo https://github.com/richtr/NoSleep.js abandoned by owner and a lot of PR hanging without merge.

Seems Safari (webkit) requires allowsInlineMediaPlayback="true" react-native-webview/react-native-webview#1251, https://developer.apple.com/documentation/webkit/wkwebviewconfiguration/1614793-allowsinlinemediaplayback?language=objc @JustusFT can we add?

Seems plugin example works on iOS: https://richtr.github.io/NoSleep.js/example. So maybe issue on our side?

@JustusFT
Copy link
Contributor Author

@donpui I'm unable to check the problem since I don't own any ios device. It works on my android device though.

All I can guess is that video playback is being blocked, but I can't test it

@donpui
Copy link
Contributor

donpui commented Jan 13, 2023

We can pair and experiment together. In NoSleep.js documentation, there is saying:
NOTE: This function call must be wrapped in a user input event handler e.g. a mouse or touch handler

@JustusFT
Copy link
Contributor Author

@donpui Since ios blocks video playback if not tied to user input, I have changed nosleep so that it activates on button presses.

I tested the following scenarios on android and it works, hopefully it works on ios as well.

  • activates when sender clicks the + button
  • deactivates if upload fails (file too large or multiple files)
  • activates when receiver clicks "Next"
  • activates when receiver submits code by hitting the enter key
  • deactivates if submitted code is invalid
  • activates when receiver accepts a file (clicks "Download")
    • unfortunately in cases where the user uses a link like https://winden.app/#7-guitarist-revenge , this is the earliest moment to get a user input. This scenario doesn't work if they don't click the download button before the screen locks.
  • deactivates on transfer completion and cancellation.

@JustusFT JustusFT requested a review from donpui January 16, 2023 07:12
@donpui
Copy link
Contributor

donpui commented Jan 16, 2023

Tested on iOS device with Safari, Chrome, Firefox:

  • Waiting for code accepted, sending from iPhone to workstation (note: manage to send 1gb file from phone to workstation)
  • Waiting to accept file on iPhone;
  • Receiving file to iPhone (note: large file receive still fails);

NoSleeps works for mobile devices, should check mobile browsers/device and do not enable/disable wakelock. Or do we leave for nosleep package to take care?

@donpui
Copy link
Contributor

donpui commented Jan 16, 2023

Several concerns, which we might need to think:

  • nosleep.js main repo doesn't have active maintenance, however community quite big and there are PRs and forks, which solves one or other issue. Not sure if we should use this or move to one of forks.
  • such functionalities, require error logging, as we will not be aware if it works properly on users side or spams JS/TS errors.
  • not sure if we can have somekind of tests for this feature (for E2E, we would need device with short screen time, fore integration tests, maybe we can just check wake status)

@JustusFT
Copy link
Contributor Author

Since our testing seems to show that it works on the common mobile os (Android, iOS) and browsers (Safari, Chrome, Firefox), I think this pr is ok to merge, maybe after adding some tests. If we find any issues, we can review the existing forks to use or make our own if needed.

We don't have any setup for automated e2e for mobile devices, but I can add integration tests that can check the NoSleep state

@donpui
Copy link
Contributor

donpui commented Jan 17, 2023

We don't have any setup for automated e2e for mobile devices, but I can add integration tests that can check the NoSleep state

Lets add at least integration test.

@JustusFT
Copy link
Contributor Author

Copying my message here (TL;DR we'll hold off adding the tests for now: #128)

I encountered some problems. The code isn't really testable in the desired layer (integration tests with react-testing-library)
The best way to test nosleep deactivation is by

  1. mocking nosleep,
  2. mounting the whole app with react-testing-library,
  3. simulate a cancelled or completed transfer.
  4. then we can verify that the mock was called at the right times.

But to do this, we have to stub out go, wasm, the webworker, some of the react context providers, etc. And I'm having trouble getting them all stubbed.
I'm currently refactoring the code so these parts could be easily be stubbed out, but it's taking me a while...
What is possible right now is to just mount as little of the ui as needed (instead of the whole app), and check for the mock calls like that, but I would say such tests would be too fragile and not that useful. (for example, the test would break every time a nosleep call moved somewhere else, even though it still works. I would have to re-do the test setup)
We can also make nosleep global and check the value through e2e, but that isn't ideal situation because we should avoid testing implementation details at the e2e layer.
(also, one thing that would help is to finally add state management because state is quite scattered at the moment. I ended up spending quite some time yesterday and today looking into solutions for that as well. I know we were gonna hold off any changes related to the worker until dilation and multiple file transfer since these parts would need to be rewritten by then anyway, but now I'm thinking it's a good idea if we refactor now, just so testing could be easier.)

@donpui
Copy link
Contributor

donpui commented Jan 19, 2023

Adding to this, we would very much also need E2E running on real devices, issue: #106. As even if we test states and activation, we are not fully sure if it works for end users. For example WakeLock API is very differently supported via different browsers and in some case workarounds are implemented inside library.

Copy link
Contributor

@donpui donpui left a comment

Choose a reason for hiding this comment

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

All the rest things looks ok. Improvement is important for mobile devices users.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants