-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Snake Plugin: Store game state on close and restore it on restart, show highscore #1922
Conversation
@DrZlo13 is currently formalizing rules for ext apps storage layout. He'll be reviewer for this PR |
@JuanJakobo Hello! |
@DrZlo13 Hi, |
@DrZlo13 please review. |
} else { | ||
if(storage_common_stat(storage, APPS_DATA, NULL) == FSE_NOT_EXIST) { | ||
if(!storage_simply_mkdir(storage, APPS_DATA)) { | ||
return NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to leak file if something will go wrong here.
Whole way of working with file looks quite dangerous, please check how it's done in other apps.
} | ||
|
||
static FlipperFormat* snake_game_open_file() { | ||
Storage* storage = furi_record_open(RECORD_STORAGE); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening and closing storage record doesn't looks good here. The best option is to do it in allocator/deallocator of snake app itself and then pass it here.
} | ||
if(storage_common_stat(storage, SNAKE_GAME_FILE_DIR_PATH, NULL) == FSE_NOT_EXIST) { | ||
if(!storage_simply_mkdir(storage, SNAKE_GAME_FILE_DIR_PATH)) { | ||
return NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leak here too
if(!flipper_format_read_header(file, file_type, &version)) { | ||
furi_string_free(file_type); | ||
snake_game_close_file(file); | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually do {} while(0) and break. Please check how other apps do that
Please un-draft when ready ;-) And let me know if you need any help |
I can't tell whether this means it was merged or is dead. |
@fennectech dead, PR was closed before merged |
What's new
Verification
Checklist (For Reviewer)