Next-Flip / Momentum-Apps

Bundle of external apps tweaked for Momentum Firmware
https://momentum-fw.dev
288 stars 48 forks source link

Feature/color guess #9

Closed leedave closed 6 months ago

leedave commented 6 months ago

Here's another one. The Color Guessing Game.

Willy-JL commented 6 months ago

I have added a second remote from xMasterX's plugin pack, cross remote wasn't there so i didnt for that one. Unfortunately I havent automated this part yet, it's basically just deleting the folder and adding again from a different source, then comparing the diff's. I have it setup where it pulls from the pack first and author second, logic being that with conflicting git histories, this will usually mean that it gets bugfixes for possible refactors first from the pack, and then new features from the author's repo without conflicting too much, reducing the amount of handholding git needs. When I had it the other way around, it would end up removing author's new features when merging bugfixes from his pack repo which did not have the new features yet.

Speaking of which, aside from some files from his pack being a bit outdated, there's 2 main differences: your repo doesnt have a license, and his color_guess_storage.c has a memory leak fix:

diff --git a/color_guess/helpers/color_guess_storage.c b/color_guess/helpers/color_guess_storage.c
index d37103248..31c791881 100644
--- a/color_guess/helpers/color_guess_storage.c
+++ b/color_guess/helpers/color_guess_storage.c
@@ -92,8 +92,10 @@ void color_guess_read_settings(void* context) {
         FURI_LOG_E(TAG, "Missing Header Data");
         color_guess_close_config_file(fff_file);
         color_guess_close_storage();
+        furi_string_free(temp_str);
         return;
     }
+    furi_string_free(temp_str);

     if(file_version < COLOR_GUESS_SETTINGS_FILE_VERSION) {
         FURI_LOG_I(TAG, "old config version, will be removed.");
@@ -109,8 +111,6 @@ void color_guess_read_settings(void* context) {

     flipper_format_rewind(fff_file);

-    furi_string_free(temp_str);
-
     color_guess_close_config_file(fff_file);
     color_guess_close_storage();
 }
\ No newline at end of file

Also, I noticed a bug, bit of an old one we have been dealing with for a while in many different apps. This app doesn't have logic to redraw every now and then, so the timer stays static until the user presses a button. This is usually mitigated by the fact that, when launched via the main menu, this will be in background, periodically issuing a redraw of the gui, which laso redraws your app and udpates the timer. When launched via FBT, or with a desktop favorite app / keybind, it will not update.

Willy-JL commented 6 months ago

Found a few more memory leaks from a very brief look at the code, and fixed the redraw issue. Feel free to backport to your repo )

Willy-JL commented 6 months ago

also is there a particular reason for the furi_hal_region_is_provisioned() and furi_hal_power_suppress_charge_enter() calls?

leedave commented 6 months ago

Thanks a lot for the valuable input

xMasterX's plugin pack I added what I saw from him, but missed the one change you mentioned. Is now in the original. The rest is pretty outdated in his repo.

Memory Management & Tick Event Great additions. I was always confused that fbt launches behaved differently, nice to have that cleaned up.

furi_hal_region_is_provisioned() Was commented out long ago, but now fully removed from code

furi_hal_power_suppress_charge_enter() Rather critical for this app, as it uses the LED light to show colors that need to be guessed. Charging status lights would interrupt that. I generally suppress charge while running apps, as other apps also want to indicate stuff with the LED.

I've updated all changes to my original Repo and added a version number to the start screen. Also added credits for your help to the Readme. Thank you for your great contributions.