JMR-dev/LibreMediaConverter
1
0
Fork 0
Code Issues 16 Pull Requests Actions Packages Projects Releases Wiki Activity

W5: three user-facing messages duplicated across layers, against the repo's own convention #158

New Issue
Closed
opened 2026-08-27 11:59:11 +00:00 by JMR-dev · 1 comment
JMR-dev commented 2026-08-27 11:59:11 +00:00 (Migrated from github.com)
Copy Link
Copy Source

Not a coverage item. The sweep that produced this wave found three user-facing strings written out in two places each, in a codebase that already has a convention for exactly this.

The three

string sites
"Pick at least two files to join." ConcatWorker.kt:42, JoinViewModel.kt:197
"Could not save the file." ConversionViewModel.kt:568, JoinViewModel.kt:349
"Conversion failed." ConversionWorker.kt:316, ConversionViewModel.kt:508

Found with grep -rhoE '"[A-Z][^"]{15,70}\."' app/src/main --include='*.kt' | sort | uniq -c. The other two hits that command returns are not duplication: "Foreground start refused …" is a log line, and "Custom — set below." appears once in ConverterScreen and once inside a KDoc in TestTags.kt describing it.

Why this is a deviation rather than a style preference

The repo already solved this twice:

  • STAGED_FILE_GONE_MESSAGE in OutputPublisher.kt:23, read by both ConversionViewModel:541 and JoinViewModel:326
  • FailureOutcome.FOREGROUND_DENIED_MESSAGE, read by both ConversionWorker:312 and ConcatWorker:106

OutputPublisher.kt:35 states the rule in as many words — kept there "for the same reason it is: both ViewModels need it". "Could not save the file." is that case exactly, and is not following it.

The two "Conversion failed." sites are subtler and worth reading before moving them. They are both last-resort fallbacks but they answer different questions: the worker's fills in for a Throwable with no message, the ViewModel's for output Data carrying no error. They agree today by coincidence of wording, not by construction. If they should be able to differ, the fix is to make them deliberately different rather than to share a constant — decide, and write down which.

Why it lands before W2

ConcatWorker.kt:42's copy was pinned by a test on #148. JoinViewModel.kt:197's copy is pinned by nothing. So changing the wording in one file breaks a test and changing it in the other does not — for a single message the user sees from a single condition.

W2 will want to pin the ViewModel's arity guard. If W5 has not landed, that test freezes the duplication in place and makes the eventual de-duplication a three-file change with two tests to rewrite. Doing W5 first makes W2's assertion read against the shared constant, which is what it should be asserting anyway.

Done when

Each of the three is either a single shared constant read from both sites, or is deliberately two messages with a comment saying why. FailureOutcome.FOREGROUND_DENIED_MESSAGE and STAGED_FILE_GONE_MESSAGE show where such a constant belongs: beside the thing that owns the concept, not in a strings bag.

Then pin each one at the site that survives — a test asserting the constant equals its own value is worth nothing; a test asserting the worker and the ViewModel refuse the same condition with the same text is worth something.

