Make UnderlineInputBorder consistent
#124153
Conversation
flutter-dashboard
bot
added
f: material design
bernaferrari
force-pushed
the
UnderlineInputBorder
branch
3 times, most recently
from
86a0f29 to
3fe5717
Compare
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
@bernaferrari Weren't you and I discussing this on another issue somewhere? Could you link that if you still have the link? And post the code to produce these fields.
Also, can you clarify what the difference is between "Currently" and "Proposal" in your image? Should the top one say BorderRadius.zero?
I couldn't get the Google tests to show up here but I will run them locally.
There was a problem hiding this comment.
Because there are two values, so the other possible way would be
Radius(clampDouble(min: 0, rect.tlRadiusX + insets.left), clampDouble(min: 0, rect.tlRadiusY + inset.top))
Do you prefer that way? I can make it.
There was a problem hiding this comment.
Oh, good news: I copied the code from what I'm doing on BoxDecoration. When my other PR gets merged, this will be redundant and I'll be able to use the BoxDecoration.paintNonUniformBorder.
There was a problem hiding this comment.
Which one do you prefer? If you prefer the top one, I can later modify in the BoxDecoration:
There was a problem hiding this comment.
I guess the clampDouble way doesn't look much worse, I say use that. The linter rule is there because .clamp is much slower, see #103917.
There was a problem hiding this comment.
But clamp from Radius uses clampDouble
|
I don't see any Google test failures. |
|
Ops, I was writing the comment and pressed the wrong button. I don't think so, I commented in the Framework channel from Discord. Hixie preferred this, a few others preferred the old one. There was no consensus. The difference is that the old one clips, so for example, if you have a super large radius, the line would be really short, it only shows the "rectangular" part. The proposal is to follow the curve, embrace the radius. It is faster, better, more consistent with other platforms and non breaking. But kind of big. The screenshot shows the code diff, from path to drrect:
(if you accept, please don't autosubmit, I have another PR on the way that will allow to simplify this further, but the result will be the same, just less LOC). |
justinmc
left a comment
justinmc
left a comment
There was a problem hiding this comment.
LGTM 👍
Thanks for the Discord link, that's what I was thinking of. I'm on board with "Proposal" approach.
Leaving this with no autosubmit for now as requested.
There was a problem hiding this comment.
Nit: Could this be static?
There was a problem hiding this comment.
Yes (but why?).
But this is going to probably be replaced by BoxBorder.paintNonUniformPath. I just need to get that approved. Shouldn't take more than a few days.
There was a problem hiding this comment.
It is funny I can make the BoxBorder changes in this commit or in the other one, lol.. But the other one seems to be in "almost approval" phase, so let's see.
bernaferrari
force-pushed
the
UnderlineInputBorder
branch
from
3fe5717 to
0ed7dc2
Compare
|
Spoke with @bernaferrari, he says this is blocked on #124417 |
|
Yes, thanks! I forgot to write this here lol |
bernaferrari
force-pushed
the
UnderlineInputBorder
branch
4 times, most recently
from
97533b2 to
cc93222
Compare
There was a problem hiding this comment.
@justinmc Ok, heeeere we go! Not sure I need another review (probably not), just double checking. I implemented what I wanted to, and the only slightly "odd" thing is this.
Reason: If topLeft/topRight borders are there, the paint causes weird anti-alias artifacts to show, so making them square, and limiting the bottomLeft/Right to h/2 prevents the issue from ever appearing.
Consequences: theoretically an elliptical border(999, 0) wouldn't be possible anymore (clamp to h/2), but a) it is not breaking, b) it is already rare in standard widgets, c) this would be even more rare. I think it is ok.
Screenshot of what I'm trying to avoid:
|
@bernaferrari Ok let's try landing it! |
bernaferrari
force-pushed
the
UnderlineInputBorder
branch
3 times, most recently
from
d6c9e19 to
b026914
Compare
bernaferrari
force-pushed
the
UnderlineInputBorder
branch
from
b026914 to
efd8c75
Compare
bernaferrari
added
the
autosubmit
Roll Flutter from e8c2bb1 to 53a57ad (39 revisions) flutter/flutter@e8c2bb1...53a57ad 2023-11-16 [email protected] Roll Flutter Engine from 8ab189b77b8d to 2e9f0df868b3 (1 revision) (flutter/flutter#138543) 2023-11-16 [email protected] Roll Flutter Engine from 622fa0614412 to 8ab189b77b8d (1 revision) (flutter/flutter#138533) 2023-11-16 [email protected] [flutter_tools] - Add `queries` section to Android manifest file (flutter/flutter#137207) 2023-11-16 [email protected] Roll Flutter Engine from 8aff9c134b8f to 622fa0614412 (1 revision) (flutter/flutter#138532) 2023-11-16 [email protected] Roll Flutter Engine from 3cfcdebe8623 to 8aff9c134b8f (18 revisions) (flutter/flutter#138529) 2023-11-16 [email protected] Roll Flutter Engine from 30327eae0802 to 3cfcdebe8623 (1 revision) (flutter/flutter#138515) 2023-11-15 [email protected] Catch error for missing directory in `FontConfigManager` (flutter/flutter#138496) 2023-11-15 [email protected] Make `UnderlineInputBorder` consistent (flutter/flutter#124153) 2023-11-15 [email protected] Prepare `ShortcutActivator` and `ShortcutManager` to migrate to `KeyEvent` from `RawKeyEvent`. (flutter/flutter#136854) 2023-11-15 [email protected] Pin package:web 0.4.0 (flutter/flutter#138428) 2023-11-15 98614782+auto-submit[bot]@users.noreply.github.com Reverts "Reland VelocityTracker update (#132291)" (flutter/flutter#138512) 2023-11-15 [email protected] [web] skip flaky overflow_clipbehavior_none.cupertino.0.png golden check (flutter/flutter#138498) 2023-11-15 [email protected] Roll Flutter Engine from 7c2b8d520b3d to 30327eae0802 (2 revisions) (flutter/flutter#138502) 2023-11-15 [email protected] Reland VelocityTracker update (#132291) (flutter/flutter#137381) 2023-11-15 [email protected] Roll Flutter Engine from f58dac64ad1a to 7c2b8d520b3d (1 revision) (flutter/flutter#138499) 2023-11-15 [email protected] Fix 2D tap to stop scrolling (flutter/flutter#138442) 2023-11-15 [email protected] Roll Flutter Engine from d22d063ac9f6 to f58dac64ad1a (2 revisions) (flutter/flutter#138494) 2023-11-15 [email protected] SemanticOwner should dispatch creation and disposal events (flutter/flutter#138388) 2023-11-15 [email protected] Roll Flutter Engine from ecaf9442034d to d22d063ac9f6 (5 revisions) (flutter/flutter#138489) 2023-11-15 [email protected] Roll Packages from 428ba3e to 0cd2378 (1 revision) (flutter/flutter#138482) 2023-11-15 [email protected] Marks Mac_android hot_mode_dev_cycle__benchmark to be unflaky (flutter/flutter#138472) 2023-11-15 [email protected] Roll Flutter Engine from a7a48a68e6f8 to ecaf9442034d (1 revision) (flutter/flutter#138468) 2023-11-15 [email protected] Roll Flutter Engine from 976edd5192d1 to a7a48a68e6f8 (3 revisions) (flutter/flutter#138463) 2023-11-15 [email protected] Roll Flutter Engine from a7f2267dd1f4 to 976edd5192d1 (1 revision) (flutter/flutter#138462) 2023-11-15 [email protected] Roll Flutter Engine from bc5bbd3b9ebe to a7f2267dd1f4 (1 revision) (flutter/flutter#138459) 2023-11-15 [email protected] Roll Flutter Engine from d7ca057b891f to bc5bbd3b9ebe (2 revisions) (flutter/flutter#138457) 2023-11-15 [email protected] Roll Flutter Engine from 1347413470b7 to d7ca057b891f (1 revision) (flutter/flutter#138456) 2023-11-15 [email protected] Roll Flutter Engine from c5a067b637f4 to 1347413470b7 (5 revisions) (flutter/flutter#138452) 2023-11-15 [email protected] Reland [SingleChildScrollView] Correct the offset pixels if it is out of range during layout (flutter/flutter#136871) 2023-11-14 [email protected] Add to TableCell docs (flutter/flutter#138258) 2023-11-14 [email protected] Roll Flutter Engine from f15b259fe98c to c5a067b637f4 (4 revisions) (flutter/flutter#138441) 2023-11-14 49699333+dependabot[bot]@users.noreply.github.com Bump dessant/lock-threads from 4.0.1 to 5.0.0 (flutter/flutter#138437) 2023-11-14 49699333+dependabot[bot]@users.noreply.github.com Bump github/codeql-action from 2.22.5 to 2.22.6 (flutter/flutter#138438) 2023-11-14 [email protected] Roll Flutter Engine from eba757803a6f to f15b259fe98c (1 revision) (flutter/flutter#138429) 2023-11-14 [email protected] Roll Flutter Engine from 603bdd48df8a to eba757803a6f (3 revisions) (flutter/flutter#138425) 2023-11-14 [email protected] Unified analytics migration for `CodeSizeAnalysis` (flutter/flutter#138351) 2023-11-14 [email protected] Roll Flutter Engine from 777dcb99f6e0 to 603bdd48df8a (1 revision) (flutter/flutter#138421) 2023-11-14 [email protected] Run all tests in examples/ (flutter/flutter#138374) 2023-11-14 [email protected] Roll Flutter Engine from 1b3fd80812c3 to 777dcb99f6e0 (2 revisions) (flutter/flutter#138420) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. ...
3 participants
This was easy to implement. I like the result, I think
borderRadius.zero->borderRadius.circularmakes a nice transition, and many places (like macOS) use an effect similar to this PR, while Google doesn't use anywhere (yet). I'm curious if it is going to break goldens or google testing.What do you think? cc @HansMuller @gspencergoog. Is this something you want, should I ask the community, or do you prefer the current one?
Side effects:
UnderlineInputBorder(TODO: fixdrawLinewhen borderRadius is zero).BoxBorder._paintNonUniformBorder(if it weren't private). Single LOC implementation.