Closed onewilk closed 2 years ago
Hi @onewilk, thanks for the fix PR, much appreciated.
Can you provide an updated version that passes the above failed required dartfmt test. For silly pub.dev scoring purposes the package requires dartfmt to be used with max 80 char line length. Which sadly is a requirement for max score in pub.dev when it scores packages. Also the same the same max line length and dartfmt length makes it easier to see the actual changes in the diff.
@onewilk the fix also seem to be a fix that works around the issue the original issue poster has, but does not really address why the issue exists to begin with. Let's analyze that a bit:
The reported error is about a nullable variable assignment to a non-nullable variable, however the variable the error message claims is none nullable, the iconSize
in IconButton
, is in fact nullable in the null-safe version of the Flutter framework. As shown here https://github.com/rydmike/flex_color_picker/issues/40#issuecomment-1050166241
And seen in the framework code here:
This fix works around the issue by, creating a non nullable intermediate, that is then assigned to the nullable iconSize
in Iconbutton
, where the OP's error message for some reason has an error that claims that iconSize
is not nullable.
As such this is doable, but the confusing part is that it should not be necessary. Do you have any insights into why the original poster might be seeing this error? I cannot replicate the error in my environment even if I try. I would like to understand what is causing the root issue for the original poster.
The fix does contain a nice little bonus and that is that accessing the fixedIconSize
variable is perhaps a bit faster than doing it via the effectiveIconTheme
and since it used a couple of times, yeah why not.
This is just semantics, but if you do fix the lines that show up as changed and causes the merge check to fail since their formatting is not in line with dartfmt, then please also rename fixedIconSize
to effectiveIconsSize
, since it better represents what it is, it the effective size result for the icon, from either the passed in icon theme or the default 22.
About dartfmt, the style you used is much nicer, with the a bit longer lines, I would write it that way myself too, but the package analyzer on pub.dev gives a package score penalties if I use that format. That is why I setup strict check for that default dartfmt output format is being used.
I can of course merge this as is, and after that do the edit to fix (I just save and it is fixed) the lines that causes the dartfmt check to fail that would also fail in my CI/CD build test, and rename fixedIconSize
, minor details really.
The fix as such will probably solve the issue the original poster is seeing, but that issue should as far as I can tell from the brief reported error message not even exist.
In any case, I will check up on all this tomorrow and do the merge and edit myself if I don't hear back from you.
It's a sensible work around so thanks for that, but does not explain why it is needed. It bothers me since the issue should not exist, so no fix should be needed. It is a bit weird to be honest, unless there is something I'm missing, or the original posters error message is wrong somehow.
Anyway thanks for your help :)
The hint in the 2nd issue report that there was an issue in Flutter 2.8.1, but not in 2.10.x helpes. Interestingly iconSize
is nullable in 2.10.x but not in e.g. 2.8.1. Weird, tried to track down how long that has been the case, since it should have been an issue long before for me too.
In any case, I merged the fix, made the minor dartfmt update and mentioned variable rename, minor details really. Thank you for your help and contribution @onewilk 💙
40 The argument type 'double?' can't be assigned to the parameter type 'double' because 'double?' is nullable and 'double' isn't. iconSize: effectiveIconTheme.size