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

Added web support #68

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added web support #68

wants to merge 7 commits into from

Conversation

mwoelk
Copy link

@mwoelk mwoelk commented Feb 19, 2024

Added support for flutter web.

Currently when using this plugin directly on web it just crashes. Instead of checking for web use I thought it would be better to open a PR and make this plugin compatible with web as well:

Logic based on https://developer.mozilla.org/en-US/docs/Web/API/Screen/orientation
See also: https://caniuse.com/screen-orientation

If browser support is not given, then we fall back to the provided defaultOrientation.

Looking forward to your feedback/suggestions.

@mwoelk
Copy link
Author

mwoelk commented Apr 18, 2024

Any feedback here?

@rmtmckenzie
Copy link
Owner

rmtmckenzie commented Apr 19, 2024

Hi @mwoelk! Sorry I've not responded to this yet, that looks like a good start but I think there's a few things that should be done before inclusion in the plugin.

  1. According to the docs, the dart:html package is being deprecated and the web package should be used instead. I'd prefer if that were done before being integrated.
  2. This implementation completely ignores the sensor parameter. I suppose it's better than nothing but that's a fairly important part of the plugin. It'd be great to have an implementation based on the Gyroscope API which seems to be supported fairly widely, or at the least throw an UnimplementedError if any of the APIs are called with the sensor parameter set to true.

If you're able to take one or both of those tasks on that'd be wonderful, otherwise I can try to get to it but it probably won't be for at least a week or two.

@mwoelk
Copy link
Author

mwoelk commented Apr 20, 2024

Hey @rmtmckenzie,

  1. Migrated to pacakge:web.
  2. Regarding the sensor parameter I decided against the Gyroscope API because although it might seem to be supported fairly widely it isn't ;) It won't work on any iOS device and even on my Android devices I wasn't able to get it working.
    But there's a sensor-based DeviceOrientationEvent that is supported across all browsers and that at least worked under iOS... But on my Redmi using Chrome it wouldn't work. Need to check out if there are any other APIs that might work here reliably as a fallback. Any suggestions? Otherwise I guess I would just add a fallback to the non-sensor orientation.

Testing with https://sensor-js.xyz/demo.html shows that using DeviceMotionEvent.accelerationIncludingGravity seems to be available on most devices BUT signs between chrome and safari seem to vary but might be fixed with some device checking.

@rmtmckenzie
Copy link
Owner

Awesome, thanks for that, that looks great, although what you did wasn't working on my (admittedly very old although running 15.8) iphone due to it not supporting window.screen.orientation 🙄 . I pushed a fix for that that falls back to the deprecated window.orientation.

Is the DeviceOrientationEvent coming through on https://sensor-js.xyz/demo.html on your redmi phone? I'm wondering if maybe you weren't seeing it using your code because it's not running over https. I've run it through an https tunnel and it seems to be working on my pixel3a, pixel6a, and samsung tablet as well as the iphone now.

I think in the case where the sensor data isn't available I would prefer an outright error to be thrown rather than falling back to the window orientation and for the error to be documented as well as handled in the OrientationReader (it could do the falling-back to a non-sensor orientation there but it would still allow people who explicitly need the sensor orientation to be notified that it isn't possible using the more direct API).

Also, if it's just the case of the https with your phone, it would probably be worth debugPrinting a message stating that the sensor orientation cannot be read on a non-https page (and maybe supply a link to use a cloudflare tunnel or ngrok) as I'm sure someone will immediately run into that.

That's all my time for today but I'll try to take a look at this again in the next couple days; if you're able to confirm what's going on with your phone and possibly do those changes to the fallback + debugprint then I'll get it all committed and if not I'll try to do the fallback changes then.

@mwoelk
Copy link
Author

mwoelk commented Apr 21, 2024

Hey @rmtmckenzie,

Sadly on my redmi, the DeviceOrientationEvent is available in chrome but when listening to it it gets fired only once with alpha, beta and gamma being null. Following the specs a device should null the values it can't provide. Same result for me on https://sensor-js.xyz/demo.html. And I ran everything over https (built for web in release and debug and just uploaded to my server). On my iPhone everything works fine and even using the Sensor in the Chrome DevTools on desktop worked well.

