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

Persistable "load" will always pass as Successful? #48

Closed
Kithio opened this Issue Feb 7, 2018 · 8 comments

Comments

Projects
None yet
3 participants
@Kithio
Copy link
Contributor

Kithio commented Feb 7, 2018

It seems that as long as you have LocalFiles enabled, the "load" function will never give back "success = false" to it's callback.

Personally I find it confusing to have it this way because trying to load invalid data would still cause it to give a "successful" response.... Why success is being passed to the "onLoaded" event when it's never going to be false as long as the player has LocalFiles enabled is beyond me... Especially since the example shown using Persistable to save/load data uses network to check if local files are enabled BEFORE calling the load function to begin with.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Feb 7, 2018

Yea, it should probably pass success and deserializeSuccess.
Looks like a regression from adding LocalFile handling.
I will wait for @SamuelMoriarty to respond and then apply a fix as needed.
Thanks for the report.

@Kithio

This comment has been minimized.

Copy link
Contributor

Kithio commented Feb 7, 2018

I just found it very strange since it means there's no way to actually find out if they tried to load a valid file or not :P

@SamuelMoriarty

This comment has been minimized.

Copy link
Contributor

SamuelMoriarty commented Feb 7, 2018

Yes, it would make sense for them both to be passed.
They should also probably be renamed fileParseSuccess and deserializeSuccess.
There are three stages where loading can potentially fail:

  1. Local User can't read files because LocalFiles aren't enabled,
  2. Local User couldn't decode the data in the file due to corruption, i.e. StringBuffer failed
  3. The data is decoded correctly, but there is a logical error in the data itself, i.e. deserialize method failed

success corresponds to 1 and 2, deserializeSuccess corresponds to 3.
In either scenario, the persistable class supports supplying default values to the class in case of an error.

@Kithio

This comment has been minimized.

Copy link
Contributor

Kithio commented Feb 7, 2018

I would like a way to actually know if the file that attempted being loaded actually existed in the first place so I can report an error to invalid file or invalid data, if that is even possible.

But fixing the "success" thing would give me more of an idea about where things went wrong.

@SamuelMoriarty

This comment has been minimized.

Copy link
Contributor

SamuelMoriarty commented Feb 7, 2018

It's definitely a good idea, and it gave me a huge headache back when I was trying to get the initial version out. Admittedly I was somewhat in a hurry, so there's still lots of room for improvement.

The thing is, though, there's no easy way to test if the user doesn't have the file without actually trying to read it first, which is an expensive operation. Either way, whether the user has no data or it is corrupted - the outcome is one and the same, you have to ditch the old data (if any) and rewrite it. Hence the .supplyDefault() method being called in both scenarios.

But there definitely needs to be better error handling.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Feb 7, 2018

I would suggest replacing the boolean with an enum that contains the possible failure cases, e.g.

enum LoadStatus
    FAIL_NOT_ENABLED
    FAIL_FILE_CORRUPT
    FAIL_DECODE
    SUCCESS
@SamuelMoriarty

This comment has been minimized.

Copy link
Contributor

SamuelMoriarty commented Mar 3, 2018

#49 Fixes this.

@Frotty

This comment has been minimized.

Copy link
Member

Frotty commented Mar 4, 2018

fixed by #49

@Frotty Frotty closed this Mar 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment