AttorneyOnline / AO2-Client

An off-the-cuff courtroom drama simulator
https://aceattorneyonline.com
GNU General Public License v3.0
61 stars 58 forks source link

Deprecate the default theme #851

Open Salanto opened 2 years ago

Salanto commented 2 years ago

Feel free to skip this part, this was written by a deranged lunatic (also me.)

But what about my 0.8kb space saving? No. Just no. Add all necessary UI elements to your theme or get lost.

Alright, folks, this one is a doozy. You know how we, in 2.10 again, frequently have to tell people to update their themes because someone introduced a new key and due to the lack of default everything looks terrible? Well, how about we just start using QSettings properly and add functions to fetch the necessary info through that instead of opening the ini file over and over? Also, it is time we remove the 3-layer relationship of themes, where subthemes inherit from their parent theme, which in turn can inherit from the default theme. It's unnecessary inheritance bingo.

oldmud0 commented 2 years ago

Sorry but I don't understand this one. Please chill out before writing an issue description.

Add all necessary UI elements to your theme or get lost.

OK, so no default values for themes. But you must also understand that every widget is always displayed and needs a position.

due to the lack of default everything looks terrible

So this would be fixed by having... a proper default value?

Well, how about we just start using QSettings properly and add functions to fetch the necessary info through that instead of opening the ini file over and over?

What does this have to do with the default theme?

Salanto commented 2 years ago

Okay wow, the original one made much more sense when I wrote it. Rn even I can't tell what I was on.

The primary gripe I have with the default theme, especially courtroom_design.ini is that it needs to be updated in order to have baseline functionality working.

This is fine until we remember that it uses QSettings, which already supplies ways to handle a default value, which we can't use due to the generic ini loader approach.

My point is more that the way we have default values is cumbersome and misused.

I am aware that some will argue that theme inheritance, a consequence of the default theme being used when content is missing, is important, but at the same time I argue that if content needs extra logic and an entirely different theme to work, it should not work.

Fixing incomplete assets by doing extra lookups and code considerations can work, but is in my eyes a bad practice.

TLDR: Default values should be realised over QSettings, remove default theme inheritance.

oldmud0 commented 2 years ago

A default value can be supplied when retrieving settings as long as the "get INI value" method in question has a parameter for it. I believe none of these methods have such a parameter, since the intention was apparently to check for a blank value after the method is called and do something with it. And in many cases, there is no such check.

Here is something that annoys me though: We have a lot of widget positions hardcoded inside the courtroom code. So, in theory, you could remove the default theme altogether, and you'll see the game fall back its own hardcoded default values, so long as you can get past the blank lobby (which doesn't work the same way). Try deleting courtroom_design.ini and see what happens.

This sounds a bit crazy and counterproductive, but I'd been thinking of a special default theme that is intrinsic to the program and cannot be modified. It's not hardcoded into the source code, but rather its resources are linked into the executable via qrc. This has two main benefits:

Next, I'd only inherit styles from the default theme. Widget positions should not be inherited, unless the theme author chooses not to write a widget design file (i.e. it's a reskin). Theme inheritance should be fine as long as theme authors don't go overboard with it, and they'll see the game performance suffer if they do.

The misc folder was a bad idea. It lumps general assets into a folder that can be overridden arbitrarily by the character or theme. Instead of trying to check if assets were overridden simply by where such things could be, characters and themes should be specific about what has been overridden. There are certain "constants" in asset searches, such as the names of chat boxes and shouts.

Like CSS, a system of cascading/overridden values is very much manageable, so long as what values are being set is made explicit.