Skip to content

Fix document name extraction from document properties#630

Merged
thestinger merged 1 commit intoGrapheneOS:mainfrom
thomasbuilds:patch-1
Apr 29, 2026
Merged

Fix document name extraction from document properties#630
thestinger merged 1 commit intoGrapheneOS:mainfrom
thomasbuilds:patch-1

Conversation

@thomasbuilds
Copy link
Copy Markdown
Contributor

@thomasbuilds thomasbuilds commented Apr 26, 2026

This PR fixes getCurrentDocumentName() in PdfViewer.java, which produced wrong or empty document names (used for the toolbar title and the Save-As default filename).

What was wrong

  • It parsed the formatted, localized strings shown in the Document Properties dialog ("File name:\n…", "Title:\n…"). The labels come from R.string.file_name / R.string.title in DocumentPropertiesLoader, so any future translation would silently break it.
  • On top of that, the original prefixes were also wrong: "File name:" was missing the trailing newline so replace() left a leading \n in the result, and "Title:-" never matched anything, so the title fallback was dead code.
  • The filename selection used a brittle length > 2 check that could reject valid short filenames.

What this changes

  • Resolves the document name directly from the raw Map<DocumentProperty, String> produced by DocumentPropertiesLoader, using the stable enum keys DocumentProperty.FileName (falling back to DocumentProperty.Title when its value isn't DEFAULT_VALUE). No string parsing of localized output.
  • DocumentPropertiesLoader now returns a DocumentPropertiesResult carrying both the formatted List<CharSequence> used by the existing dialog and the pre-resolved documentName. The dialog UI is unchanged.
  • PdfViewer caches the resolved name in a new mDocumentName field; getCurrentDocumentName() becomes a trivial accessor. The field is reset alongside mDocumentProperties on document change and loader reset.

Net effect: name resolution is fully decoupled from UI strings, so future translations of the property labels won't break the toolbar/Save-As behavior.

@RankoR RankoR self-requested a review April 27, 2026 19:11
@muhomorr
Copy link
Copy Markdown
Member

getCurrentDocumentName() has hidden dependency on UI string formatting in DocumentPropertiesLoader. This code will break after addition of UI string translations. Deeper changes are needed to address this issue fully.

@thomasbuilds
Copy link
Copy Markdown
Contributor Author

Yeah you're right the prefixes "File name:\n" / "Title:\n" are built from R.string.file_name / R.string.title in DocumentPropertiesLoader.kt, so once those strings get translated, the parser silently returns "" and both the Save-As default name and the toolbar title break. This PR only fixes the :- typo, the missing \n in the File name: prefix, the leading-newline bug, and the brittle length > 2 check, the coupling to localized strings is still there.

To fix it properly I'm thinking:

  1. Add a small DocumentPropertiesResult data class carrying both the formatted List<CharSequence> (for the existing dialog) and a pre-resolved documentName: String.
  2. Have DocumentPropertiesLoader resolve the name directly from the raw Map<DocumentProperty, String> using the stable enum keys; DocumentProperty.FileName first (always a real OS filename, never "-"), falling back to DocumentProperty.Title when its value isn't DEFAULT_VALUE. No string parsing of localized output.
  3. Change DocumentPropertiesAsyncTaskLoader to return DocumentPropertiesResult.
  4. In PdfViewer, cache the resolved name alongside mDocumentProperties and reset it on document change / loader reset. getCurrentDocumentName() becomes a trivial accessor.

That way name resolution is entirely decoupled from UI strings, so future translations can't break it.
No change to the Document Properties dialog.

Happy to update the PR with this approach if that works for you.

@muhomorr
Copy link
Copy Markdown
Member

Yes, the approach that you've described should be fine.

@ggtlvkma356
Copy link
Copy Markdown
Contributor

I also noticed the Title:- match is essentially dead code, nice you have a fix for it.

getCurrentDocumentName() previously matched against the formatted strings
shown in the Document Properties dialog ("File name:\n...", "Title:\n...").
Those labels come from R.string.file_name and R.string.title via
DocumentPropertiesLoader, so once the strings get translated the parser
silently returns "" and both the Save As default name and the toolbar
title break. The original code also had several smaller bugs: the
"Title:-" prefix never matched, "File name:" was missing the trailing
newline so replace() left a leading \n in the result, and the brittle
length > 2 check could reject valid short filenames.

Resolve the document name from the raw Map<DocumentProperty, String>
in DocumentPropertiesLoader using the stable enum keys (FileName,
falling back to Title when its value isn't DEFAULT_VALUE). The loader
now returns a DocumentPropertiesResult carrying both the formatted list
used by the dialog and the pre-resolved document name. PdfViewer caches
the name in mDocumentName and getCurrentDocumentName() becomes a
trivial accessor; the field is reset alongside mDocumentProperties on
document change and loader reset.
@thestinger thestinger merged commit 21716ea into GrapheneOS:main Apr 29, 2026
2 checks passed
ggtlvkma356 added a commit to ggtlvkma356/PdfViewer that referenced this pull request Apr 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants