Don't panic when loading preferences with wrong type

[nh3000-org]

Description:

Fix panics in case of type change or file corruption

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[Jacalz]

Thanks for the PR. I left a few comments on improving the variable naming. Most of the comments do apply in more places than where I added them (a lot of similar code in various functions).

One question for the other reviewers: Should we really keep the casting between float and int? At least for slices, this does seem like a very slow operation.

[Jacalz]

I forgot to add, would you mind giving the PR a more description name?

[andydotxyz]

Thanks for the PR. I left a few comments on improving the variable naming. Most of the comments do apply in more places than where I added them (a lot of similar code in various functions).

One question for the other reviewers: Should we really keep the casting between float and int? At least for slices, this does seem like a very slow operation.

The float/int casting is required because Go loads JSON ints as floats...

[Jacalz]

The float/int casting is required because Go loads JSON ints as floats...

Sure, but if all number lists are stored as slices of floats then surely all of our IntList() methods that we have would not have worked before this PR?

[andydotxyz]

Ah yes maybe some of it could be avoided. As long as it doesn't introduce type issues between int/float in load/save happy to remove what we can.

[coveralls]

coverage: 66.2% (+0.05%) from 66.149% when pulling ee5c3bc on nh3000-org:develop into e050d79 on fyne-io:develop.

[andydotxyz]

This looks good to me, I think we need to resolve the casting discussion with @Jacalz

[andydotxyz]

Sure, but if all number lists are stored as slices of floats then surely all of our IntList() methods that we have would not have worked before this PR?

It is possible we missed a test case in the new number list code?...

[Jacalz]

I did a quick test on develop and int lists do work fine on develop without any casts from float in the slices we return. Do we really want to do the costly translation then? I'd be with it just returning the fallback if I try to load a FloatList() when I have set an IntList() on that key. It seems better to just make the developer understand that they used the wrong ket rather than allocating a new slice and copying over all the values.

FYI: The code we have to casting over slices if incorrect for float in this PR. If we decide on keeping the casting, we never want to cast float to int as any possible decimal precision is rounded down.

[nh3000-org]

FYI: The code we have to casting over slices if incorrect for float in this PR. If we decide on keeping the casting, we never want to cast float to int as any possible decimal precision is rounded down.

Agree with rounding, but is fallback better than return 1 rather than 1.0

I can change it. Let me know....

[andydotxyz]

I think the rounding is a red-herring. Ints can be deserialised as floats, but they will have ".0" after the decimal place because they were ints in the first place... so if int cast is here to cope with JSON storage then there is no rounding to worry about?

[nh3000-org]

I have tested on linux, android and tests run as expected.

[Jacalz]

Thanks. This is looking good. Sorry for the confusion with the casting. What you have now makes sense.

I've left two comments about removing some newlines that I think you might have accidentally added in. Apart from that, I think we need a test for the []int to []float slice cast because that wasn't there before. This should be good to go after that :)

[nh3000-org]

Tested on linux and android.

[andydotxyz]

Excellent to have more safety thanks :)

[Jacalz]

Thanks