Skip to content

Conversation

@gspencergoog
Copy link
Contributor

@gspencergoog gspencergoog commented Feb 27, 2020

Description

This PR adds the linux and windows target platform enum values, along with automatically setting the defaultTargetPlatform to the appropriate value on those platforms.

Related Issues

Tests

  • Modified tests to take new platforms into account.

Breaking Change

  • Yes, this is a breaking change, since any switch statements that handle the platforms will now give lint warnings that not all enum values are handled. Here's the website PR with the migration guide.

@fluttergithubbot fluttergithubbot added f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Feb 27, 2020
@gspencergoog gspencergoog force-pushed the target_platform branch 2 times, most recently from 60ec054 to 11bdcf0 Compare February 27, 2020 02:00
@fluttergithubbot fluttergithubbot added d: examples Sample code and demos c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. labels Feb 27, 2020
@gspencergoog gspencergoog changed the title [WIP] Add Linux and Windows target platforms Add Linux and Windows target platforms Feb 27, 2020
@flutter flutter deleted a comment from fluttergithubbot Feb 27, 2020
@gspencergoog
Copy link
Contributor Author

/cc @stuartmorgan

Copy link
Contributor

@stuartmorgan-g stuartmorgan-g left a comment

Choose a reason for hiding this comment

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

Nice!

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this actually what we want, given that Roboto isn't on Windows or Linux by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I thought about that, but it's the default font for Material, and we're doing Material on these platforms. I think it might look wrong to, say, use "Arial" on Windows and, "San Francisco" on macOS. We can try it, though, if that makes it a smaller download. Don't we package all the fonts in use?

Copy link
Contributor

Choose a reason for hiding this comment

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

it's the default font for Material

Is it though?

"Roboto is the default system font for Android. For platforms outside of Android and web products, use a system typeface that is preferred on that platform. For example, iOS applications should use Apple’s San Francisco font."
https://material.io/design/typography/understanding-typography.html#system-fonts

Don't we package all the fonts in use?

It's possible to package fonts in an application, but it's not done by default that I'm aware of. Doing it yourself via pubspec doesn't have a per-platform specification that I'm aware of, so I don't think it's possible to have an app that packages Roboto for Windows and Linux without bloating everything else (which would be really bad for iOS and Android), although maybe I'm wrong about that.

Also, if we wanted to auto-package it, then we would need to add tool support to auto-download it, since we can't package something that's not on the machine to start with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, I was mistaken about that then. I'll switch it to default to "Segoe UI" on Windows. On Linux, it's not so clear: Ubuntu's default font is "Ubuntu", so I'm guessing that it's not available on Embedded Gentoo. :-)

Copy link
Contributor Author

@gspencergoog gspencergoog Feb 27, 2020

Choose a reason for hiding this comment

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

We should also probably find out if you can specify fonts per platform in a pubspec: someone's definitely going to want that.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should talk to folks about how to handle Linux. We may need a way to specify a list of a few options depending on what's present, or we may need to pick one that we think is most likely and then rely on font fallback (which is currently awful on Linux per recent discussions, but needs to be fixed anyway).

Copy link
Contributor Author

@gspencergoog gspencergoog Feb 27, 2020

Choose a reason for hiding this comment

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

OK, I added whiteRedmond and blackRedmond typography sets based on "Segoe UI". I left the linux as "Roboto", for lack of a better default for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I made linux use Roboto, but added fallbacks of DejaVu Sans, Liberation Sans, and finally Arial.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm still not sure about Roboto as the first choice, but we can revisit that later; this is clearly better than nothing. Speaking of revisiting though, will we consider changing this list later to be a breaking change?

Copy link
Contributor Author

@gspencergoog gspencergoog Feb 27, 2020

Choose a reason for hiding this comment

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

That's a tough one. One one hand, rendering run on the same machine might change, on the other, the presence of a particular font is purely a stochastic sample.

I'd lean towards "yes", simply because it might change goldens for some testers.

@HansMuller
Copy link
Contributor

This isn't a breaking change?

@gspencergoog
Copy link
Contributor Author

Hmm. I guess it could cause new warnings about unhandled switch cases (if that lint is turned on). OK, I'll go with "breaking change".

@gspencergoog gspencergoog merged commit 1ba4f1f into flutter:master Mar 3, 2020
@gspencergoog gspencergoog deleted the target_platform branch March 3, 2020 12:38
gspencergoog added a commit that referenced this pull request Mar 6, 2020
Remove a target platform override that is no longer needed (I missed this in #51519).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

a: desktop Running on desktop c: contributor-productivity Team-specific productivity, code health, technical debt. customer: gallery Relating to flutter/gallery repository. Please transfer non-framework issues there. d: examples Sample code and demos f: cupertino flutter/packages/flutter/cupertino repository f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Windows, and Linux as TargetPlatforms

5 participants