I don't expect the average modder or even many versed developers to comprehend // (name({<,<=,==,>=,>}version)?)+.
Definitions shouldn't be inside an example.
(<=|>=|<|>|=) instead of {<,<=,==,>=,>} is found two statements in the JS documentation below the example.
Modding_Guide contains more human readable words, but still lacks some definition.
It would be best to write sufficiently elaborate proper documentation separately so that we can achieve complete documentation.

1. The transitions between the two disjoint sets are called disable and enable in both code and GUI. If disable is improper, then so is enableMod, g_EnabledMods and the according strings.
2. If available mods are exactly disabled mods, then Engine.GetAvailableMods() should return only disabled mods or be renamed to GetExistingMods.

ack

Wouldn't disagree to delete types, because neither use case nor feature are well defined.
Removing it won't break mods, because the codebase doesn't complain about excess properties (yet).

Feature idea induced.

lobby.js, gamesetup.js, session.js are examples without the tag in globals.
tooltips.js is an example with a tag in the global.
The latter is ugly IMO (both global defs and functions using them).

If we want to allow a font in every place that allows changing of the color, then it could be [color, font] or an equal object everywhere. Thus increasing modifiability while keeping the benefit of separation of data and code.

mod JSON validation would be a great addition, especially when performed before processing the data.

I should have looked at the undefined entries of jshint too.

Had that function until I reverted it because it only contains 2 statements that are executed only once on init. Not opposed to such a function though.

"// sort mods before saving" before a "sortMods" and saving is redundant.
The reasoning is not unclear but absent.
Thanks for explaining why this is sorted here though, I was one of the readers who it wasn't transparent to.

Only need one copy of an explanation

TODOs should be reserved for important defects that aren't self evident. A simple trac ticket with two sentences would be already be sufficient to document an improvement that can be implemented if time permits

It's not complex, just needs some time thrown at it if one wants to look into removing the var.

(Could also be done in the XML iirc)

Ok

(See above)

0ad=0.0.23

Being used twice if I hadn't forgotton one reference.

Thanks. Still works for "=", ">", "<", but not "<=" and "=>" which is why I didn't notice it when testing.

I'd propose deletion instead documenting the weird special case. If someone wants to release a new version compatible to an older version, he can alread do that with the <= checks by having the last number denote non-simulation updates in all versions of the mod.

Indeed. Misses a warn if we don't do the validation in a separate function and the continue should have been a return false;, right?

Introducing a dead scope seems more problematic than showing an error if it's triggered.

misses g_CompareVersion in the split. Wasn't found because the sorting still worked with the versions attached. (I had already noticed and fixed that before I overwrote the patch with something I had something something reviewed :/)

(Even the extraction failed)

Enabling two mods with the same identifier sounds like a reason to warn independent of the versions used.

Not pointless but redundant. First part self evident, second part would have been more clear if g_CompareVersion wouldn't have been forgotton.

Unused code misleads the reader to believe that it is used (public mod argument).
For mods, you are right in general, but one should show likeliness of usability when adding unused code.
Here, if a mod wants to remove a wrapper, it should be removed, not hidden.
If a mod wants to relocate it, it likely not only has to have its size property changed but also receive a different parent.
If we care to maximize mod support of XML files, the panel elements like filters should be moved to separate files probably.

Not preposterous to claim that g_ModsDisabled is g_Mods minus g_ModsEnabled.

(These properties are required)

Found that only once. And that was not in the documentation.

And having documentation that at least lists something even if it isn't directly understood by everyone is better than just having that somewhere else.

Yes, because that modifies the enabled list.

Yes, naming of some things can be improved, but changing the terminology without even thinking...

Why is the color special but the font cannot be? If you are going to argue with modifiability at least do it properly. This change doesn't.

That's what can be done in this loop by just continuing. Not so nicely doable in the new code.

Then why did you remove the function that already existed?

Nobody cares about what is disabled. The only thing we should care about is what is enabled, and what can be.

So now we don't even have a bad comment that might make anyone stop and think. So someone will just remove the sorting and things will eventually break and nobody will have an idea why.

So someone will change this, nobody will look at the other hunk that does something similar and things break in one case, until someone decides that the shorter version is nicer and also breaks the long one, because nobody reads comments (as indicated by this removal).

Nothing prevents a TODO and a trac ticket. now someone reading or working on this code will just not know that this might be nice, and who checks trac tickets?

That is not obvious from the comment, in case you didn't get the hint.

That does not fix the issue at hand at all. One cannot release a new version of a mod that others depend on it without having them change things.

No. I'm not going to do your thinking for you if you first decide that you know better then don't invest the needed time to understand the issue.

I suspect there are some discussions one could dig up, but I guess nobody does that anymore. Would reveal one source of inspiration though, not that anyone cares.

How would you do that then? Ah, right you don't.

Good luck with moving fast and breaking things.

Until someone makes a mod that provides what another does, and someone removes and changes code without a clue what that code is meant to do.

Then having hardened the code against such issues would come in handy.

A name is not unused code. But then again I'm not the one that started out changing tons of things for the lulz.

Also the use case of one mod providing what another does is not identical with two mods with the same name but different versions becoming enabled.

And yes, a dependency requiring a version above X and below Y was and still is not covered by the code, but can be changed easily (changing some to every and a x && y to !x || y afaics).

We seem to have a general setter since rP20697, so swapping all the calls from the colorfunction to this would re-symmetrise the fonts and color

The comment as it was indeed doesn't contain any useful information with that function name below, but without any comment it is unclear why we execute the function, thus a comment explaining that would be in place.

So maybe just have the second part as comment?

Personally I would have left the names, as they can be good for mods and ease changing js a bit, but removed the comments, as they just mirror the names in most cases.

bb what was your opinion on the naming?

If a mod depends on another mod, then the other mod must be launched prior to the other mod to satisfy the dependency of the mod.

Code should become self-explanatory, documentation is only a fallback if that's not possible. So we should always aim to make things as easy to comprehend as possilbe.
I'd propose a rename to sortEnabledModsDependency.

We can add a comment // Ensure mod dependencies remaining satisfied, but I don't see the impending doom not having it.

Doesn't the reader get the same information plus the reward of having understood and validated the code when reading dependencies[folder] = g_Mods[folder].dependencies.map(d => d.split()[0]);

Keeping the unused names as usable for mods seems seems to be a valid, argument, because it allows the mods to change the styles and sizes without overwriting the XML.
So the removal of the names was wrong in this and one or two other commits (just as not introducing names was wrong in every other gui xml commit) and should be fixed globally starting here then.