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

Platform.isFuchsia #27103

Closed
abarth opened this issue Aug 18, 2016 · 8 comments
Closed

Platform.isFuchsia #27103

abarth opened this issue Aug 18, 2016 · 8 comments
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io type-enhancement A request for a change that isn't a bug

Comments

@abarth
Copy link
Contributor

abarth commented Aug 18, 2016

It would be convenient to have a Platform.isFuchsia getter along side Platform.isWindows and such.

@lrhn
Copy link
Member

lrhn commented Aug 18, 2016

I believe the main reason for having Platform.isWindows is to allow path computations to use the correct separator. Does Fuchsia need a non-unixy path?

@abarth
Copy link
Contributor Author

abarth commented Aug 18, 2016

We use Platform to provide various platform-specific UI adaptations. For example, scrolling physics is different on Android and iOS and will likely be different on Fuchsia again. We use the current Platform to determine which scrolling physics to use.

@lrhn
Copy link
Member

lrhn commented Aug 18, 2016

That makes sense (but would be better exposed as a scrollingPhysics property of the UI library).

I'm not particular happy about an open-ended list of getters for the platforms that we happen to know.
I'd rather have a single getter returning a string (which we do, it's Platform.operatingSystem) and then use if (Platform.operatingSystem == "fuchsia") ....

It's obvious that the platform is-getters have grown by budding (likely starting with Linux/Windows only), so let's nip this here and not add any more getters to the core libraries, and move towards a more general approach where the core library doesn't need to know about Fuchsia - only Fuchsia itself does when it implements the platform libraries.

If we can remove the old getters too, at a later point, that would be awesome!

@lrhn lrhn added area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io type-enhancement A request for a change that isn't a bug labels Aug 18, 2016
@abarth
Copy link
Contributor Author

abarth commented Aug 18, 2016

That makes sense (but would be better exposed as a scrollingPhysics property of the UI library).

That's what we do, but the UI library is built up in layers. The layer that's hard coded into the Flutter engine binary doesn't know anything about scrolling. That concept is introduced at higher levels in package:flutter. (Also, there are more platform adaptations than just scrolling---for example the title bar is centered on iOS and left-aligned on Android.)

I'd rather have a single getter returning a string (which we do, it's Platform.operatingSystem) and then use if (Platform.operatingSystem == "fuchsia") ....

Yes, that's what we're using today:

https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/foundation/platform.dart#L36

As written, this code sends the message that Fuchsia is less supported a platform than Android and iOS, which is contrary to the message we want to send.

so let's nip this here

This seems like an arbitrary point at which to draw this line. For example, if we'd had this discussion a month or so ago, would you have drawn the line at not including iOS?

Perhaps a more rational place to draw a line might be at whatever platforms are officially supposed. For example, we wouldn't have a Platform.isBeOS because Dart does not officially support BeOS.

@lrhn
Copy link
Member

lrhn commented Aug 19, 2016

This seems like an arbitrary point at which to draw this line. For example, if we'd had this discussion a month or so ago, would you have drawn the line at not including iOS?

Yes. It was arbitrary from the very beginning.

If I had been asked, I would have drawn the line at the start and not added any member at all. It's pure convenience methods, but they need to be maintained forever, and we have to explain why some platforms are there and others are not - and we don't want to actually add all platforms up front because it requires us to pick a string to represent the platform (and know about the platform).

Personally, I'd deprecate every is<Platform> getter and remove them in Dart 2.0.
We can have a package with convenience getters for those who want it, checking the operatingSystem string against any number of known OS strings instead of hardcoding a number of them into the platform libraries (why Fuchsia but not FreeBSD? OpenBSD?).

I have similar issues with the path library using isWindows to decide the path separator and a few other things - and it has leaked into the Uri class too because of file/URI conversions. It's always a bad idea, and we should fix it, not compound the problem.

@abarth
Copy link
Contributor Author

abarth commented Aug 19, 2016

why Fuchsia but not FreeBSD? OpenBSD?

Because https://github.com/dart-lang/sdk/blob/master/runtime/bin/platform_fuchsia.cc#L35 exists and no corresponding line of code exists for FreeBSD or OpenBSD.

@lrhn
Copy link
Member

lrhn commented Aug 22, 2016

I don't think we should design our libraries according to which platforms happen to be included in the SDK repository. As this issue shows, that would be a moving target. If another platform decides to have their patches in their own repository, like https://github.com/mulander/openbsd-dart, then the API should work just as well for them.

I still lean towards deprecating all the is getters and perhaps moving them to a package.

@floitschG
Copy link
Contributor

I'm less averse to adding a new getter, but there is a high bar for being added. As Lasse mentioned, there isn't even any isBSD.
I would love to add isFuchsia at some point in the future, but we won't add one now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-core-library SDK core library issues (core, async, ...); use area-vm or area-web for platform specific libraries. library-io type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

3 participants