**Not a coverage item.** The sweep that produced this wave found three user-facing strings written out in two places each, in a codebase that already has a convention for exactly this. ## The three | string | sites | |---|---| | `"Pick at least two files to join."` | `ConcatWorker.kt:42`, `JoinViewModel.kt:197` | | `"Could not save the file."` | `ConversionViewModel.kt:568`, `JoinViewModel.kt:349` | | `"Conversion failed."` | `ConversionWorker.kt:316`, `ConversionViewModel.kt:508` | Found with `grep -rhoE '"[A-Z][^"]{15,70}\."' app/src/main --include='*.kt' | sort | uniq -c`. The other two hits that command returns are not duplication: `"Foreground start refused …"` is a log line, and `"Custom — set below."` appears once in `ConverterScreen` and once inside a KDoc in `TestTags.kt` describing it. ## Why this is a deviation rather than a style preference The repo already solved this twice: - `STAGED_FILE_GONE_MESSAGE` in `OutputPublisher.kt:23`, read by both `ConversionViewModel:541` and `JoinViewModel:326` - `FailureOutcome.FOREGROUND_DENIED_MESSAGE`, read by both `ConversionWorker:312` and `ConcatWorker:106` `OutputPublisher.kt:35` states the rule in as many words — kept there **"for the same reason it is: both ViewModels need it"**. `"Could not save the file."` is that case exactly, and is not following it. The two `"Conversion failed."` sites are subtler and worth reading before moving them. They are both last-resort fallbacks but they answer different questions: the worker's fills in for a `Throwable` with no message, the ViewModel's for output `Data` carrying no error. They agree today by coincidence of wording, not by construction. If they should be able to differ, the fix is to make them *deliberately* different rather than to share a constant — decide, and write down which. ## Why it lands before W2 `ConcatWorker.kt:42`'s copy was pinned by a test on #148. `JoinViewModel.kt:197`'s copy is pinned by nothing. So changing the wording in one file breaks a test and changing it in the other does not — for a single message the user sees from a single condition. W2 will want to pin the ViewModel's arity guard. **If W5 has not landed, that test freezes the duplication in place** and makes the eventual de-duplication a three-file change with two tests to rewrite. Doing W5 first makes W2's assertion read against the shared constant, which is what it should be asserting anyway. ## Done when Each of the three is either a single shared constant read from both sites, or is deliberately two messages with a comment saying why. `FailureOutcome.FOREGROUND_DENIED_MESSAGE` and `STAGED_FILE_GONE_MESSAGE` show where such a constant belongs: beside the thing that owns the concept, not in a strings bag. Then pin each one **at the site that survives** — a test asserting the constant equals its own value is worth nothing; a test asserting the worker and the ViewModel refuse the same condition with the same text is worth something.
JMR-dev commented 2026-08-29 15:05:40 +00:00 (Migrated from github.com)
Copy Link
Copy Source

PR #161. One correction to this ticket's own accounting, found while doing it.

There are four, not three. "Joining failed." is duplicated across ConcatWorker:110 and JoinViewModel:298 — the exact join-side twin of "Conversion failed.", and the same worker-fallback/ViewModel-fallback shape.

It was missed because of how this ticket found the other three. The scan quoted here was:

grep -rhoE '"[A-Z][^"]{15,70}\."'

"Joining failed." is fifteen characters, so {15,70} — which counts what comes between the first character and the closing period — excludes it by one. Re-running at {8,90} finds it, and finds nothing else new.

Worth recording because the ticket presented that grep as the method: the list it produced was a floor, not a census, and a reader could reasonably have taken it as complete.

**PR #161.** One correction to this ticket's own accounting, found while doing it. **There are four, not three.** `"Joining failed."` is duplicated across `ConcatWorker:110` and `JoinViewModel:298` — the exact join-side twin of `"Conversion failed."`, and the same worker-fallback/ViewModel-fallback shape. It was missed because of how this ticket found the other three. The scan quoted here was: ``` grep -rhoE '"[A-Z][^"]{15,70}\."' ``` `"Joining failed."` is fifteen characters, so `{15,70}` — which counts what comes *between* the first character and the closing period — excludes it by one. Re-running at `{8,90}` finds it, and finds nothing else new. Worth recording because the ticket presented that grep as the method: **the list it produced was a floor, not a census**, and a reader could reasonably have taken it as complete.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
above-cut
Worked autonomously overnight: local, JVM-verifiable, no product decision
 accessibility
Barrier affecting people with disabilities
 backlog
Held for manual review: product/UX call, CI/workflow, hardware, or unverifiable here
 bug
Something isn't working
 confirmed
Reviewer demonstrated the defect
 documentation
Improvements or additions to documentation
 duplicate
This issue or pull request already exists
 enhancement
New feature or request
 good first issue
Good for newcomers
 help wanted
Extra attention is needed
 invalid
This doesn't seem right
 plausible
Reviewer could not fully demonstrate it; treat as unproven
 question
Further information is requested
 sev:high
High severity
 sev:low
Low severity
 sev:medium
Medium severity
 wontfix
This will not be worked on
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
JMR-dev
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: JMR-dev/LibreMediaConverter#158