Funny. I initially had put in some debugPrints but wasn't sure how you would handle logging/debugging and removed it afterwards. I guess a way to provide feedback (e.g. sensor not available / permission on iOS not given / non-https context would be great). Regarding the fallback I guess it makes sense to wrap them in custom errors (could be re-used across platforms e.g. SensorNotAvailableException, SensorPermissionNotGrantedException, etc.) and throw these into the stream to be handled in the reader.

Regarding the reader I would add a default-false boolean property fallbackOnError that would in case of an error automatically fall back to a non-sensor stream.

For those interested in handling errors themselves, I see three options:

  1. Pass the error down in an optional error property of _InheritedNativeDeviceOrientation. If someone calls NativeDeviceOrientation.orientation we could throw the error to the user.
  2. Add an onError callback to the reader that gets called whenever an error occurs and let the user handle it upwards.
  3. Add an errorBuilder that the user can use to build his own fallback widget.

What do you think? In my opinion it could be beneficial to have all three :)

@rmtmckenzie
Copy link
Owner

rmtmckenzie commented May 4, 2024

I've been busy moving the last while, sorry about that. That's annoying re: the redmi but I guess it is what it is. Strange that it can't provide the event, I'm quite sure that the device itself would have an accelerometer but I guess if the driver doesn't expose the standard API for chrome to consume that could explain it.

I think for debugPrints, the main thing I'd avoid is anything that prints on a per-event basis as people get (understandably) upset about that. But if it's a one-off thing on start/finish of a stream, I see nothing wrong with giving the developer a bit of context.

I think for the reader the fallbackOnError option is a good idea although could you call it fallbackOnSensorError to be a bit more precise. Another option would be to make the sensor parameter an enum with options something like (no sensor, sensor with fallback, sensor only), but introducing a breaking change just for that seems unnecessary at this point as it's mostly going to be for the web. Maybe I'll refactor that eventually if I have other breaking changes...

I would actually recommend making fallbackOnSensorError default true. The purpose of the reader is for it to "just work" in as many cases as possible, and hopefully what you're seeing with your redmi is the exception not the rule. Plus the reader should stay as simple as possible, so maybe your #1 option is sufficient. I think that anyone who really needs something more complicated than for error handling could simply use the communicator directly.

Hmm. Actually, now that sealed classes exist, that might be a good way to do it too although it would entail a breaking change. But it would make the API better as it would be possible to have this:

sealed class NativeDeviceOrientation {
  const NativeDeviceOrientation();
  
  DeviceOrientation? get deviceOrientation;
}

class LandscapeLeft extends NativeDeviceOrientation {
  const LandscapeLeft();
  
  @override
  final deviceOrientation = DeviceOrientation.landscapeLeft;
}

class LandscapeRight extends NativeDeviceOrientation {
  const LandscapeRight();
  
  @override
  final deviceOrientation = DeviceOrientation.landscapeRight;
}

class PortraitUp extends NativeDeviceOrientation {
  const PortraitUp();
  
  @override
  final deviceOrientation = DeviceOrientation.portraitUp;
}

class PortraitDown extends NativeDeviceOrientation {
  const PortraitDown();
  
  @override
  final deviceOrientation = DeviceOrientation.portraitDown;
}

class UnknownOrientation extends NativeDeviceOrientation {
  @override
  final deviceOrientation = null;

  final Error error;
  
  const UnknownOrientation(this.error);
}


...

void handleOrientation(NativeDeviceOrientation orientation) {
  switch(orientation) {      
    case LandscapeLeft():
      // do whatever
      break;
    case LandscapeRight():
      break;
    case PortraitUp():
      break;
    case PortraitDown():
      break; 
    case UnknownOrientation():
      orientation.error
  }
}

What do you think about that? It'd be a cleaner way to handle errors on the native side too tbh. I'd be willing to a version bump & API change if we did that as well as the sensor-parameter enum change.

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