Page MenuHomeWildfire Games
Differential D1029

mod.io support
ClosedPublic
Actions

Authored by Itms on Nov 12 2017, 7:59 AM.

Details

Reviewers
temple
• leper
Commits
rP21759: mod.io support.
Summary

I'm going to write an announcement, you already know what this is about!

Test Plan

Test all the things

Diff Detail

Repository
rP 0 A.D. Public Repository
Branch
upstream
Lint
Lint Skipped
Unit
Unit Tests Skipped
Build Status
Buildable 4380
Build 7685: Vulcan BuildJenkins
Build 7684: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
• leper added inline comments.Apr 17 2018, 8:36 PM
binaries/data/mods/mod/l10n/messages.json
49 ↗(On Diff #6396)

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
229

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

This revision now requires changes to proceed.Apr 17 2018, 8:36 PM
Itms updated this revision to Diff 6423.Apr 18 2018, 5:17 PM
Itms marked 7 inline comments as done.

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.

Owners added a subscriber: Restricted Owners Package.Apr 18 2018, 5:17 PM
Itms added inline comments.Apr 18 2018, 5:18 PM
binaries/data/mods/mod/l10n/messages.json
38 ↗(On Diff #6416)

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

Itms added a comment.Apr 18 2018, 5:30 PM

Fuck me I did forget the starts_with improvement. This I wanted to do, actually.

Vulcan added a comment.Apr 18 2018, 6:05 PM

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

Harbormaster completed remote builds in B5901: Diff 6423.Apr 18 2018, 6:05 PM
• leper added a comment.Apr 18 2018, 9:49 PM
In D1029#59608, @Itms wrote:

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.

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.

• leper requested changes to this revision.Apr 20 2018, 4:01 AM

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
312 ↗(On Diff #6396)

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
38 ↗(On Diff #6416)

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.

This revision now requires changes to proceed.Apr 20 2018, 4:01 AM
Itms updated this revision to Diff 6443.Apr 20 2018, 10:36 PM
Itms marked an inline comment as done.

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
312 ↗(On Diff #6396)

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

Vulcan added a comment.Apr 20 2018, 10:57 PM

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

Harbormaster completed remote builds in B5923: Diff 6443.Apr 20 2018, 10:57 PM
• leper requested changes to this revision.Apr 21 2018, 10:05 AM

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
88 ↗(On Diff #6443)

Lots of double spaces starting from here, see the linter.

212 ↗(On Diff #6443)

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
6 ↗(On Diff #6443)

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
5 ↗(On Diff #6443)

This could mention the button this probably refers to.

7 ↗(On Diff #6443)

"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–21

See elsewhere, still why and to be a little less obscure, WTH?

This revision now requires changes to proceed.Apr 21 2018, 10:05 AM
elexis added inline comments.Apr 21 2018, 11:00 AM
binaries/data/mods/mod/gui/common/functions_msgbox.js
7 ↗(On Diff #6443)

(Could be function messageBoxCallbackFunction(btnCode))

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.
The two messages.json diffs excluding the txt hunk and functions_global_object.js diff can be considered accepted by me too if needed.

elexis added inline comments.Apr 21 2018, 11:18 AM
binaries/data/mods/mod/gui/modio/modio.js
78 ↗(On Diff #6396)

1 KiB /1.2 GiB
1 MiB / 1.2 GIB,
1 / 1.2GiB
Because omitting units when writing numbers is common, making things inconsistent and requiring the reader to guess why the unit is omitted is nice.

Itms updated this revision to Diff 6451.Apr 22 2018, 2:19 PM
Itms marked 6 inline comments as done.

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
78 ↗(On Diff #6396)

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
6 ↗(On Diff #6443)

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–21

Source files and their corresponding header will always move together, no? It's just style here to my eyes.

elexis added inline comments.Apr 22 2018, 3:14 PM
binaries/data/mods/mod/gui/modio/modio.js
78 ↗(On Diff #6396)

If the quantity of expressed opinion decides, then concerns on commits could be closed if the one raising the concern is in the minority.

• leper added a comment.Apr 22 2018, 3:48 PM
In D1029#59924, @Itms wrote:
  • 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.

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
78 ↗(On Diff #6396)

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
6 ↗(On Diff #6443)

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
8 ↗(On Diff #6451)

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
215

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.

Vulcan added a comment.Apr 22 2018, 3:57 PM

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

Harbormaster completed remote builds in B5933: Diff 6451.Apr 22 2018, 3:58 PM
Itms marked 2 inline comments as done.Apr 22 2018, 5:46 PM
Itms added inline comments.
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
215

Fixed with unordered_map locally.

• leper added inline comments.Apr 22 2018, 5:52 PM
source/ps/ModIo.cpp
120

It ended up inconsistent with the other one that got changed to ENSURE(foo && "bar"); above.

Itms marked 3 inline comments as done.Apr 22 2018, 6:10 PM
Itms added inline comments.
source/ps/ModIo.cpp
120

Ahhhhh. I would have preferred a comment on the other ENSURE then, but whatever.

Itms updated this revision to Diff 6453.Apr 22 2018, 6:14 PM

And one more plate for Jenkins.

Vulcan added a comment.Apr 22 2018, 6:16 PM

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

Harbormaster failed remote builds in B5937: Diff 6453!Apr 22 2018, 6:16 PM
Itms added a comment.Apr 22 2018, 6:36 PM

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

• leper added a comment.Apr 22 2018, 6:43 PM

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

• leper added a comment.Apr 22 2018, 6:47 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.

Itms updated this revision to Diff 6455.Apr 22 2018, 6:52 PM

You mean the message about how to credit you in the commit message I suppose?

• leper added a comment.Apr 22 2018, 7:08 PM

The part about not indicating something that I don't.

Vulcan added a comment.Apr 22 2018, 7:34 PM

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

Harbormaster completed remote builds in B5939: Diff 6455.Apr 22 2018, 7:34 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 22 2018, 8:14 PM
Closed by commit rP21759: mod.io support. (authored by Itms). · Explain Why
This revision was automatically updated to reflect the committed changes.
elexis added a comment.May 17 2018, 3:36 PM

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

elexis mentioned this in rP21890: Inform users that they are subject to the mod.io Terms and Conditions and….Sep 22 2018, 7:01 PM
elexis changed the visibility from "Custom Policy" to "Public (No Login Required)".Sep 22 2018, 11:38 PM
elexis added inline comments.Sep 23 2018, 1:29 AM
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.
If the server is offline, didn't authenticate or such, we will get a misleading failure to parse the empty response as JSON instead of the proper error handling below.

elexis added inline comments.Sep 23 2018, 1:33 AM
source/ps/ModIo.cpp
349

Condition comes from https://gitlab.com/na-Itms/0ad/commit/cdc324f7f57bf3098c196c59a92718b1ab4d1e87

Asynchronous mod downloads, fixes #4

elexis mentioned this in rP21891: Fix mod.io curl error handling, following rP21759 / D1029 / https://gitlab..Sep 23 2018, 2:08 AM

Revision Contents
Changeset List

PathSize
binaries/
data/
config/
5 lines
mods/
mod/
gui/
modmod/
65 lines
81 lines
4 lines
9 lines
source/
ps/
GameSetup/
3 lines
70 lines
447 lines
scripting/
3 lines
41 lines

Diff 5171

View Options

binaries/data/config/default.cfg

Loading...
View Options

binaries/data/mods/mod/gui/modmod/modio.js

Loading...
View Options

binaries/data/mods/mod/gui/modmod/modio.xml

Loading...
View Options

binaries/data/mods/mod/gui/modmod/modmod.xml

Loading...
View Options

binaries/data/mods/mod/gui/page_modio.xml

Loading...
View Options

source/ps/GameSetup/GameSetup.cpp

Loading...
View Options

source/ps/ModIo.h

Loading...
View Options

source/ps/ModIo.cpp

Loading...
View Options

source/ps/scripting/JSInterface_Mod.h

Loading...
View Options

source/ps/scripting/JSInterface_Mod.cpp

Loading...
Wildfire Games · Phabricator