My current attempts of refactoring MoveToBeActive

[Someone0nEarth]

Hi @fevieira27 and a happy New Year!

I really like your Watchface and I appreciate a lot, that your are sharing your work as open source :)

For some personal preferences, I wanted to do some small changes in the code. While doing that, I found a lot of redundancy, magic numbers and another things in the existing code, which make it difficult to understand, maintain and improve it. So I started an incremental refactoring approach to make the code more "clean".

The current progress can be seen here: https://github.com/Someone0nEarth/MoveToBeActive/tree/dev

My goal is (when my refactoring reached some progress), to make a pull request to your repository, if you are up for it :)

So, would you up for it?
Do you like my current refactoring attempts?
Do you have some feedback?

Also one code related question:
It seems, that "AnalogSettingsView.mc" has no significant differences between the normal and the low-memory version? So, there is no need for the extra low-memory file, right?

Best regards,
Nils

[fevieira27]

Hi @Someone0nEarth , happy new year and thanks for your help!

Of course I would be open to suggestions and accepting changes from other people, for me that was the whole point about keeping it open source. However, we'd just need to make sure that the code improvements don't increase memory and battery usage too much, as well as keeping support for all the watches that I currently do.

I'm not a proper software developer and really only wanted to create a watch face for myself that would suit my needs, hence why I decided to learn MonkeyC. I'm sure you'd may be able to find ways to improve the code, however since I'm not sure how much experience you have with MonkeyC and Garmin watches (I'm been maintaining this watch face since Feb/2021), then some things you'd need to keep in mind: Some of paths that I may have chosen to follow on the code could have been triggered by memory limitations on some of the supported watches and/or to improve battery / memory usage. With limited memory devices like Garmin watches, sometimes "worse" code (harder to read) could mean less memory usage. Not all the time, but it can happen.

Answering your question regarding the low-memory settings, it's currently being used on Fenix 5 plus watches (all 3 variations of it). The reason is that those watches don't have the capability to display weather-related information, so since the settings menu is a memory hog, I've created a new settings file that is (for now) exclusive to the Fenix 5 and that doesn't have any of the weather-related features. You can check which watch models are using which annotations and source code files within the monkey.jungle file.

I'll have a look at your code next week to do some testing and give you some feedback. Thanks once again, help is always very much appreciated.

[Someone0nEarth]

Hi!

Of course I would be open to suggestions and accepting changes from other people, for me that was the whole point about keeping it open source. However, we'd just need to make sure that the code improvements don't increase memory and battery usage too much, as well as keeping support for all the watches that I currently do.

Nice to hear :) And sure, regarding the memory and battery issues: My current impression is that the refactoring will improve both memory and power consumption.

I'm not a proper software developer and really only wanted to create a watch face for myself that would suit my needs, hence why I decided to learn MonkeyC. I'm sure you'd may be able to find ways to improve the code, however since I'm not sure how much experience you have with MonkeyC and Garmin watches (I'm been maintaining this watch face since Feb/2021), then some things you'd need to keep in mind: Some of paths that I may have chosen to follow on the code could have been triggered by memory limitations on some of the supported watches and/or to improve battery / memory usage. With limited memory devices like Garmin watches, sometimes "worse" code (harder to read) could mean less memory usage. Not all the time, but it can happen.

I understand your concerns regarding memory usage etc. . I have experience with Monkey C and developing software for other smaller devices.

There is a good software developing "philosophy": Make it work, make it right, make it fast :) Which means: First focus on featutes/ functionalities (make it work), then a deeper focus on code design, architecture, clean code etc. (make it right) and finally focus on finding potential bottlenecks, like cpu/battery or memory usage etc. (make it fast). If changes are needed due to performance issues, you simply go through these phases again with the new improvements.

You have already fulfilled the "make it work" part. I am currently trying to improve the "make it right" aspect. Of course, while doing that, I am already keeping an eye on the memory and cpu usage . However, the main focus on that will be during the make it fast phase (identifying and fixing any performance-related issues).

