Leave unsupported rows unticked in mod config export picker - #656
Conversation
Translation statusen-US is the source and carries 908 keys.
Drafted values are the machine-drafted ones still waiting for a native review, listed per locale in |
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed at 1f079c8. The move leaves the import side exactly as it was: dev's tip and this head give the same answers to the 35 made-up packs, to 138 keys sent through the pack parser and the apply channel, and to 200,000 generated keys. On a made-up folder of 25 files, the picker starts the 21 names a pack cannot carry unticked, each with the right caption, and the count follows the ticks. Once the one file whose bytes are not UTF-8 is unticked as well, the export carries exactly the files left ticked. Gates and CI pass.
One thing blocks, and it is about the trust boundary #572 built.
Blocking
- The rule moved without its comments, and its new test file holds half of it. The four comments inside the old function did not come along, including the one that says what the backslash and empty segment checks are for. The backslash check is the line that keeps a stranger's key inside ModConfig on Windows. Without it,
..\clientsettings.jsonpasses the key rule, the pack parser and the apply request,joinon Windows resolves it to the Installation's ownclientsettings.json, andassertManagedPathgrants that folder (pathPolicy.ts:107). Removing that check leaves all 5031 tests green. So does removing the 512 ceiling or raising it by one, raising the 255 one to 256, or dropping the control range or DEL fromWINDOWS_FORBIDDEN_CHARACTER. Five keys in the refused list oftests/domain/mods/modConfigs.test.tscatch all six:..\\clientsettings.json,"a".repeat(251) + ".json"(a 256 character name), a 513 character key whose segments all stay under 255,a\u0001b.jsonanda\u007fb.json. The.and..checks need nothing, since the trailing dot rule refuses both anyway. Last, the privateassertSafeFileNamein the new module is a copy ofsrc/ipc/validation.ts:304under the same name. A line saying so would tell whoever tightens one of them that a second one exists.
Not blocking
- A name Windows refuses can still be ticked. Tick
CON.jsonand the count goes from 1 to 2, the export is asked for it, and the host refuses the whole pack under its name. #653 settled the same case on the import side by disabling the box of a row the host will refuse (ImportModConfigsDialog.tsx:209). Doing that forbad-namerows keeps the count honest and makes that refusal unreachable from the picker. Acollidesrow has to stay tickable, since one file of the pair travels fine. - Links still get no row. The listing the picker reads already names them (
listing.linked), the export never carries one, and nothing in the picker says so: in the made-up folder, neitherlinked.jsonnor a linked folder shows up. A disabled row with a caption would finish "rows the walk already knows a pack cannot carry are shown with why". A follow-up issue would do as well. - Wording. No other string in en-US says "dialog", and the window the player has to reopen is titled "Mod configs to export". "Untick it in the list of mod configs to export" names what they will see. In French, "la boîte d'export" reads oddly on its own, and "Décochez-le dans la liste des configs à exporter" says the same thing. And when nothing in the folder can travel, the picker now opens on "This modpack will carry 0 mod config files from this Installation. Untick the ones it should not carry.", which asks for an untick with every box already clear.
- The French test restates the locale file. "names unticking the row in French refusal sentences" (
exportModConfigsDialog.test.tsx:285) renders nothing: it reads three fr-FR values throughi18n.tand compares them with the same text. The in-use warning test also pins French (versionsListVersions.test.tsx:189), but it renders the page to show that no English connector leaks in. Here, a translator who improves one of these sentences on Weblate turns the Weblate pull request red. Matching/Décochez/still fails when dev's sentence comes back, which is what the test is for. - Outside this diff. A file whose name is not UTF-8 makes the whole listing fail, so the include box is disabled, its tooltip saying the folder could not be read, and there is no row to untick. A folder holding
fine.jsonand a file namedf, byte0xFF,.jsonanswersmod-config-unreadable, on dev and here: the walk gets the name back with U+FFFD in it, and thelstaton the rebuilt path finds nothing. Same family as #590, and I found no issue for it.
Validation
Import side, dev against this head. Both regexes are the same bytes as dev's, and the inlined copies of assertString and assertSafeFileName keep their conditions and their messages (validation.ts:160 and :304). The third round's probes of #572 (43 cases, among them the 35 made-up packs and the key battery) print the same lines on dev's tip and on this head, apart from a random sample of write-file-atomic's temp names and two memory readings. The 35 packs and the battery also answer what they answered in that round. 138 keys (that battery, 56 more at the edges of every condition the move copied, and nine values that are not strings) get the same answer from assertModConfigKey, from parseModpackSettings and from the apply channel on both sides. 200,000 seeded keys give the same verdicts, line for line.
Export side, real handlers, made-up folder. 25 files and 2 links: 3 plain configs, 17 names the key rule refuses (CON.json, nul/inner.json, COM¹.json, clock$.json, a trailing dot and a trailing space in a file name and in a folder name, the seven forbidden characters, a control character, a 527 character key), 2 case pairs (AutoMap.json and automap.json, Sub/x.json and sub/x.json) and latin1.json. The picker's rule ticks the 3 and latin1.json, whose bytes the listing never reads, and that export is refused as not-utf8 for latin1.json. With it unticked, the export carries the 3 byte for byte with matching digests, and the pack EXPORT_MODPACK writes holds exactly those keys. Any refused name ticked on top is refused under its own name. One file of a case pair is carried, and both are refused as collides under the second. On dev the same folder opens with all 25 ticked and stops at COM¹.json.
The picker, driven through the page. With those 25 names, the 21 rows start clear with the caption their diagnosis calls for, the count says 4 and the export is asked for those 4. Ticking CON.json, then AutoMap.json, moves the count to 2, then 3, the refusal shown names unticking, and the next open starts from the defaults again. In French the captions read as written and the count reads "2 fichiers", "1 fichier", "0 fichier".
Mutations. 42 changes to the new module, the picker and the sentences. The PR's two test files catch 29 of them, and the mod config feature files with the i18n suite (13 files, 182 tests) catch 32. Of the ten left, the . and .. checks change no answer over the 200,000 keys, and the empty key and type checks only change the error message on the keys tried. The other six change answers, survive the whole suite, and are the ones in the first item. The five keys suggested there catch all six.
Translations. Two new keys and three changed sentences, in en-US and fr-FR only, the new keys between the same neighbours in both files. The three changed keys exist in no other locale, so nothing goes stale, and drafted.json is untouched. npm run i18n:status -- --write docs/contribute/translation-status.md answers "already up to date", the Weblate writer round trip gives identical files for all 14 locales, and tests/i18n passes.
Gates and CI. typecheck, lint:ci (0 errors, the 12 known warnings) and format:check pass. test:coverage passes: 280 files, 5031 passed, 4 skipped, 94.45 / 90.66 / 95.31 / 96.31. The first run here failed one unrelated test, "compresses a source past the old 2 GiB archive ceiling", for lack of free space in the temp folder, and it passes with TMPDIR pointed at a folder with room. The new module has every line covered and 37 of 38 branches, the missing one being the ?? "" after a split. CI is green on this head, with the macOS build skipped. The branch sits on dev's tip, so there is nothing to merge.
Not run. The game, Windows and macOS by hand, and a headless build: the only layout change is a caption line the import dialog already uses.
Changes requested, for the first item only. The fix does what #587 asks, and the move changes no answer on the import side. But the rule it moves is the one that keeps a stranger's key inside ModConfig, and its backslash check, which is what does that on Windows, can be removed today without a single test failing. The five keys above pin it. The rest can follow.
|
Addressed Pixnop's review feedback in
The non-UTF-8 filename listing failure noted outside the changed lines remains separate: the current listing contract exposes names as strings, so handling the original raw path bytes needs a separate change. |
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed again at 344446b. The blocking point is settled: the four comments are back inside assertModConfigKey, and the new case in tests/domain/mods/modConfigs.test.ts fails against each of the six changes the whole suite let through last time. The other points were all taken up, apart from the file name that is not UTF-8. What stands now is the rebase over #657, which lands first.
First round
- Comments. Settled, with one clause to drop. The note on the private
assertSafeFileNamesays it is the one insrc/ipc/validation.ts"with the additional NUL-byte refusal this key needs". That helper refuses NUL too, throughassertString(validation.ts:161), so the two copies are one rule, and whoever comes to align them will look for a difference that is not there. - Tests for the moved rule. Settled. "keeps traversal, whole-key and per-segment Windows bounds pinned" fails against all six: the backslash check removed, the 512 ceiling removed or raised by one, 255 raised to 256, the control range dropped, DEL allowed. Its 513 character key leans on two other edges, though, since its first folder is 255 characters long and its file is a bare
.json. Remove the ceiling and also lower the segment limit to 254, or refuse an empty stem, and that test still passes, where the first round's key of three short segments fails both times. Today it pins the ceiling as it should. - A name Windows refuses can be ticked. Settled. In the made-up folder the 17 refused names are disabled rows. Clicking the
CON.jsonbox or its label changes neither the ticks nor the count, and a colliding row still ticks. - Links get no row. Settled:
linked.jsonandlinkdirare disabled rows with a caption, in both languages. It is the only one of the three captions that ends in a full stop. A folder holding nothing but links still shows no include box, sinceconfigCountcounts configs alone, so those links go unexplained; a follow-up at most. - Wording. Settled in both languages, and with nothing ticked the picker now says "No mod config files are selected to travel in this modpack."
- The French test. Settled: it renders each of the three refusals through the page. It matches "Décochez-le dans la liste des configs à exporter", so rewording the end of that phrase on Weblate still turns that pull request red.
Décochezalone would not. - A file name that is not UTF-8. Still open, and outside this diff. I found no issue for it, and one would keep it from getting lost.
The new commit
It changes nothing beyond the answers, and three small things trail behind. The comment above ExportModConfigsDialog still says the rows a pack cannot carry "start unticked", while a bad-name row now cannot be ticked at all and links have rows of their own. Nothing checks that a link row's label names its box: drop its htmlFor and the whole suite passes, because the new test finds the box by id, so looking it up by label (getByLabelText(/^linked\.json/)) would cover it. And the description and the second commit, which has no body, still describe the first round, so a squash message built from them would miss the disabled rows, the link rows and the zero sentence.
Rebase over #657
#657 lands first. The two conflict in src/ipc/handlers/modConfigs.ts, both locale files and translation-status.md, whichever goes first, and the overlap is more than textual. diagnoseModConfigKey calls every failure of assertModConfigKey bad-name, so once #657's class /[\p{Cf}\p{Default_Ignorable_Code_Point}]/u is in the rule, a file named config + U+034F + .json becomes a disabled row drawn as config.json under "Windows does not accept this file name", which is not the reason. The rebase is expected to:
- Carry that class into
src/domain/mods/modConfigs.tsand export it forcollectModConfigs, withshowDefaultIgnorables, which the picker needs for the next point. - Give the picker a
hidden-characterrow whose caption spells the name with<U+XXXX>, the way #657's export message does. It is disabled like a bad-name row and tested before it, ascollectModConfigsdoes. - Bring #657's
exportModpackConfigHiddenCharacterto the "Untick it in the list of mod configs to export" wording, in en-US and fr-FR. - Keep #657's tests passing through the moved function. They reach it through the handler's re-export, so they fail if the class is left behind.
The rest is mechanical: #657's new sentence goes between the two this PR rewrites, and the status page is regenerated.
Validation
Mutations. The first round's 42 changes, plus 18 on what the new commit changed. The PR's two test files now catch 50 of the 60, and the mod config feature files with the i18n suite (13 files, 187 tests) catch 54. The six left survive the whole suite. Four are last round's: removing the . or .. check changes no answer over the 138 keys and 200,000 generated ones, and the empty key and type checks only change the error message. The other two are the link label above and a fr-FR zero sentence left in English, which no test in the repo checks for.
Import side. The rule's code is the first round's; only its comments changed. The 138 keys through assertModConfigKey, the pack parser and the apply channel, the 200,000 generated keys and the third round's probes of #572 (43 cases, the 35 made-up packs among them) print the same lines on dev's tip and on this head, apart from write-file-atomic's temp names and a memory reading.
The picker, through the page. The made-up folder of 25 files and 2 links gives 27 rows: the same 4 ticked, 19 disabled (17 refused names and both links) and 4 colliding rows left tickable, each with the caption its diagnosis calls for. Clicking a disabled box, its label or a link row changes nothing. Ticking AutoMap.json moves the count to 2, the export is asked for exactly those two, the refusal names unticking, and the next open starts from the defaults. A folder with nothing ticked by default gets the new sentence and a disabled button until a row is ticked. In French the captions, the link caption and the count at 2, 1 and 0 read as written. On the host side the same folder answers as in the first round, line for line.
Translations. Two new keys and three reworded sentences, in en-US and fr-FR only, each new key between the same neighbours in both files. drafted.json is untouched, npm run i18n:status -- --write docs/contribute/translation-status.md answers "already up to date", the Weblate writer round trip gives identical files for all 14 locales, and tests/i18n passes.
Gates and CI. typecheck, lint:ci (0 errors, the 12 known warnings) and format:check pass. test:coverage passes: 280 files, 5038 passed, 4 skipped, 94.45 / 90.66 / 95.31 / 96.31. CI is green on this head, with the macOS build skipped. Since the first round dev has moved by one commit (#662, Optimum files only), and the branch merges into it without a conflict.
Not run. The game, Windows and macOS by hand, a packaged build, and the rebase itself.
Changes requested, for the rebase over #657 only. The first round's blocking point is settled: the backslash check and both ceilings are pinned, and the comments say what they guard. Of the smaller points, the NUL clause is the one worth fixing in the same push.
In the export picker, files a modpack cannot carry (names Windows refuses, invisible Unicode characters, or names colliding in letter case) and symbolic links are shown with explanations under their names. Disabled rows prevent selecting names Windows refuses or names containing hidden characters, as well as symlinks and linked directories. Colliding names start unticked but remain selectable. When no configs are selected, the dialog displays a zero sentence and disables the export button. Move key validation, collision diagnosis, and hidden character utilities to a shared domain module re-exported by the IPC handler. In the export dialog, render dedicated disabled rows for links and hidden-character files, spelling invisible Unicode code points with hex tags. Update export refusal notifications in en-US and fr-FR to explicitly name unticking the row as an alternative to renaming or stopping, and update translation status. Fixes #587
344446b to
79229ef
Compare
|
Rebased onto
|
Pixnop
left a comment
There was a problem hiding this comment.
Reviewed again at 79229ef: one commit on dev's ab5ad06, which holds #657. The four rebase items are done, the probes of #657 and #572 answer through the moved rule as they do on dev, and on 42,260 file names put on disk the picker and the export never disagree. Nothing blocks. The rebase brought four small things with it, under "New".
Round two
- The class in the shared module. Settled.
HIDDEN_UNICODE_NAME_CHARACTERandshowDefaultIgnorablesare exported fromsrc/domain/mods/modConfigs.tsbyte for byte as #657 wrote them, the rule tests the class on every segment,collectModConfigsimports both, and the handler'sassertModConfigKeyis the shared function itself. - The hidden-character row. Settled: disabled, diagnosed before bad-name as
collectModConfigsdoes, its caption spelled with<U+XXXX>. Nine made-up names (one per kind, one also refused by Windows, two a case pair) give nine disabled, unticked rows, and each caption is exactly the name the export refuses when that row is ticked anyway. - #657's sentence. Settled in en-US and fr-FR, and the page renders both with the spelled name.
- #657's tests. Settled.
tests/ipc/modConfigs.test.tsis untouched and passes, reaching the rule through the handler. Drop the class from the shared rule and all four of #657's tests fail; drop the re-export and the five tests that import the rule through the handler fail. - The rest. Settled. The NUL clause is gone and the component comment describes the disabled, link and colliding rows. The link test finds its box by label, so dropping that
htmlForfails it. The single commit's message covers the disabled, hidden-character and link rows and the zero sentence. The file name that is not UTF-8 has #669 now.
#657 through the move
Dev's tip against this head, with the earlier rounds' probes unchanged. The sweep of every Default_Ignorable_Code_Point, Bidi_Control, Cf and variation selector, at ten positions through assertModConfigKey, prints the same lines on both: none of the 4,174 default-ignorable or bidi code points passes anywhere, and the 32 Cf that are not default-ignorable are refused too. #572's key battery and 35 packs, and the 43 probes of its third round, print the same lines apart from write-file-atomic's random temp names. The 138 keys through the rule, the pack parser and the apply channel, and the 200,000 generated keys, are identical.
On the export side, round two's made-up folder with the nine hidden names added (34 files, 2 links) answers the same on both sides, file by file. Then one real file for each of 4,226 code points at each of the ten positions: the listing and the export give the same answers on dev and here, and each of the 38,034 names the listing shows (all but those ending after .json) gets the picker diagnosis and caption that match the export's answer.
New
- The hidden-character caption says what, not why. The row for
config+ U+034F +.jsonreadsconfig.json, withconfig<U+034F>.jsonunder it. The other three captions give a reason in words, and this one reads the same in French as in English. Since the row cannot be ticked, the player never reaches #657's message either, so nothing on screen says the name holds an invisible character. A key along the lines of "Holds invisible characters: {{name}}", in both languages, would match the other rows. Not blocking: the row is disabled and the spelling points at the character. - Four re-exports nobody imports. The handler re-exports five names from the shared module, and imports
isValidModConfigKeyandMAX_MOD_CONFIG_KEY_LENGTHonly for that. OnlyassertModConfigKeyis reached through it, bytests/ipc/modConfigs.test.ts. Importing the three names the handler uses and re-exportingassertModConfigKeyalone, as round two's version did, passes typecheck, lint, format and the whole suite. - A comment lost its subject in the merge. Above
assertModConfigKey, "Those are refused here rather than discovered on a Windows machine by everybody else" now follows #657's bidi sentence, so it reads as if bidi controls were a Windows matter. Swapping the two sentences fixes it. OndiagnoseModConfigKey, "(spelling them with code points)" describes the caller; the function spells nothing. - The skip test. "exports with no configs at all when the player says so" stops calling
openThePickerand repeats its clicks inline, under a new comment saying the action bar's export button "does not open the dialog at all". The test ticks the box first and then clicks the skip button on the dialog that very button opened, so the comment only holds with the box clear. Restoring the helper, or saying "with the box clear", would do.
Validation
Mutations. Round two's 60, rewritten where the code moved, plus 21: the class and its place in the rule, the hidden-character diagnosis, row, caption and default, the spelling, the host's refusal, the re-exports, #657's sentence in both languages and the config row's label. The PR's two test files catch 67, and the mod config feature files with the i18n suite (13 files, 198 tests) catch 74. The seven left survive the whole suite. Five are round two's: the . and .. checks change no answer over the 138 keys and the 200,000 generated ones, the empty key and type checks only change the error message, and nothing checks that the fr-FR zero sentence is French. The sixth is the re-export cut from point 2. The last belongs to #657 and is the same on dev: with the class reduced to Default_Ignorable_Code_Point, the 32 Cf that are not default-ignorable pass again, U+FFF9 to U+FFFB among them, and a file named U+0600 followed by price.json exports. Now that the class has a test file of its own, one key holding U+FFF9 in tests/domain/mods/modConfigs.test.ts would pin it.
The picker, through the page. 36 rows: the same 4 ticked as in round two, 28 disabled (17 refused names, 9 hidden names, 2 links) and 4 colliding rows left tickable. The count says 4 and the export is asked for those 4. Clicking the box or the label of a hidden row changes nothing, and a folder holding only hidden names gets the zero sentence and a disabled button. The 25 rows of round two diagnose as they did then.
Translations. Four new keys and four reworded sentences, in en-US and fr-FR only, each between the same neighbours in both files. drafted.json is untouched, npm run i18n:status -- --write docs/contribute/translation-status.md answers "already up to date", the Weblate writer round trip gives identical files for all 14 locales, and tests/i18n passes.
Gates and CI. typecheck, lint:ci (0 errors, the 12 known warnings) and format:check pass. test:coverage passes: 281 files, 5070 passed, 4 skipped, 94.51 / 90.73 / 95.36 / 96.38. CI is green on this head, with the macOS build skipped. The branch sits on dev's tip.
Not run. The game, Windows and macOS by hand, and a packaged build.
Approved. The four points under "New" can go in this push or after it; the caption is the one a player would see.
Summary
In the mod config export picker, files a modpack cannot carry (names Windows refuses, invisible Unicode characters, or names colliding in letter case) and symbolic links are presented with subtitles explaining why under their names. Disabled rows prevent selecting names Windows refuses or names containing hidden characters, as well as symlinks and linked directories. Colliding names start unticked but remain selectable. When no configs are selected, the dialog displays a zero sentence and disables the export button.
This change moves key validation, collision diagnosis, and hidden character utilities into a shared domain module re-exported by the IPC handler. In the export dialog, dedicated disabled rows render for links and hidden-character files, spelling invisible Unicode code points with hex tags. Export refusal notifications in en-US and fr-FR explicitly name unticking the row as an alternative to renaming or stopping, and translation status is updated.
Type
Checklist
dev, notmain.npm run typecheckpasses.npm run lint:cipasses.npm run format:checkpasses.npm run test:coveragepasses, coverage at or above the floor invitest.config.ts.npm run build:unpackpasses.Testing
Ran the required adapter suite:
npm run typecheck: clean exit (code 0) across all tsconfigs.npm run lint:ci: clean exit (code 0, 0 errors, 12 pre-existing warnings).npm run format:check: clean exit (code 0, all files formatted).npm run test:coverage: clean exit (code 0). Added unit tests intests/domain/mods/modConfigs.test.ts(12 tests) and component tests intests/renderer-dom/exportModConfigsDialog.test.tsx(18 tests) verifying default checkboxes, collision subtitles, bad name subtitles, hidden characters, link row labels via htmlFor, zero sentence, and French locale translations.npm run build:unpack: clean exit (code 0).node scripts/i18n-status.js: verified 0 missing and 0 stale strings for fr-FR.Related issues
Fixes #587