Skip to content

Migration guide for deprecating textScaleFactor in favor of TextScaler - #8903

Merged
sfshaza2 merged 16 commits into
flutter:mainfrom
LongCatIsLooong:android-14-nonlinear-text-scaling
Jul 24, 2023
Merged

sfshaza2 merged 16 commits into
flutter:mainfrom
LongCatIsLooong:android-14-nonlinear-text-scaling

Conversation

@LongCatIsLooong

Copy link
Copy Markdown
Contributor

Corresponds to the framework PR: flutter/flutter#128522

Presubmit checklist

@sfshaza2 sfshaza2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I know this is still in draft, but I have feedback anyway. :)

Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
@sfshaza2

sfshaza2 commented Jun 15, 2023 •

Copy link
Copy Markdown
Contributor

Oh, this PR is currently breaking the build because these two links are missing:

[Android 14 nonlinear font scaling][]
[Ensure Android 14 compatibility][]

Finally, don't forget to add this page to the index! :D

@LongCatIsLooong
LongCatIsLooong force-pushed the android-14-nonlinear-text-scaling branch from d85b467 to 1b86a28 Compare June 22, 2023 19:47
@LongCatIsLooong
LongCatIsLooong force-pushed the android-14-nonlinear-text-scaling branch from 1b86a28 to 46d6ce3 Compare June 22, 2023 19:51
@LongCatIsLooong
LongCatIsLooong marked this pull request as ready for review June 22, 2023 21:20
@LongCatIsLooong

Copy link
Copy Markdown
Contributor Author

I think this is ready for review now that the framework PR is LGTM'd. One thing that I might not have conveyed clearly in this migration guide is that the framework PR in fact doesn't add Android 14 non-linear text scaling support (it will be added in a follow up PR). So if people want to test their apps against Android 14 nonlinear text scaling then they would have to wait for a different PR to land. Do you have any suggestions on how I should add that information to the migration guide?

Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
| `SelectableText.rich({ double? TextScaleFactor = 1.0 })` constructor argument | 'textScaleFactor' is deprecated and shouldn't be used. |
| `SelectableText.textScaleFactor` getter | 'textScaleFactor' is deprecated and shouldn't be used. |

## Migration guide

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

since non-linear text scaling isn't actually implemented I would focus the guide just on the change from textScaleFactor to textScaler and what app developers need to do to adapt to that. (In the context section it is good to mention the non-linear scaling as motivation for the change, but we shouldn't leave the impression that that's already implemented and shouldn't ask people to test it)

consider replacing `textScaleFactor` with `TextScaler.textScaleFactor` first. The
`TextScaler.textScaleFactor` getter was added for backward compatibility during
the migration process and is expected to have a longer lifetime than other
`textScaleFactor` deprecations.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we should promise a longer deprecation period. The regular policy applies here (for now).

Comment on lines +146 to +147
// For now, implementers can still access the `textScaleFactor`.
// scalar via textScaler.textScaleFactor.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Let's focus the guide on what the right way to migrate is, e.g. how should textScaler be used correctly?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ideally, this guide shouldn't mention textScaler.textScaleFactor at all since that is not the right way to migrate your code long-term. If necessary, we can at the very end mention it for completeness, but it I wouldn't focus the guide this much around it. It's not a long-term solution and asking people to migrate to something that is already deprecated is somewhat frustrating.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should I add a section for apps that don't target Android 14 and tell them to add an extension?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is that realistic for an app to not target Android 14? That'll have to happen sooner or later, no? I don't think I would spend too much time on how to do the thing we don't want developers to do...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

that's true for desktop-only apps or web-only apps I think?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lol, ok. The long-term goal is that TextScaler is the only text scaling mechanism in Flutter, though. And I don't think we should (pretend to) support anything else. The migration guide should show people how to migrate to TextScaler, which is the API we want people to use and the only one we will actually support in the future.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'll save that for the migration guide for nonlinear text scaling

@LongCatIsLooong LongCatIsLooong changed the title Migration guide for Android 14 nonlinear text scaling Migration guide for deprecating textScaleFactor in favor of TextScaler Jun 26, 2023

@sfshaza2 sfshaza2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have some minor updates but, overall, this looks good. @goderbauer, it seems like your issues are addressed? Is this lgtm for you?


## Timeline

Landed in version: xxx<br>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can't land this PR until the "Landed in" field is filled out.

@LongCatIsLooong LongCatIsLooong Jun 27, 2023 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to land the PR probably early next week.

Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
Co-authored-by: Shams Zakhour (ignore Sfshaza) <[email protected]>

@goderbauer goderbauer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM


The `scale` method should be used to scale font sizes in lieu of `textScaleFactor`.
The `textScaleFactor` getter provides an estimated `textScaleFactor` value, it
is for backward compatibility purposes and is already marked as deprecated.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add to be very explicit: It will be removed in a future version of Flutter.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought it was discouraged to use future tenses in migration guides? Not sure how best to phrase this @sfshaza2 any suggestions?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That use is fine, @LongCatIsLooong.

Comment thread src/release/breaking-changes/android-14-nonlinear-text-scaling.md Outdated
```


If the API that provides `textScaleFactor` hasn't been migrated, consider

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Basically move this before the "bad" migration advice above. (Or better: just remove that "bad" advice)

@sfshaza2

sfshaza2 commented Jul 5, 2023

Copy link
Copy Markdown
Contributor

OK! Just ping me.

@sfshaza2 sfshaza2 removed the review.copy Awaiting Copy Review label Jul 10, 2023
@atsansone atsansone added the act.await-dev-pr Needs dev PR to merge before merging docs label Jul 13, 2023
@LongCatIsLooong

Copy link
Copy Markdown
Contributor Author

Hi @sfshaza2 the PR is now merged. I added a section at the end of the migration guide to explain how to opt-out of non-linear scaling, since some customers wanted to QA the migration and slowly roll out.


## Timeline

Landed in version: xxx<br>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you fill in this field?

@sfshaza2 sfshaza2 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm!

@sfshaza2

Copy link
Copy Markdown
Contributor

The build failed. Do you use API that's not yet available? If so, can you use the master channel API link?

@sfshaza2 sfshaza2 removed the act.await-dev-pr Needs dev PR to merge before merging docs label Jul 19, 2023
@LongCatIsLooong

LongCatIsLooong commented Jul 19, 2023 •

Copy link
Copy Markdown
Contributor Author

Reran the test and it's passing now.

@sfshaza2
sfshaza2 merged commit 5ecc8a0 into flutter:main Jul 24, 2023
@LongCatIsLooong
LongCatIsLooong deleted the android-14-nonlinear-text-scaling branch July 24, 2023 17:28
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.

4 participants