HomeWildfire Games

Cleanup mod selection GUI page.
Needs VerificationrP20552

Description

Cleanup mod selection GUI page.

Refactor functions and unify sort dropdown choices.
Move colors to globals so that they are easily modifiable, even from external files, refs rP20195 / D911.
Fix broken "Dependency not met" translation and wrong equal comparison operator description (== vs =), refs rP15677.
Use localeCompare, simpler loops and array functions, JSdoc syntax, ternaries, deepfreeze, let keyword and prefix increment operator.
Renames to increase descriptiveness and consistency.
Remove duplication, tautologic and unsatisfiable conditions, dead code, unused variables and GUI object names, example within example,
unneeded variables, parentheses, TODOs, strict checks and keep XML element, misleading linebreaks, pointless comments and "Mods Loaded" string and the switch fall-through.

Details

Auditors
leper
Group Auditors
Restricted Owners Package
Committed
elexisNov 28 2017, 11:43 PM
Parents
rP20551: Allow civ specific techs with {civ}
Branches
Unknown
Tags
Unknown
Build Status
Buildable 3871
Build 6735: Post-Commit BuildJenkins

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
This commit now has outstanding concerns.Nov 29 2017, 12:49 AM

Thanks for the in-depth audit!

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
11

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.

28
  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.
34

ack

36

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).

42

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.

51

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

87

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

91

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.

112

"// 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.

119

Only need one copy of an explanation

129

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

169

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

185

(Could also be done in the XML iirc)

203

Ok

307

(See above)

323

0ad=0.0.23

327

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

332

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

337

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.

353

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?

371

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

378

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 :/)

380

(Even the extraction failed)

424

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

483

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

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.xml
207

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.

elexis added inline comments.Nov 29 2017, 12:10 PM
/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
92

Not preposterous to claim that g_ModsDisabled is g_Mods minus g_ModsEnabled.

188

(These properties are required)

leper added inline comments.Nov 29 2017, 10:15 PM
/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
11

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.

28

Yes, because that modifies the enabled list.

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

42

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.

51

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

91

Then why did you remove the function that already existed?

92

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

112

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.

119

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).

129

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?

323

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

337

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.

353

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.

371

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

378

Good luck with moving fast and breaking things.

424

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.

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.xml
207

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

elexis requested verification of this commit.Dec 18 2017, 8:50 PM
elexis marked 30 inline comments as done.
elexis added inline comments.
/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
11

rP20637 adds the explanatory specification the mod.json properties missing since rP15677.

28

I never heard of https://en.wikipedia.org/wiki/Vertical_axis_wind_turbine until now.

changing the terminology without even thinking...

The opposite is true

  1. because it was added without thinking in rP15677 (observerved at the contradiction GetAvailableMods != g_modsAvailable)
  2. because the commit addeded symmetry (g_ModsEnabled vs g_ModsDisabled), rendering it obvious (i.e. without reading any character besides the two variable names) that the Arrays are disjunct and that their concatenation is`g_Mods`.

The naming in this commit is:

available mods = enabled mods + disabled mods

Previously it was:

GetAvailableMods = available mods + enabled mods

To gain the benefit of the symmetry, the only alternative naming would have been unenabled.
But if there is the choice between an uncommon term like unenabled and an equally correct, common term such as disabled, one should prefer the common one since that allows the reader to relate to familiar patterns, lowering the difficulty and temporal effort to comprehend the GUI page.

If we had this unfriendly fruitless discussion in a review about linting and dead code removal patch, then none of the other patches that implement non-linting features, including those stated missing by both of us and others on trac, could have been authored in the same moon cycle, let alone the same day. If the features would have been written at all.

In 17 years of software engineering I have never seen a "bushfire".
Even the worst release blockers, OOS or pathfinder bugs, duplication massacres, camouflaged logic bugs and software design issues are correctable in a complexity less than proportional to the complexity of the correctable commits.
Commits are split by logic, so they can be reverted quickly if not fixed quickly.
So no matter how bad a commit is, any possible harm to the software structure can be undone quickly if there is noone interested in fixing something.

The few oversights in this commit were so simple (one liners) that they were and have been found instantly, so there was no harm anywhere comparable to the time and energy lost in fruitless repetitive discussions about non-issues.

