DS-Homebrew / TWiLightMenu

DSi Menu replacement for DS/DSi/3DS/2DS
https://wiki.ds-homebrew.com/twilightmenu/
GNU General Public License v3.0
3.17k stars 199 forks source link

[RFC/WIP] Consolidate common code across themes #758

Open GerbilSoft opened 4 years ago

GerbilSoft commented 4 years ago

Currently, all of the themes appear to duplicate code from each other. This results in issues where code might be updated in one theme, but not another, resulting in theme-specific issues.

What would be done is something like this:

Theme-specific code would go in arm9/ and be enabled using conditional compilation, e.g. -DTHEME_DSI or -DTHEME_AK.

I'll start a proof-of-concept branch myself sometime this weekend to see how far I can get with this.

NightScript370 commented 4 years ago

Currently, all of the themes appear to duplicate code from eachother

Not 100% true. DSi, 3DS and Saturn use the same folder for code with if conditions for the different graphics per themes. Akmenu code is completely different as its a direct port. The only theme in which you can say this is accurate is R4 and Robz said that its because the ROM select code is too different to be able to merged into one.

This results in issues where code might be updated in one theme, but not another, resulting in theme-specific issues.

Again, this only applies to R4 and DSi and any bugfix we apply for DSi goes to R4 and vice versa. The theme-specific issues happens cause either an if statement is messed up or the codebase is completely different and therefore could not be a simple port

Theme-specific code would go in arm9/ and be enabled using conditional compilation, e.g. -DTHEME_DSI or -DTHEME_AK.

Good luck. For example, the 3DS theme has the top menu bar while the DSi theme has a select menu. The R4 theme has a completely custom launch menu not in the other themes.

You'd have to completely redo Acekard and then change most of R4 to comply with DSi/3DS/Saturn

GerbilSoft commented 4 years ago

At the very least, common code should be taken out and put in a common library or similar. For example, SetSpeedBumpExclude() is duplicated in quickmenu, r4theme, and dsimenutheme, and in aktheme, it's BootstrapConfig::speedBumpExclude().

So maybe not a full refactoring, but at least splitting out common code to make it easier to maintain.

chyyran commented 4 years ago

There is a lot of common utility code that can be refactored out. Essentially most of the work has been done in regards to this, just needs to tweaking of makefiles and consolidate the common folder within each theme.

The aktheme config builder classes were meant to eventually replace the methods in the other themes when I wrote them, and provide a clean interface instead of using raw CIniFile calls, but there is more work to do there than just tweaking makefiles. As well, it should probably be extended to handle widescreen now, there is work there to be done in LauncherConfig. Still, it should be relatively trivial.

As for everything else...not possible unless you rewrite stuff. Although if we give custom themes for dsimenu more power, we may be able to move out the runtime stuff that swaps between the DSi Menu, Saturn, and 3DS themes and just implement them as custom themes. This is a whole lot of work however.

If you need any help, feel free to ask in the TWL Hacking Discord

chyyran commented 4 years ago

@GerbilSoft going through your changes, looking good so far except for a5d3c06a5aa0976a7e0a31c8f710f2a5fb3303a1 -- allocating the buffer at runtime will cause memory fragmentation issues. In general it's better to allocate large blocks of memory as early as possible in runtime if not possible statically.

NightScript370 commented 4 years ago

Do we need this here? This isn't necessarily an issue with the app or a suggestion, so wouldn't it be better to have something like this pinned in the discord server?

FlameKat53 commented 4 years ago

was @NightYoshi370 working on something like this?

NightScript370 commented 4 years ago

No I was not. I still find this a good idea to try and implement though

chyyran commented 4 years ago

Working on this once akmenu bugs are fixed