Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix effectiveIconTheme.size null safe error #41

Merged
merged 1 commit into from
Mar 3, 2022

Conversation

onewilk
Copy link

@onewilk onewilk commented Feb 25, 2022

#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

@rydmike
Copy link
Owner

rydmike commented Feb 25, 2022

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.

@rydmike
Copy link
Owner

rydmike commented Feb 26, 2022

@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 #40 (comment)

And seen in the framework code here:

https://github.com/flutter/flutter/blob/35df3aa43969eff5ab79ba1f4d8c81db8475dbe5/packages/flutter/lib/src/material/icon_button.dart#L156

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 :)

@rydmike
Copy link
Owner

rydmike commented Mar 3, 2022

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 💙

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.

None yet

2 participants