Maybe remove the SLOTS_FREE calculation. I don't think you need it. Let's say you tell InfiniTime to make an app screen with 3 pages, and 14 apps. That gives you 4 empty slots. You are manually adding Apps:none in each of these empty slots. I don't think you need to do that. In my Infinitime build I have 14 apps, and the empty slots just show up as gray squares on my watch automatically.

Its easier and works 😊 Thank you very much!

Ok, I think I got it. Works fine for me, the screen count bug seems to be gone. I cannot see what the problem with the formatting is and

clang-format --style=file:index.clang-format -i src/displayapp/DisplayApp.cpp

does not solve it.

@minacode Thank you for your work on this feature, and my apologies for not giving any feedback until now.

I've had this kind of feature in the back of my head for a while now, but I haven't had the opportunity to think about it yet.

I had a quick look at your work : your PRs well documented, easy to read and they focus on a single functionality, good job!

I noticed that this PR uses #ifdef and that's definitely something I would like to avoid as much as possible. #ifdef make the code harder to read and to maintain, especially when the same constants are checked at multiple places of the code. In this case, we'll have to check a new constant at 2 or 3 different places when adding a new app.

Could another design allow to avoid using #ifdef and code generators? I have no specific idea in mind, but I have the feeling that it should be possible to do something interesting using CMake and functionalities from modern C++ (constexpr, for example).

The maintainability of this feature is another concern to me. I guess the final goal of this feature is to add more applications in the InfiniTime codebase than what the hardware is capable of (most because of RAM/FLASH constraints). This way, users will be able to easily build the firmware that contains only the apps they need.

My concerns are multiple:

• How to ensure that all combinations of apps will actually work ?
• The compiler won't return an error if a change that breaks disabled apps is applied.
• Apps that are not enabled won't receive as much testing as the other.

This functionality (together with adding more apps) will require more and more maintenance work from maintainers. The global quality of the project might also decrease because not everything will be tested enough.

Have you already thought about that? What's your opinion?

@minacode Thank you for your work on this feature, and my apologies for not giving any feedback until now.

Don't worry and thank you for taking a look at it.

I noticed that this PR uses #ifdef and that's definitely something I would like to avoid as much as possible. #ifdef make the code harder to read and to maintain, especially when the same constants are checked at multiple places of the code. In this case, we'll have to check a new constant at 2 or 3 different places when adding a new app.

Could another design allow to avoid using #ifdef and code generators? I have no specific idea in mind, but I have the feeling that it should be possible to do something interesting using CMake and functionalities from modern C++ (constexpr, for example).

I am completely open to any idea. The main reason I started it was because I thought it would be good if someone did something, so I did. It's always easier to have a discussion when there is already something. This PR is just a simple proof-of-concept.
I am learning C++ and the world aroud it, but I did not reach CMake yet, so I do not know what is possible with it. I will continue to experiment with reducing the preprocessor macros.

Edit about what I found out since writing this:

1. The #ifs around #includes are not necessary.
2. template<bool containsMetronomeApp> etc might be a way to go if the compiler is able to find all the dead code it has to cut out.

The maintainability of this feature is another concern to me. I guess the final goal of this feature is to add more applications in the InfiniTime codebase than what the hardware is capable of (most because of RAM/FLASH constraints). This way, users will be able to easily build the firmware that contains only the apps they need.

Yes, exactly. People come here with apps that cannot easily reach other users because we cannot merge them. This PR frees the code base from those memory constraints.

My concerns are multiple:

• How to ensure that all combinations of apps will actually work ?
• The compiler won't return an error if a change that breaks disabled apps is applied.

I thought about this and this is my rough take:
We do not check all combinations.
We check a build with all apps, but obviously not on hardware.
We check a default build, as given by the APP_X variables until now. This is the one we also distribute with every version. That is basically just as it is right now.
I theory, we could check a build for each single app. I know, this is a lot and probably not worth it.

• Apps that are not enabled won't receive as much testing as the other.

This is true but still better than no apps. And in the future with loadable third-party apps we will have no control about app quality at all.
My vision is that InfiniTime guarantees you a standard set of apps as it does right now, but gives people the ability to try out less official apps if they want to. If those apps somehow got broken, people need to report an issue or submit a PR and then they will hopefully be fixed. I have the Arch User Repository in mind.
Take the officially released distribution and you have the guarantee that things are well tested, or build your own custom thing from the code base with the risks attached to it.

This functionality (together with adding more apps) will require more and more maintenance work from maintainers. The global quality of the project might also decrease because not everything will be tested enough.

The complexity would grow, that is true. I don't want to tell anyone that we can/should/will do more than is done right now, so I don't know about the rise in maintenance. One more radical idea I had is to split the code base in a core repo and a repo only for apps and have more maintainers for the apps to improve the situation without the core development going out of control.

@JF002, is it problematic to build this PR on C++17/20 features? (Aside from the discussion about where this all is headed anyways...) I removed most of the preprocessor now.

Edit: I need operator[] as constexpr which is from C++17. Also I added some if constexpr, but I think it also works well without them.

Right now, there are still the global APP_X definitions. I think we could reduce them by:

1. Building something like

   struct AppIncluded {
     bool music,
     bool metronome,
     ...
   }

   and passing a reference of that to DisplayApp (via template?).

2. Not having displayapp/AvailableApps.h at all and setting the variables as wanted through CMake.

If this PR uses C++17/20 features, I would say you should change CMAKE_CXX_STANDARD in CMakeLists.txt.

I don't think requiring changes in even more places just to add an app is a good idea. It'll make issue #1571 worse.

I agree. The question is: can we have both optional apps and less places where apps have to be added?

That's what #1571 is for. It'll probably require a lot of refactoring.

@minacode Thanks for your work on this topic! Since you created this PR, we merged #1894 which, I think, provide a similar functionality by selecting the apps that must be built into the firmware at build time. Is this OK for you if we close this one in favor of #1894 ?