Normally, I would work with a lot of (unit) tests, but in this case, I decided, I will first do the refactoring without unit tests and fater it, I will decide on write tests or not.

Answering your question regarding the low-memory settings, it's currently being used on Fenix 5 plus watches (all 3 variations of it). The reason is that those watches don't have the capability to display weather-related information, so since the settings menu is a memory hog, I've created a new settings file that is (for now) exclusive to the Fenix 5 and that doesn't have any of the weather-related features. You can check which watch models are using which annotations and source code files within the monkey.jungle file.

Thanks! Yes, I saw it on the jungle file. I skipped refactoring the low-memory settings atm (because of the high degree of code redundancy), but in the ende, I will present a low-memory version without weather settings and reduced code redundancy :)

I'll have a look at your code next week to do some testing and give you some feedback. Thanks once again, help is always very much appreciated.

Nice :) Have in mind, that the refactoring is done incremental, so you see the current state in my dev branch (it is not "final" atm). But every commit should be runnable.

Looking forward for your feedback!

[Someone0nEarth]

Hi @fevieira27 !

Just for your information:

My first step of refactoring/remastering is done: The settings/configuration stuff

Beside introducing a central configuration class, I refactored the whole settings menu (only for the "normal" settings profile at the moment. low- and tinymemory will be done later (if there will still a need for it))

At the moment, the memory usage of the running watchface is even 1.6kb less. Also the memory usage within the main menu is a bit reduced, but has a higher usage in some sub menus (but there is still room for improvement). Also the cpu usage should be already improved, too.

My next step will be refactoring the mtba functions file.

[fevieira27]

Hello @Someone0nEarth,

I've had the chance to have a look into the code these past couple of days. Even though it initially took me a while to understand what was happening (lots of changes!), I now understand the purpose of what you're doing and completely agree that it's now much easier to understand from a perspective of someone that's looking at the code first time. It's still going to take me some time to get used to it, but I really appreciate how it looks like now. Can't thank you enough!

I have to say that it must not have been easy for you to understand all that was going on with the watch face, but you nailed it so far. Most of the things are working just like they were in the past, apart from a few small things that are likely easy to fix (listed below).

From my testing so far, I have 1 major concern (not yet an issue, but could become one), 1 issue that is going to require some logic changes, 2 minor issues, and 1 that is for sure due to WIP status of the code. I'll list them below so that we can have a look at them:

