Open every format the core renders, and ask it what can be edited - #548
Open
andiwand wants to merge 4 commits into
Open
Open every format the core renders, and ask it what can be edited#548andiwand wants to merge 4 commits into
andiwand wants to merge 4 commits into
Conversation
odrcore v6 keeps reference html output for doc, ppt, xls and pptx next to the ones the app already claimed, so the "TODO: enable pptx too" that sat in CoreLoader was the only thing keeping presentations out - and the legacy binary formats reached the core only by accident, through the doc mime type that was listed before ppt and xls ever were. What the loader claims is now one list: opendocument, the whole ooxml family, the three legacy microsoft formats and pdf. Prefixes, so the templates and the macro enabled variants come along - the latter are spelled vnd.ms-word/vnd.ms-excel/vnd.ms-powerpoint rather than under the openxmlformats prefix, which is why those appear without their subtypes. Text, csv and images stay out: the core renders them too, but RawLoader is what gives them their player, and it only gets a turn when the core loader says no. The STRICT_CATCH filter follows, so the system offers us pptx/ppt/xls. Two things fall out of it. The doOoxml flag is gone - it has been passed true since ooxml stopped being experimental and now gates nothing. And the ".doc-files are not real documents" special case becomes a check for all three legacy types: the core does parse them now, but declares them neither editable nor savable, so keeping one open only buys a second parse. pdf changes which viewer a failed upload falls back to, so that stays explicit: google docs still gets the pdf, the microsoft viewer keeps everything else the core knows. Verified on device - .pptx, .ppt, .doc and .xls all render through the core, with CoreTest covering each of them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eTZB8rnh9PrdNn1YZexue
doc, ppt and xls now reach CoreLoader, and prepareMenu offered Edit for every CORE result except ODS and PDF - so the legacy formats got a button that could only ever end in a failed save, because host() would not keep a document open for them. The list of exceptions was never ours to keep. odrcore answers this itself: Document.isEditable()/isSavable() is false for the three legacy binary formats, for ooxml spreadsheets and presentations, and for every spreadsheet including odf - the last being the core's own TODO, the same gap as #442 that the UI was spelling out by mime prefix. host() now keeps the document only when the core says yes, which makes "we have a document" the answer, and it travels to the fragment on Result. While in there, two more copies of the same idea: - CoreLoader.MIME_PREFIXES moves out into SupportedDocumentTypes, which adds what RawLoader shows and an extension fallback for the application/octet-stream a provider volunteers when it knows nothing better. What stays separate is the manifest, which XML keeps out of reach - so SupportedFormatsTest walks odrcore's FileType values, asks Odr.mimetypeByFileType for each, and asserts the table and the package manager agree. Adding a format to one side and forgetting the other is now a red test, and the ooxml templates, slideshows and macro enabled variants that walk cannot reach get a case of their own; without them spelled out in the manifest a .dotx was not offered to us at all. The core cannot replace the table itself: its lookups are native, and know one canonical mime per format - no templates, no macroEnabled, no x-vnd.oasis, which is exactly what providers hand out. - MainActivity kept the catch-all switch's component names, preference and toggling inline. That is CatchAllSetting now, so a test can ask for STRICT_CATCH without going through the UI - which SupportedFormatsTest has to, or a previous run that flipped the switch leaves CATCH_ALL answering for everything. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eTZB8rnh9PrdNn1YZexue
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d053b3f4f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Codex on #548: the eleven ooxml variant mime types the last commit spelled out only help a provider that sends the exact mime type. A file manager that sends ACTION_VIEW with just a content:/file: uri falls through to the pathPattern filters, and those listed docx, xlsx, pptx, doc, xls and ppt - so a .dotx or .pptm was claimed by mime and not by name. The gap was older than that, and wider: ott, ots, otp and otg have been in SupportedDocumentTypes for as long as it has existed, with no pattern for any of them. Nothing noticed because the only test of the extension fallback went through application/octet-stream, which the filter claims outright - it matches whatever the patterns happen to say, so it could never fail on a missing one. So aFileWithNoMimeTypeReachesUsByItsExtension asks the question that route actually poses: no type at all, one assertion per entry of EXTENSIONS. Verified it catches this by deleting the .ott and .dotx patterns again - "the manifest has no pathPattern for .ott". EXTENSIONS grows the eleven variants to match, and splits in two on the way. odrcore's fileTypeByFileExtension names one suffix per format and answers UNKNOWN for every one of them, though it renders them all - a .dotx is the same wordprocessing package as a .docx, and libmagic identifies it by content once the file is cached. CORE_EXTENSIONS is the half held against that table; OOXML_VARIANT_EXTENSIONS is the half that asserts the core still does not name them, so a core that learns them turns red rather than leaving the split there for no reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eTZB8rnh9PrdNn1YZexue
The api 32 job crashed the lite run after 13 tests, in the middle of testPDF, with a SIGSEGV inside httplib's accept loop. Chasing it turned up two separate races, both on the teardown path, and odrcore has no safe way through either: #00 httplib::Server::listen_internal() #1 odr::HttpServer::Impl::listen() const #16 CoreLoader.initialize$lambda$0 close() destroyed the server while the thread that called listen() was still inside it. HttpServer.close() frees the native server there and then, so that thread segfaults the next time it touches it - six seconds and two tests later, here. Waiting first does not settle it either: stop() only asks, and the accept loop comes back on its own time. Waiting longer surfaced the second one, an abort rather than a segfault: fdsan: attempted to close file descriptor 101, expected to be unowned, actually owned by ZipArchive 0x73adcf9fe0 #2 close+16 #3 httplib::Server::listen_internal()+920 stop() closes the listening socket itself, and listen_internal() closes it again on the way out. In between the number is free, and in a jvm process something always takes it - art had opened the apk and got 101. fdsan catches the second close and aborts; without fdsan it would quietly close an unrelated file. So the loader stops tearing down. The server is created on the first initialize() and never stopped: close() calls clear(), which drops the documents this loader published and leaves the socket listening, and the process exit reclaims the rest. That was already what happened - nothing here ever outlived its process - and it is what makes both races unreachable, since both need a stop() or a destroy that no longer comes. Smaller as a result: no join, no timeout, no thread to wait on the listening one, and no blocking work left in close() at all, which LoaderService.onDestroy runs on the main thread. bind()'s fallback also stops being reached by our own teardown, because the port is now bound once rather than once per service lifetime. Reported upstream as opendocument-app/OpenDocument.core#631. Until it is fixed, bind and listen once, clear() between documents, never stop(). Both flavors, five runs each, 25 tests, green, and no tombstone written. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012eTZB8rnh9PrdNn1YZexue
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The two commits out of #547 that have nothing to do with the redesign. Both were
reviewed there already; they are here so the redesign PR is only the UI work, and so
this can go out on its own.
Open every format the core renders (#546)
odrcore v6 keeps reference html output for doc, ppt, xls and pptx next to the ones the
app already claimed, so the
TODO: enable pptx tooinCoreLoaderwas the only thingkeeping presentations out - and the legacy binary formats reached the core only by
accident, through the doc mime type that was listed before ppt and xls ever were.
What the loader claims is now one list: opendocument, the whole ooxml family, the three
legacy microsoft formats and pdf. Prefixes, so the templates and the macro enabled
variants come along - the latter are spelled
vnd.ms-word/vnd.ms-excel/vnd.ms-powerpointrather than under the openxmlformats prefix, which is why thoseappear without their subtypes. Text, csv and images stay out: the core renders them too,
but
RawLoaderis what gives them their player, and it only gets a turn when the coreloader says no.
The
STRICT_CATCHfilter follows, so the system offers us pptx/ppt/xls.Two things fall out of it. The
doOoxmlflag is gone - it has been passed true sinceooxml stopped being experimental and now gates nothing. And pdf changes which viewer a
failed upload falls back to, so that stays explicit: google docs still gets the pdf, the
microsoft viewer keeps everything else the core knows.
Editability is the core's answer, not a mime type
doc, ppt and xls now reach
CoreLoader, andprepareMenuoffered Edit for every COREresult except ODS and PDF - so the legacy formats got a button that could only ever end
in a failed save, because
host()would not keep a document open for them.The list of exceptions was never ours to keep.
Document.isEditable()/isSavable()isfalse for the three legacy binary formats, for ooxml spreadsheets and presentations, and
for every spreadsheet including odf - the last being the core's own TODO, the same gap as
#442 that the UI was spelling out by mime prefix.
host()now keeps the document onlywhen the core says yes, which makes "we have a document" the answer, and it travels to the
fragment on
Result.Two more copies of the same idea go with it:
CoreLoader.MIME_PREFIXESmoves out intoSupportedDocumentTypes, which adds whatRawLoadershows and an extension fallback for theapplication/octet-streama providervolunteers when it knows nothing better. What stays separate is the manifest, which XML
keeps out of reach - so
SupportedFormatsTestwalks odrcore'sFileTypevalues, asksOdr.mimetypeByFileTypefor each, and asserts the table and the package manager agree.Adding a format to one side and forgetting the other is now a red test. The ooxml
templates, slideshows and macro enabled variants that walk cannot reach get a case of
their own; without them spelled out in the manifest a
.dotxwas not offered to us atall.
The core cannot replace the table itself: its lookups are native, and know one canonical
mime per format - no templates, no macroEnabled, no
x-vnd.oasis, which is exactly whatproviders hand out.
MainActivitykept the catch-all switch's component names, preference and togglinginline. That is
CatchAllSettingnow, so a test can ask forSTRICT_CATCHwithout goingthrough the UI - which
SupportedFormatsTesthas to, or a previous run that flipped theswitch leaves
CATCH_ALLanswering for everything.Tests
New:
SupportedFormatsTest(instrumented),CoreLoaderTest,SupportedDocumentTypesTest,and
CoreTestcoverage for .doc/.ppt/.xls/.pptx against real fixtures plus a case pinningwhich of them the core reports editable. 22 instrumented tests green on device, up from 12.
🤖 Generated with Claude Code