[url_launcher] Fixes new unnecessary boolean operations warnings

[FMorschel]

I had opened #9361 and messed up when rebasing the changes. That made the old PR get closed.

CC @stuartmorgan-g for continuing the review.

Description from the old PR:

I'm fixing dart-lang/sdk#60614, which adds some other cases to no_literal_bool_comparisons, and these were triggered here.

I'm unsure of the intended behaviour of the packages\url_launcher\url_launcher\lib\src\legacy_api.dart part, but I can fix it if the result wasn't intended this way (the operators' priority may be less known here).

About the other cases with || true, I think these might be a pattern from the team, so maybe adding a file ignore could also fix this. Please say so that I can make this the right way for you.

Pre-Review Checklist

• I read the [Contributor Guide] and followed the process outlined there for submitting PRs.
• I read the [Tree Hygiene] page, which explains my responsibilities.
• I read and followed the [relevant style guides] and ran [the auto-formatter].
• I signed the [CLA].
• The title of the PR starts with the name of the package surrounded by square brackets, e.g. [shared_preferences]
• I [linked to at least one issue that this PR fixes] in the description above.
• I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy], or I have commented below to indicate which [version change exemption] this PR falls under¹.
• I updated CHANGELOG.md to add a description of the change, [following repository CHANGELOG style], or I have commented below to indicate which [CHANGELOG exemption] this PR falls under¹.
• I updated/added any relevant documentation (doc comments with ///).
• I added new tests to check the change I am making, or I have commented below to indicate which [test exemption] this PR falls under¹.
• All existing and new tests are passing.

Footnotes

1. Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩² ↩³

[FMorschel]

Continuing from my last message:

Sorry for the long delay. I came back to this today. I (fortunately) don't think that any cases are broken.

There would only be a problem if either the forceWebView or forceSafariVC default was true.

When adding the change you suggested on the last comment, no test broke, and I could not think of a test that would break before/after this change. If you can, please say so that I can add it.

[stuartmorgan-g]

I (fortunately) don't think that any cases are broken.

There would only be a problem if either the forceWebView or forceSafariVC default was true.

Why would a problem only happen with defaults? Clients can pass non-default values.

When adding the change you suggested on the last comment, no test broke

I'm aware that your PR didn't break any tests; that's why I said we needed to add a test. The lack of test breakage with the previous version indicates missing test coverage.

and I could not think of a test that would break before/after this change. If you can, please say so that I can add it.

As I said in the previous review:

forceSafariVC = false and forceWebView = true

A test asserting that this call throws should pass:

launch('nonwebscheme://', forceSafariVC: false, forceWebView: true);

[FMorschel]

Alright, I see where I made the mistake in my consideration. Sorry, I'll add the test. Thanks!

[stuartmorgan-g]

Since it would only affect the last two cases, it would make them the middle cases and the outputs are exactly the same.

That is not correct.

Have you actually run the test I suggested before and after the PR?

[FMorschel]

Have you actually run the test I suggested before and after the PR?

Now I have. I'm really sorry I got confused on my last comment.

I'm adding the new test now. Thanks for your patience!

[stuartmorgan-g]

url_launcher needs a version bump, per the CI failure, since there are changes to production code.

[FMorschel]

I've bumped the version already @stuartmorgan-g.

I've also added url_launcher to the PR name since this is the only package that had any significant changes.

Is the version bump I did enough? Am I missing anything still? Thanks!

[stuartmorgan-g]

Am I missing anything still?

Yes; all of the failures flagged by CI need to be resolved.

[FMorschel]

I've added the version number on the Changelog, and a new entry about the new case that will throw.

1. One of the tests timed out.
2. I need an exemption for in_app_purchase. The only change there was adding the ignore comment.

[stuartmorgan-g]

LGTM

[stuartmorgan-g]

Version/CHANGELOG override: Although the change is to example/lib/main.dart, and thus usually requires a version change since it's published on pub.dev, the // ignore isn't important for the use case of using the code from pub.dev since it's analysis-option specific.

[auto-submit]

autosubmit label was removed for flutter/packages/9409, because This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.

• Merge guidelines: A PR needs at least one approved review if the author is already part of flutter-hackers or two member reviews if the author is not a flutter-hacker before re-applying the autosubmit label. Reviewers: If you left a comment approving, please use the "approve" review action instead.

[FMorschel]

Hey @Hixie and @bparrishMines can I get some reviews here please? The changes are really minimal. Simply adding an ignore comment.

Thanks a lot!

[stuartmorgan-g]

@bparrishMines for secondary review; please add autosubmit once it's approved.

[bparrishMines]

LGTM