1. Major - I can see that the memory usage on the watch face and most of its settings screens are now lower than before (which is great!), but the "data fields" screen is using a bit more (3.5kb out of 54 on Fenix 6s) than before. Not only that, but the peak memory usage (I'm assuming because of that same screen increase) when playing around with the settings screens is also 3.5Kb higher now. It might look small, but it could end up being a bottleneck for some older watches that have small memory availability. The 2 watches that are currently more memory limited are the Fenix 6s pro and especially Venu sq. The latter is very close to reaching that peak memory usage, so we'll need to be careful about this. Hopefully the other changes will reduce memory usage, but just something to bring up and keep in mind during testing.
2. Medium - The accent color is not changing when toggling between dark and light themes, neither when in the settings menu or when returning to the watch face. The background color changes as expected, but not the accent. The only way to make this work is to go back into the settings a second time and change the accent color (not the theme). I don't think it's that easy to fix and might require a change in logic on some of those functions to fix that.
3. Easy - The center of the watch face (hand arbor assembly point) should turn black when on light theme, but it's currently always grey (default for dark theme)
4. Easy - Some default values for settings menu are different now. Examples: Atm. pressure unit, hands thickness
5. Easy - Warnings about "statement not reachable" on MtbA_Functions (3 locations). I'm assuming this is because this is still in early days on WIP refactoring, so for sure it will be addressed later. But would like to point out in case you haven't noticed.
6. Fixed on my local repo - Venu Sq wasn't running due to an incorrect number of parameters and an incorrect function name being called inside the drawHashMarks function (square annotation). So I've removed the extra unused parameter and corrected the function name (from getAODColorMinute to getAodUseAccentColor) and was able to run it without issues. But I don't have access to make a pull request to your repo, so did it locally only.

I've now built the watch face and I'm using it on my watch, for testing purposes. Interested to see if I come across any issues (don't think I will) and to see if there's any noticeable change in battery consumption (for better or worse). Will keep you updated on the next couple of weeks on how the real testing looks like so far.

[Someone0nEarth]

Hi @fevieira27 !

I've had the chance to have a look into the code these past couple of days. Even though it initially took me a while to understand what was happening (lots of changes!), I now understand the purpose of what you're doing and completely agree that it's now much easier to understand from a perspective of someone that's looking at the code first time. It's still going to take me some time to get used to it, but I really appreciate how it looks like now. Can't thank you enough!

I am glad to hear you like the changes :) And yes, clean code helps a lot for readability and maintenance (and not just for new people).

I have to say that it must not have been easy for you to understand all that was going on with the watch face, but you nailed it so far. Most of the things are working just like they were in the past, apart from a few small things that are likely easy to fix (listed below).

Yes, it was (and still is) quite an adventure :D

1. Major - I can see that the memory usage on the watch face and most of its settings screens are now lower than before (which is great!), but the "data fields" screen is using a bit more (3.5kb out of 54 on Fenix 6s) than before. Not only that, but the peak memory usage (I'm assuming because of that same screen increase) when playing around with the settings screens is also 3.5Kb higher now. It might look small, but it could end up being a bottleneck for some older watches that have small memory availability. The 2 watches that are currently more memory limited are the Fenix 6s pro and especially Venu sq. The latter is very close to reaching that peak memory usage, so we'll need to be careful about this. Hopefully the other changes will reduce memory usage, but just something to bring up and keep in mind during testing.

Yes, as I mentioned: At the current phases ("make it right") I will keep an eye on the memory usage, but the main goal is to improve the clean code quality. I will go for memory usage optimizations in the "make it fast" phase :)

2. Medium - The accent color is not changing when toggling between dark and light themes, neither when in the settings menu or when returning to the watch face. The background color changes as expected, but not the accent. The only way to make this work is to go back into the settings a second time and change the accent color (not the theme). I don't think it's that easy to fix and might require a change in logic on some of those functions to fix that.

Oh yes! I would consider it as a mix of "refactoring bug and WIP" ^^ I pushed a fix (it works but it is still WIP uses slight more memory atm).

3. Easy - The center of the watch face (hand arbor assembly point) should turn black when on light theme, but it's currently always grey (default for dark theme)

Hm, I just double check it on your version: Its gray for AoD or light theme and white for dark theme. But maybe I overlooked something. But sure, I can change this, if you want.

4. Easy - Some default values for settings menu are different now. Examples: Atm. pressure unit, hands thickness

Okay. Lets adjust this, after the rework is done.

5. Easy - Warnings about "statement not reachable" on MtbA_Functions (3 locations). I'm assuming this is because this is still in early days on WIP refactoring, so for sure it will be addressed later. But would like to point out in case you haven't noticed.

Oh yes, I noticed them and yes, part of WIP :)

6. Fixed on my local repo - Venu Sq wasn't running due to an incorrect number of parameters and an incorrect function name being called inside the drawHashMarks function (square annotation). So I've removed the extra unused parameter and corrected the function name (from getAODColorMinute to getAodUseAccentColor) and was able to run it without issues. But I don't have access to make a pull request to your repo, so did it locally only.

I invited you as a collaborator, so you can push stuff now :)

But be aware, at the moment, I only refactoring :round (because of the limited support of VS Code handling these kind of annotation while doing refactorings). And the current refactorings have increased memory usage due to transition steps. The "fix" commit I mentioned is done before the new refactoring transitions, so you should still be able to use it.

I will ping you, when I am done with :rectangle. But this could take some time.

I've now built the watch face and I'm using it on my watch, for testing purposes. Interested to see if I come across any issues (don't think I will) and to see if there's any noticeable change in battery consumption (for better or worse). Will keep you updated on the next couple of weeks on how the real testing looks like so far.

Nice! Looking forward to the updates. And thanks a lot for the feedback!