I'm going to write an announcement, you already know what this is about!
Details
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Branch
- /ps/trunk
- Lint
Lint Passed - Unit
No Test Coverage - Build Status
Buildable 5937 Build 9914: Vulcan Build Jenkins Build 9913: arc lint + arc unit
Event Timeline
binaries/data/mods/mod/l10n/messages.json | ||
---|---|---|
46 | I'll let you guess this one. | |
source/ps/ModIo.cpp | ||
155 | I'm not sure I agree with any of those, given that some of those also misuse defines meant for parameters only and break code if those aren't defined. | |
317 | A switch over an enum where all entries of the enum are handled does not need a default case. If it does that might hide compiler warnings when someone changes the entries of the enum. | |
479 | Hm, maybe mismatch would be another option. | |
source/ps/ModIo.h | ||
57 | I'll repeat the enum class thing since it seem to have been ignored. | |
130 | Seems like it. | |
source/ps/scripting/JSInterface_Mod.cpp | ||
162 | If we have a public definiton of a type, why not also have a public conversion function for that type, instead of having one line for each of those in here? (hint, the block starts here.) |
Another iteration. Includes @s0600204's change to the download size units, and changes from me after your comments. I also made a change to the error propagation in parsing. It uses the production instance.
Concerning the big struct: I made this: https://gitlab.com/na-Itms/0ad/commit/c7d189b17740c86eb6c85bb056641f0200493834. It improves the separation between headers and implementation, uses some of your ideas, but as you can see it doesn't even save 10 lines of code. Tell me if you think that's an improvement regardless. The "pair" suggestion still remains very obscure to me, I guess it's because I wrote this and can't see things differently.
Concerning things that were not fixed: I am a bit drowning in comments made on different diffs. I took care of going through them linearly each time, I did ignore a few ones that were about style but I don't think I missed some that were not. Please tell me if I'm wrong and if yes where.
binaries/data/mods/mod/l10n/messages.json | ||
---|---|---|
35 | I was clinging to the use of separate folders like we do in the public mod, but that is actually because of the separation into resources, that doesn't exist in the mod mod. | |
source/ps/ModIo.cpp | ||
317 | Not all the entries are handled, as you can see. And if the status is none of those three, we shouldn't do anything here, hence default: break;. What is wrong? | |
707 | This err is not supposed to be the same as the ParseSignature one, since FAILing in ParseSignature is not supposed to make ParseModsResponse fail. So I removed propagation (the signature parsing error is actually only used in tests). |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Default... Executing section Source... Executing section JS... binaries/data/mods/mod/gui/msgbox/msgbox.js | 52| » switch·(captions.length) | | [NORMAL] ESLintBear (default-case): | | Expected a default case. | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 92| 92| function translateWithContext(context, message) | 93| 93| { | 94| 94| if (!g_TranslationsWithContext[context]) | 95| |- g_TranslationsWithContext[context] = {} | | 95|+ g_TranslationsWithContext[context] = {}; | 96| 96| | 97| 97| if (!g_TranslationsWithContext[context][message]) | 98| 98| g_TranslationsWithContext[context][message] = Engine.TranslateWithContext(context, message); | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 194| 194| function translateObjectKeys(object, keys) | 195| 195| { | 196| 196| if (keys instanceof Array) | 197| |- { | | 197|+ | 198| 198| for (let property in object) | 199| 199| { | 200| 200| if (keys.indexOf(property) > -1) | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| 209| } | 210| |- } | | 210|+ | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 196| 196| if (keys instanceof Array) | 197| 197| { | 198| 198| for (let property in object) | 199| |- { | | 199|+ | 200| 200| if (keys.indexOf(property) > -1) | 201| 201| { | 202| 202| if (isTranslatableString(object[property])) | 206| 206| } | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| |- } | | 209|+ | 210| 210| } | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | 214| 214| else | 215| |- { | | 215|+ | 216| 216| for (let property in object) | 217| 217| { | 218| 218| if (property in keys) | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| 230| } | 231| |- } | 232| |-} | | 231|+ | | 232|+} | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 214| 214| else | 215| 215| { | 216| 216| for (let property in object) | 217| |- { | | 217|+ | 218| 218| if (property in keys) | 219| 219| { | 220| 220| if (isTranslatableString(object[property])) | 227| 227| } | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| |- } | | 230|+ | 231| 231| } | 232| 232| } binaries/data/mods/mod/gui/common/l10n.js | 95| » » g_TranslationsWithContext[context]·=·{} | | [NORMAL] JSHintBear: | | Missing semicolon. binaries/data/mods/public/gui/common/functions_utility.js | 178| » }·catch·(e)·{ | | [NORMAL] ESLintBear (no-empty): | | Empty block statement. | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'failed_listing'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 85| 85| [closePage, init]); | 86| 86| return false; | 87| 87| }, | 88| |- "failed_listing": progressData => { | | 88|+ "failed_listing": progressData => { | 89| 89| // Mod list couldn't be retrieved | 90| 90| showErrorMessageBox( | 91| 91| sprintf(translateWithContext("mod.io error message", "Mod List could not be retrieved.\n\n%(technicalDetails)s"), { | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'failed_downloading'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 96| 96| [cancelRequest, updateModList]); | 97| 97| return false; | 98| 98| }, | 99| |- "failed_downloading": progressData => { | | 99|+ "failed_downloading": progressData => { | 100| 100| // File couldn't be retrieved | 101| 101| showErrorMessageBox( | 102| 102| sprintf(translateWithContext("mod.io error message", "File download failed.\n\n%(technicalDetails)s"), { | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'failed_filecheck'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 107| 107| [cancelRequest, downloadMod]); | 108| 108| return false; | 109| 109| }, | 110| |- "failed_filecheck": progressData => { | | 110|+ "failed_filecheck": progressData => { | 111| 111| // The file is corrupted | 112| 112| showErrorMessageBox( | 113| 113| sprintf(translateWithContext("mod.io error message", "File verification error.\n\n%(technicalDetails)s"), { | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'none'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 121| 121| /** | 122| 122| * Default | 123| 123| */ | 124| |- "none": progressData => { | | 124|+ "none": progressData => { | 125| 125| // Nothing has happened yet. | 126| 126| return true; | 127| 127| } | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 308| 308| | 309| 309| let elapsedTime = Date.now() - g_RequestStartTime; | 310| 310| let remainingTime = progressPercent ? (100 - progressPercent) * elapsedTime / progressPercent : 0; | 311| |- let avgSpeed = filesizeToObj(transferredSize / (elapsedTime / 1000)) | | 311|+ let avgSpeed = filesizeToObj(transferredSize / (elapsedTime / 1000)); | 312| 312| // Translation: Mod file download status message. | 313| 313| Engine.GetGUIObjectByName("downloadDialog_status").caption = sprintf(translate("Time Elapsed: %(elapsed)s\nEstimated Time Remaining: %(remaining)s\nAverage Speed: %(avgSpeed)s"), { | 314| 314| "elapsed": timeToString(elapsedTime), binaries/data/mods/mod/gui/modio/modio.js | 311| » let·avgSpeed·=·filesizeToObj(transferredSize·/·(elapsedTime·/·1000)) | | [NORMAL] JSHintBear: | | Missing semicolon. | | [NORMAL] ESLintBear (indent): | | Expected indentation of 3 tabs but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | 367| 367| | 368| 368| g_ModsEnabled.sort((folder1, folder2) => | 369| 369| dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 : | 370| |- dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | | 370|+ dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | 371| 371| | 372| 372| displayModList("modsEnabledList", g_ModsEnabled); | 373| 373| }
Link to build: https://jenkins.wildfiregames.com/job/differential/406/display/redirect
One of the recent gui changes seems a step on the road of using stale data if things go wrong.
Concerning the big struct: I made this: https://gitlab.com/na-Itms/0ad/commit/c7d189b17740c86eb6c85bb056641f0200493834. It improves the separation between headers and implementation, uses some of your ideas, but as you can see it doesn't even save 10 lines of code. Tell me if you think that's an improvement regardless. The "pair" suggestion still remains very obscure to me, I guess it's because I wrote this and can't see things differently.
That's because you still cling to lots of things that seem pointless from an outside perspective. You have lots of code handling "logic" that shouldn't be needed for the most part, mapping code that shouldn't be needed (there are two ways to solve that issue, but well), and overall thinking that having this complicated makes anything easier than just having it complicated and a mess to maintain (not that I suspect that this part of the code will get a lot of changes afterwards).
You have a struct with 2 (well 3, but that's related to clinging to IMO useless things), why not just have a pair (or tuple if you really need those 3 things, but I doubt it), and if you really need it 2 functions to operate on that. To make things nicer to read you could use a typedef.
From what I've seen you decided to move code around, instead of thinking why that code exists in the first place. By doing that I don't see how you can improve things apart from not having the definition mostly public (which given the enum that is possibly needed elsewhere (and if it isn't why is it there in the first place) is sort of a requirement).
Concerning things that were not fixed: I am a bit drowning in comments made on different diffs. I took care of going through them linearly each time, I did ignore a few ones that were about style but I don't think I missed some that were not. Please tell me if I'm wrong and if yes where.
Well, there are a bunch by now, dunno if one can only show recent ones, or ones made on that version of the diff. That said if one would fix all the issues in a bunch of comments first before pushing a new version that would help with managing things.
I might go through the comments to check sometime soon, but I can read through things probably about as well as you can.
The below should be most of the things that are still not fixed, there might be more, but there are more interesting things to do than complain about things that were broken relatively recently...
About the huge struct see the last comment, then think about the whole issue without considering what code is currently there, rather thinking about what is really needed.
binaries/data/mods/mod/gui/common/functions_msgbox.js | ||
---|---|---|
48 ↗ | (On Diff #6396) | The move of those things could have been done already if you consider the diff to be too large. |
binaries/data/mods/mod/gui/modio/modio.js | ||
313 | That was referring to the progressPercent option above. I take it that one of the progress dialogs does not display a progress bar. | |
binaries/data/mods/mod/l10n/messages.json | ||
35 | Yes, and that is mostly done to make it easier to translate certain parts of the game. Though I suspect even there it might be possible to make things easier there by using exclusion and inclusion masks. | |
source/ps/ModIo.cpp | ||
33 | I suppose you consider that a stylistic issue, but the way includes are handled here and elsewhere seems both ridiculous and maintenance-unfriendly. What if you move one file in that same directory elsewhere? Right you need to update not only that file, but all other files that include it. As opposed to only one of those with proper full paths. | |
124 | This was not fixed, see the comment a bit above. The latter part of the comment was also not really fixed or considered... | |
124 | Not having this fail, and fail hard at that possibly opens an issue in that the resulting state of those values allows someone to sign things and get them accepted. I repeat, this has to fail hard. |
Almost ready. I still haven't figured out the tuple thing but I'll give it a last go tomorrow after a good night's sleep. I also left the linter comments aside until the last moment, which is either now or tomorrow if I manage to write the tuple.
binaries/data/mods/mod/gui/modio/modio.js | ||
---|---|---|
313 | Yeah one of the dialogs has a bar, the others don't. The boolean leaves us with one neat function with no code duplication (and its's cleaner in the current version of the patch). | |
source/ps/ModIo.cpp | ||
33 | You are right, it is maintenance-unfriendly, I hadn't thought of this. I put that back in order and I'll change that part of the conventions. | |
124 | Now fails hard again and prevents the key to stay set to something that could create security concerns (the value reset prevents me from being consistent with the other ENSURE but if I understood correctly (maybe I didn't) this wasn't your actual issue). |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Default... Executing section Source... source/ps/ModIo.h | 55| enum·class·DownloadProgressStatus·{ | | [MAJOR] CPPCheckBear (syntaxError): | | syntax error Executing section JS... binaries/data/mods/mod/gui/msgbox/msgbox.js | 52| » switch·(captions.length) | | [NORMAL] ESLintBear (default-case): | | Expected a default case. binaries/data/mods/public/gui/common/functions_utility.js | 178| » }·catch·(e)·{ | | [NORMAL] ESLintBear (no-empty): | | Empty block statement. | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'failed_listing'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 85| 85| [closePage, init]); | 86| 86| return false; | 87| 87| }, | 88| |- "failed_listing": progressData => { | | 88|+ "failed_listing": progressData => { | 89| 89| // Mod list couldn't be retrieved | 90| 90| showErrorMessageBox( | 91| 91| sprintf(translateWithContext("mod.io error message", "Mod List could not be retrieved.\n\n%(technicalDetails)s"), { | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'failed_downloading'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 96| 96| [cancelRequest, updateModList]); | 97| 97| return false; | 98| 98| }, | 99| |- "failed_downloading": progressData => { | | 99|+ "failed_downloading": progressData => { | 100| 100| // File couldn't be retrieved | 101| 101| showErrorMessageBox( | 102| 102| sprintf(translateWithContext("mod.io error message", "File download failed.\n\n%(technicalDetails)s"), { | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'failed_filecheck'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 107| 107| [cancelRequest, downloadMod]); | 108| 108| return false; | 109| 109| }, | 110| |- "failed_filecheck": progressData => { | | 110|+ "failed_filecheck": progressData => { | 111| 111| // The file is corrupted | 112| 112| showErrorMessageBox( | 113| 113| sprintf(translateWithContext("mod.io error message", "File verification error.\n\n%(technicalDetails)s"), { | | [NORMAL] ESLintBear (key-spacing): | | Extra space before value for key 'none'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 121| 121| /** | 122| 122| * Default | 123| 123| */ | 124| |- "none": progressData => { | | 124|+ "none": progressData => { | 125| 125| // Nothing has happened yet. | 126| 126| return true; | 127| 127| } | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 187| 187| function clearModList() | 188| 188| { | 189| 189| let modsAvailableList = Engine.GetGUIObjectByName("modsAvailableList"); | 190| |- modsAvailableList.selected = -1 | | 190|+ modsAvailableList.selected = -1; | 191| 191| for (let listIdx of Object.keys(modsAvailableList).filter(key => key.startsWith("list"))) | 192| 192| modsAvailableList[listIdx] = []; | 193| 193| } | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modio/modio.js | 323| 323| | 324| 324| let elapsedTime = Date.now() - g_RequestStartTime; | 325| 325| let remainingTime = progressPercent ? (100 - progressPercent) * elapsedTime / progressPercent : 0; | 326| |- let avgSpeed = filesizeToObj(transferredSize / (elapsedTime / 1000)) | | 326|+ let avgSpeed = filesizeToObj(transferredSize / (elapsedTime / 1000)); | 327| 327| // Translation: Mod file download status message. | 328| 328| Engine.GetGUIObjectByName("downloadDialog_status").caption = sprintf(translate("Time Elapsed: %(elapsed)s\nEstimated Time Remaining: %(remaining)s\nAverage Speed: %(avgSpeed)s"), { | 329| 329| "elapsed": timeToString(elapsedTime), binaries/data/mods/mod/gui/modio/modio.js | 190| » modsAvailableList.selected·=·-1 | | [NORMAL] JSHintBear: | | Missing semicolon. binaries/data/mods/mod/gui/modio/modio.js | 326| » let·avgSpeed·=·filesizeToObj(transferredSize·/·(elapsedTime·/·1000)) | | [NORMAL] JSHintBear: | | Missing semicolon. | | [NORMAL] ESLintBear (semi): | | Missing semicolon. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 92| 92| function translateWithContext(context, message) | 93| 93| { | 94| 94| if (!g_TranslationsWithContext[context]) | 95| |- g_TranslationsWithContext[context] = {} | | 95|+ g_TranslationsWithContext[context] = {}; | 96| 96| | 97| 97| if (!g_TranslationsWithContext[context][message]) | 98| 98| g_TranslationsWithContext[context][message] = Engine.TranslateWithContext(context, message); | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 194| 194| function translateObjectKeys(object, keys) | 195| 195| { | 196| 196| if (keys instanceof Array) | 197| |- { | | 197|+ | 198| 198| for (let property in object) | 199| 199| { | 200| 200| if (keys.indexOf(property) > -1) | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| 209| } | 210| |- } | | 210|+ | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 196| 196| if (keys instanceof Array) | 197| 197| { | 198| 198| for (let property in object) | 199| |- { | | 199|+ | 200| 200| if (keys.indexOf(property) > -1) | 201| 201| { | 202| 202| if (isTranslatableString(object[property])) | 206| 206| } | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| |- } | | 209|+ | 210| 210| } | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | 214| 214| else | 215| |- { | | 215|+ | 216| 216| for (let property in object) | 217| 217| { | 218| 218| if (property in keys) | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| 230| } | 231| |- } | 232| |-} | | 231|+ | | 232|+} | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 214| 214| else | 215| 215| { | 216| 216| for (let property in object) | 217| |- { | | 217|+ | 218| 218| if (property in keys) | 219| 219| { | 220| 220| if (isTranslatableString(object[property])) | 227| 227| } | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| |- } | | 230|+ | 231| 231| } | 232| 232| } binaries/data/mods/mod/gui/common/l10n.js | 95| » » g_TranslationsWithContext[context]·=·{} | | [NORMAL] JSHintBear: | | Missing semicolon. | | [NORMAL] ESLintBear (indent): | | Expected indentation of 3 tabs but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | 367| 367| | 368| 368| g_ModsEnabled.sort((folder1, folder2) => | 369| 369| dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 : | 370| |- dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | | 370|+ dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | 371| 371| | 372| 372| displayModList("modsEnabledList", g_ModsEnabled); | 373| 373| }
Link to build: https://jenkins.wildfiregames.com/job/differential/420/display/redirect
I suppose if you fix those, and think a lot about the struct thing this is close enough to a state I'm not going to disagree with too much...
But in case anyone forgot, read my replies in the corresponding forum topic carefully.
binaries/data/mods/mod/gui/modio/modio.js | ||
---|---|---|
89 | Lots of double spaces starting from here, see the linter. | |
213 | Not actually clearing the current state after a user action seems to just beg for bugs that will be hard to debug down the line. Yes, the current code does not have an issue, but the current structure will cause someone to introduce a bug, that nobody is going to notice, which is going to remain unnoticed for a while, and I wouldn't want to be the one who has to debug such an issue that could have been prevented before it was introduced. | |
binaries/data/mods/mod/gui/modio/modio.xml | ||
7 | I complained about having too inclusive includes for this more than thrice by now. I also expect this to break in interesting ways down the line, I wish whoever will have to debug the resulting mess a lot of fun, because I don't think that this is going to be obvious. I also hope that whoever is responsible for this mess will be involved in debugging said issues, and that this comment and similar ones have been forgotten by then. | |
binaries/data/mods/mod/gui/modmod/help/help.txt | ||
6 | This could mention the button this probably refers to. | |
8 | "The mod.io service is developed [...]" would probably be nicer to not have a sentence start with a lower case "word". Come and get some! | |
source/ps/ModIo.cpp | ||
33 | Having the ps/**.h separated from the others still keeps this slightly unfriendly. That said, moving towards proper full paths seems a lot nicer than moving to partial paths. | |
124 | ToJSVal would possibly be nicer, not that the whole thing seems that important. | |
193 | No, well, better than before. Doing this after the failure is ok, but seems bad, the original code would likely leave the state in some somewhat random state at least, this leaves it in a predictable state, which, while it might be hard to generate the corresponding key, is quite bad. | |
193 | Nah, not really, well still not, see the main comment IIRC. | |
source/ps/scripting/JSInterface_Mod.cpp | ||
20 | See elsewhere, still why and to be a little less obscure, WTH? |
binaries/data/mods/mod/gui/common/functions_msgbox.js | ||
---|---|---|
48 ↗ | (On Diff #6396) | Wanted to ask for the msgbox move to be committed separately as well so as to not clog the modio diff. |
7 ↗ | (On Diff #6443) | (Could be function messageBoxCallbackFunction(btnCode)) |
binaries/data/mods/mod/gui/modio/modio.js | ||
---|---|---|
79 | 1 KiB /1.2 GiB |
Final version.
- I just committed rP21754.
- I got rid of all the helper methods of that struct. It indeed removes a significant number of lines. However the tuple version makes it difficult to name elements (std::get<0> is quite ugly) and even first, second in the case of a pair is less clear than status and progress, so I didn't go all the way.
- Plus the comments I made.
binaries/data/mods/mod/gui/modio/modio.js | ||
---|---|---|
79 | You seem to be the only one with that opinion so far ? Personally both look equally good to me, as already said, and using sarcasm usually has opposite effects on me (and on most people). | |
binaries/data/mods/mod/gui/modio/modio.xml | ||
7 | More specific includes would be nice but right now we are doing this for all the GUI pages in both mods. I don't even know the syntax for single-file script includes in our GUI because we don't use it anywhere at all. So I'm going to open a ticket and we'll look into improving that. | |
source/ps/scripting/JSInterface_Mod.cpp | ||
20 | Source files and their corresponding header will always move together, no? It's just style here to my eyes. |
binaries/data/mods/mod/gui/modio/modio.js | ||
---|---|---|
79 | If the quantity of expressed opinion decides, then concerns on commits could be closed if the one raising the concern is in the minority. |
I suppose the naming helps with that yes. But the main issue was (and to some extent maybe still is) too much code for what is actually required.
binaries/data/mods/mod/gui/modio/modio.js | ||
---|---|---|
79 | If anything is making it harder to the reader then I'd vote for the split of the $number $unit thing from just having $number unit, but well. Now that we've deadlocked Itms in his sarcasm avoidance, we should figure out how to turn that into profit. And if review comments aren't evaluated based on whether they are valid and only based on who might or might not be expressing them, then you seem to have bad times ahead of you as you will hopefully learn those the hard way, but at least soon. | |
binaries/data/mods/mod/gui/modio/modio.xml | ||
7 | Those were changed in lots of places, and while they do make sense in most of them some (including to the original patch, and related other gui pages) do not make any. But I might have complained about that already in multiple places. | |
binaries/data/mods/mod/gui/page_modhelp.xml | ||
9 | Not quite sure if that is a good idea or actually wanted, but this one (and the other one below) could either be replaced in the public mod with that global.xml include, or have it here with a nearly empty global.xml include. (Can probably be ignored, because who cares?) | |
source/ps/ModIo.cpp | ||
120 | I still sort of disagree with having ENSUREs (or similar) have code that should always be called in them. Not that I expect someone to just #define ENSURE(p) them away, but given a few recent developments I want to be able to say "told you so" once that happens. | |
source/ps/ModIo.h | ||
28 | Why the newline? | |
source/ps/scripting/JSInterface_Mod.cpp | ||
148 | Will probably be a nice candidate for constexpr at some point, similar to other const things in the ModIo implementation. Could also be const char *, but that shouldn't really make any noticable difference here. Or unordered_map, since nothing seems to require any order here. |
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Default... Executing section Source... source/ps/ModIo.h | 55| enum·class·DownloadProgressStatus·{ | | [MAJOR] CPPCheckBear (syntaxError): | | syntax error Executing section JS... | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 194| 194| function translateObjectKeys(object, keys) | 195| 195| { | 196| 196| if (keys instanceof Array) | 197| |- { | | 197|+ | 198| 198| for (let property in object) | 199| 199| { | 200| 200| if (keys.indexOf(property) > -1) | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| 209| } | 210| |- } | | 210|+ | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 196| 196| if (keys instanceof Array) | 197| 197| { | 198| 198| for (let property in object) | 199| |- { | | 199|+ | 200| 200| if (keys.indexOf(property) > -1) | 201| 201| { | 202| 202| if (isTranslatableString(object[property])) | 206| 206| } | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| |- } | | 209|+ | 210| 210| } | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | 214| 214| else | 215| |- { | | 215|+ | 216| 216| for (let property in object) | 217| 217| { | 218| 218| if (property in keys) | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| 230| } | 231| |- } | 232| |-} | | 231|+ | | 232|+} | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 214| 214| else | 215| 215| { | 216| 216| for (let property in object) | 217| |- { | | 217|+ | 218| 218| if (property in keys) | 219| 219| { | 220| 220| if (isTranslatableString(object[property])) | 227| 227| } | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| |- } | | 230|+ | 231| 231| } | 232| 232| } | | [NORMAL] ESLintBear (indent): | | Expected indentation of 3 tabs but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | 367| 367| | 368| 368| g_ModsEnabled.sort((folder1, folder2) => | 369| 369| dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 : | 370| |- dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | | 370|+ dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | 371| 371| | 372| 372| displayModList("modsEnabledList", g_ModsEnabled); | 373| 373| }
Link to build: https://jenkins.wildfiregames.com/job/differential/425/display/redirect
source/ps/ModIo.cpp | ||
---|---|---|
120 | So what was wrong about the if (!check) ENSURE(0 && "message") that I had in a previous iteration? (I did consider it was just a remark, not a change request, but you explicitly said I had ignored the comment so here I am ? ) | |
source/ps/ModIo.h | ||
28 | I wanted to separate that from the 0 A.D. headers since it is almost a library include but not quite, but that's just silly. Fixed locally. | |
source/ps/scripting/JSInterface_Mod.cpp | ||
148 | Fixed with unordered_map locally. |
source/ps/ModIo.cpp | ||
---|---|---|
120 | It ended up inconsistent with the other one that got changed to ENSURE(foo && "bar"); above. |
source/ps/ModIo.cpp | ||
---|---|---|
120 | Ahhhhh. I would have preferred a comment on the other ENSURE then, but whatever. |
Build failure - The Moirai have given mortals hearts that can endure.
Linter detected issues: Executing section Default... Executing section Source... source/ps/ModIo.h | 54| enum·class·DownloadProgressStatus·{ | | [MAJOR] CPPCheckBear (syntaxError): | | syntax error Executing section JS... | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 194| 194| function translateObjectKeys(object, keys) | 195| 195| { | 196| 196| if (keys instanceof Array) | 197| |- { | | 197|+ | 198| 198| for (let property in object) | 199| 199| { | 200| 200| if (keys.indexOf(property) > -1) | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| 209| } | 210| |- } | | 210|+ | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 196| 196| if (keys instanceof Array) | 197| 197| { | 198| 198| for (let property in object) | 199| |- { | | 199|+ | 200| 200| if (keys.indexOf(property) > -1) | 201| 201| { | 202| 202| if (isTranslatableString(object[property])) | 206| 206| } | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| |- } | | 209|+ | 210| 210| } | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | 214| 214| else | 215| |- { | | 215|+ | 216| 216| for (let property in object) | 217| 217| { | 218| 218| if (property in keys) | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| 230| } | 231| |- } | 232| |-} | | 231|+ | | 232|+} | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 214| 214| else | 215| 215| { | 216| 216| for (let property in object) | 217| |- { | | 217|+ | 218| 218| if (property in keys) | 219| 219| { | 220| 220| if (isTranslatableString(object[property])) | 227| 227| } | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| |- } | | 230|+ | 231| 231| } | 232| 232| } | | [NORMAL] ESLintBear (indent): | | Expected indentation of 3 tabs but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | 367| 367| | 368| 368| g_ModsEnabled.sort((folder1, folder2) => | 369| 369| dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 : | 370| |- dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | | 370|+ dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | 371| 371| | 372| 372| displayModList("modsEnabledList", g_ModsEnabled); | 373| 373| }
Link to build: https://jenkins.wildfiregames.com/job/differential/427/display/redirect
I'm a bit surprised here. The error message is not helpful at all but I assume GCC on oldstable supports unordered_map without considering at() should have a const overload. So I think I'll be just a bit less optimal and use map. Unless I figure something out quickly enough...
Meh, use a normal map if that works. Not worth worrying to much about this (like it seems last) issue.
For the sake of completeness I'll still refer you to one of the old posts in the forum topic (or the other one, or was it some PM?).
Ah, it most likely fails because it cannot nicely convert the enum class into an int (which is the whole point of using an enum class), while not having a hash specialization for the enum class. Seems very much like something that's too complicated. Use a map.
Successful build - Chance fights ever on the side of the prudent.
Linter detected issues: Executing section Default... Executing section Source... source/ps/ModIo.h | 54| enum·class·DownloadProgressStatus·{ | | [MAJOR] CPPCheckBear (syntaxError): | | syntax error Executing section JS... | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'if' condition. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 194| 194| function translateObjectKeys(object, keys) | 195| 195| { | 196| 196| if (keys instanceof Array) | 197| |- { | | 197|+ | 198| 198| for (let property in object) | 199| 199| { | 200| 200| if (keys.indexOf(property) > -1) | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| 209| } | 210| |- } | | 210|+ | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 196| 196| if (keys instanceof Array) | 197| 197| { | 198| 198| for (let property in object) | 199| |- { | | 199|+ | 200| 200| if (keys.indexOf(property) > -1) | 201| 201| { | 202| 202| if (isTranslatableString(object[property])) | 206| 206| } | 207| 207| else if (object[property] instanceof Object) | 208| 208| translateObjectKeys(object[property], keys); | 209| |- } | | 209|+ | 210| 210| } | 211| 211| // If ‘keys’ is not an array, it is an object where keys are properties to | 212| 212| // translate and values are translation contexts to use for each key. | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'else'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 212| 212| // translate and values are translation contexts to use for each key. | 213| 213| // An empty value means no context. | 214| 214| else | 215| |- { | | 215|+ | 216| 216| for (let property in object) | 217| 217| { | 218| 218| if (property in keys) | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| 230| } | 231| |- } | 232| |-} | | 231|+ | | 232|+} | | [NORMAL] ESLintBear (curly): | | Unnecessary { after 'for-in'. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/common/l10n.js | 214| 214| else | 215| 215| { | 216| 216| for (let property in object) | 217| |- { | | 217|+ | 218| 218| if (property in keys) | 219| 219| { | 220| 220| if (isTranslatableString(object[property])) | 227| 227| } | 228| 228| else if (object[property] instanceof Object) | 229| 229| translateObjectKeys(object[property], keys); | 230| |- } | | 230|+ | 231| 231| } | 232| 232| } | | [NORMAL] ESLintBear (indent): | | Expected indentation of 3 tabs but found 2. |----| | /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | |++++| /mnt/data/jenkins-phabricator/workspace/differential/binaries/data/mods/mod/gui/modmod/modmod.js | 367| 367| | 368| 368| g_ModsEnabled.sort((folder1, folder2) => | 369| 369| dependencies[folder1].indexOf(g_Mods[folder2].name) != -1 ? 1 : | 370| |- dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | | 370|+ dependencies[folder2].indexOf(g_Mods[folder1].name) != -1 ? -1 : 0); | 371| 371| | 372| 372| displayModList("modsEnabledList", g_ModsEnabled); | 373| 373| }
Link to build: https://jenkins.wildfiregames.com/job/differential/429/display/redirect
For future reference, on about march 8th it was decided to add it to alpha 23 rather than have it a proof of concept not to be considered before A24 https://wildfiregames.com/forum/index.php?/topic/23009-moddb-mods-api/&page=2&tab=comments#comment-349922
source/ps/ModIo.cpp | ||
---|---|---|
349 | The message that satisfies message->msg == CURLMSG_DONE and message->easy_handle == m_Curl may not be skipped, because it is the only one that triggers the error code handling below. |
source/ps/ModIo.cpp | ||
---|---|---|
349 | Condition comes from https://gitlab.com/na-Itms/0ad/commit/cdc324f7f57bf3098c196c59a92718b1ab4d1e87
|