If we allow people to accept patches that cannot or do not comprehend the code of a commit or only test if a feature works as advertized, we have gained at most a syntax check while degrading the developer, making him inevitably repeatedly beg for reviews and by trusting him less than a person that doesn't comprehend the work, additionally making the reviewer lose hours if not days that could have been invested in life if not useful features.

If you actually want shorter release cycles without losing code quality, revert to the guideline of 2015 (which hadn't been the problem it was portrayed as to begin with) or adapt the guideline to reflect that the responsability of developers to keep code quality goes both ways.

36

The mod types committed in rP15677 were deleted in rP20575.

42

If you are going to argue with modifiability at least do it properly. This change doesn't.

Judging from your comment below "g_OpeningTagFoo, g_EndTagFoo" and your commits, you mean rP16262 which is improper because it violates the separation of concerns pattern and serves the duplication anti-pattern.

Properly implementing mod support in this commit means doing what every other GUI page does.
Properly enhancing mod support beyond that means doing it in separate commits.

rP20585, which we have fruitlessly debated 2015 already, is the first step towards the independent feature proposal that was mentioned but not responded to above. P99 was the first unaccepted template to going for the second part. So we are wasting both our time here discussing a commit of the past.

Given that you have never complained about any other code that you read and contains the same pattern (session, gamesetup, summary, lobby) means that the logic in comments about this commit is different from the logic of comments on other code.

51

False, rP20637 shows that the validation of mod values that was missing since rP15677 is more nicely doable in new code.

Doing validation while parsing implements the anti-pattern of "Shotgun Parsing" (there was a good lecture by Meredith Patterson on some conference, I only find the slides quickly http://langsec.org/ShotgunParsersShmoo.pdf).
Only confirmed valid input data should be processed and the invalid data should be rejected prior to processing.
rP20637 still lists invalid mods, but invalid fallback representations of invalid mods ought to be displayed if an "invalid mods" filter checkbox (similar to the one in the replay menu) were added.

87

(and I had corrected this oversight in a previous iteration of a patch I had lost before the commit.)

91

because is not a word that I consider unknown to the reader.

92

Every reader of the variable cares about disabled mods because it holds the disabled mods because it stores the sorting order.

112

If sort mods before saving makes one stumble and think, then so does a call to sortEnabledMods.

129

Listed the unsimple feature request which might become nice-to-have once there are dozens of mods and mods that don't only depend on the public mod at #4890. The ticket is central due to it's summarizing nature.
It was you who tought me that we use issue trackers to track issues rather than collecting TODOs in the code.
Furthermore since the column based sorting in rP20555, it is self-evident that sorting mods by dependency should not be a simple string comparison ideally.

151

Furthermore passing not one array of objects but many arrays means that the arrays can have different number of elements. A suboptimal noncrucial unrelated design choice which we should have a ticket for, so that GUI people who actually do use trac will stumble upon it and can extend the discussion years later.

185

The answer doesn't qualify why the folder is more important than the rest, let alone why it should be visualized using a differently colored column, let alone dark gray on black in rP15677.

203

No patch touched this hunk yet.

323

Got the hint, but as stated documentation on the mod format should be somewhere at the top of the file as done in rP20637, which is why I only answered the question.

332

This is the only oversight which wouldn't have been discovered until a mod uses that less likely comparison operation, fixed in r20574.
It certainly wouldn't have been discovered by review that is not mandated to verify.

337

Indeed the mods would have to know about that versioning scheme in advance.

Could probably be removed, but in that case we should error hard

Both done in rP20637

Releasing new versions compatible to each other is still covered by the feature you formulated in the other comment (two mods should use the same identifier (name + version) if and only if they implement the same dependency).

The user should distinguish mods based on the human-readable label (possibly the D1080 mod version label) or the machine-readable unique identifier folder.

A more stable system of orderable version numbers independent from dependency matching would be a custom property or reserved version number part (for instance the third or fourth number).

353

Greping IRC logs is successful if one recalls a specific phrase or knows a specific date. For instance I have failed to find an explanation why a funny directory name is prefered over a descriptive one (D1095).

rP20637 solves it without shotgun parsing, so the two lines could be entirely removed.

371

If adding dead code and adding a warning if it is reached is recommended, I will certainly have some entertaining patches to write.

378

Writing a patch and fixing oversights in 10min is by 10 hours faster than writing the same patch and writing proofs for 10 hours.

424

that I have as a TODO locally

If you send me a mailbox version of your code I can rebase it.

a mod that provides what another does

If that is a design choice, it is not obvious from the code, not documented in the code or the wiki page either, the GUI contradicts it and the software architecture could have taken that better into account (by receiving an array of dependency identifiers).

The column order should correspond to the precedence of the identifiers (folder as a unique identifier first, then the non-unique dependency identifier that all mods I encountered confused with a human-readable label property).

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.xml
207

XML is code, just not source code of a programming language.

I'm not the one that started out changing tons of things for the lulz.

I'm not the one requiring developers to write arguments for hours for the lulz or reducing patches to the subset of defects.

This commit now requires verification by auditors.Dec 18 2017, 8:50 PM
leper raised a concern with this commit.Dec 19 2017, 6:39 PM

Still broken. Well I guess because at some point I stopped reading commits that fixed one thing and broke at least two others.

I suspect the mod support code will just break in more ways until it isn't distinguishable from something random anymore.

Also nice way of interpreting a presentation out of context.

This commit now has outstanding concerns.Dec 19 2017, 6:39 PM
elexis added an auditor: Restricted Owners Package.Dec 19 2017, 8:18 PM

Still broken.

You mean that one thing?

Well I guess because at some point I stopped reading commits

And I appreciate constructive criticism.
Evidently we are not the only ones reading the code of this GUI page.
Audits are as voluntary as any other part of the development and they must be considered as such if one doesn't develop the affected code.

that fixed one thing and broke at least two others.

Correct, two commits fixed many things broken in mine and your code and broke at least two lines of code each, so it fixed one thing and broke at least two others.
It is false that things get progressively worse by a ratio of 2 to 1 or that things get worse at all.

I suspect the mod support code will just break in more ways until it isn't distinguishable from something random anymore

Neither your code nor the JS comments defined the input language, now it does.
The design from which the input language is derived is now documented.
This is the opposite of entropy.

invalidating parts of the design

I don't know what you are refering to. This commit addressed linting issues and removed dead code but did not change any part of the design.

instead of figuring out or even just asking to clarify that

Neither know what is meant here.
Every tangent design topic was discussed with you and either never programmed or uploaded for review after this linting patch:

  • removal of -_ in version numbers
  • removal of the type property
  • translated mod label and mod version label
  • distinct mod names (asked without any patch)

being smug

Taking this to the personal level is the last thing I am here for, so I aim to avoid the words "I" and "you" in comments but speak about the effects of code.
But if this is taken to the personal level, I have to say that you are blaming my personality for code issues that I didn't introduce or worsen or issues that don't exist to begin with,
and now I have to listen to third parties unfamiliar with the discussed code taking that condemnation as truth and recommending me I should simply agree with anything independent of any rational arguments on topic.

I do feel sorry about the 6 wrong variable names in the 700 added lines, fixed by rP20574 and rP20656.
If there is any good argument to change something, I will not delay a fix.
But the less proportionate and rational judgment becomes, the less I can understand it or any decision based on it.
I realize this is an unfortunate time for disputes, but I cannot continue anymore leaving these claims of code doom unchallenged for the fourth year.

We have done many good and bad things to code, but at the end of the day it's always getting better, not worse when we don't stall development.

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
424

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).

elexis requested verification of this commit.Dec 31 2017, 1:44 PM

The comments so far were all responded to in word or deed.
The revision was briefly discussed with bb on irc.
Mentioning the subject of the concern enables clarification or action.

This commit now requires verification by auditors.Dec 31 2017, 1:44 PM
bb added a subscriber: bb.Jan 2 2018, 12:45 AM
bb added inline comments.
/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
42

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

112

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.

483

So maybe just have the second part as comment?

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.xml
207

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.

elexis added a comment.Jan 2 2018, 1:18 AM

Thanks bb for reading through the long list of comments and the two changed files and finding the courage to enter the stage.
Do you see any regression in this commit that was not addressed?
Anything that invalidates parts of the design?
Anything (besides the forgotton variables) actually changing code flow?
Was it a useful commit?

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.js
92

bb what was your opinion on the naming?

112

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.

483

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]);

/ps/trunk/binaries/data/mods/mod/gui/modmod/modmod.xml
207

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.