feat: scope file-list icons to .elp/.elpx and give legacy files the old logo#25
Merged
Conversation
…ld logo Closes #21. Two-part fix: 1. The eXeLearning icon was leaking onto every `.zip` in Nextcloud Files. Root cause: `PermissionService::isElpxFile()` accepted `application/zip` and `application/octet-stream` as standalone MIME signals, so `ElpxPreviewProvider::isAvailable()` returned `true` for any plain ZIP and the provider's fallback PNG (the eXeLearning doc outline) was rendered as the thumbnail. Split `Application::ALLOWED_MIME_TYPES` into two lists: a broad one (still includes zip/octet-stream — used by routing code that already knows the request is for an `.elpx`) and a new narrow `VENDOR_MIME_TYPES` (only `application/vnd.exelearning.elpx`, `application/x-exelearning`, and the new `application/x-exelearning-legacy`). `isElpxFile()` now uses the narrow list for MIME matching, while extension matching is unchanged — `.elpx` and `.elp` still win without a MIME check. 2. Modern `.elpx` and legacy `.elp` are now visually distinguishable. New MIME `application/x-exelearning-legacy` is registered for `.elp` via `make up`'s `mimetypemapping.json`, and a new `mimetypealiases.json` aliases the two vendor MIMEs to their respective icons: - `application/vnd.exelearning.elpx` → `exelearning` (the existing modern teal doc-with-brand-mark icon). - `application/x-exelearning-legacy` → `exelearning-legacy` (new asset: cream/olive doc shape with the pre-`.elpx` upstream logo — the green "eXe" circles over the dark globe-X — embedded as base64 PNG so the icon is a single self-contained file). `application/zip` stays out of the alias file on purpose — that was the original symptom. `ElpxPreviewProvider::MIME_REGEX` also widens to cover `x-exelearning-legacy` so the preview generator still runs on legacy archives that happen to contain a `screenshot.png`. Tests: extended `ApplicationConstantsTest` with two new assertions that pin the bug down — `VENDOR_MIME_TYPES` must not contain `application/zip` or `application/octet-stream`, and must contain both the modern and legacy vendor MIMEs. 10/10 PHPUnit, 51/51 JS, typecheck/lint/build all green.
…pen as eXeLearning" kebab on plain `.zip` Two follow-ups after manual verification on the running stack: 1. Icon discovery: `maintenance:mimetype:update-js` only scans `core/img/filetypes/` (see the comment at the top of the generated `mimetypelist.js`). Apps' `img/filetypes/*.svg` are never picked up automatically, so even with the alias entries in `mimetypealiases.json` the modern and legacy icons fell back to the generic file glyph. `make up` now copies both `exelearning.svg` and `exelearning-legacy.svg` from the staged `custom_apps/exelearning/img/filetypes/` into `core/img/filetypes/` before regenerating the JS list. Production installs need the equivalent admin step — README updated with the new install snippet (and the legacy icon added to the existing alias example). 2. New kebab action `Open as eXeLearning` on plain `.zip` files (extension or `application/zip` mime), shown only when our default action is NOT already claiming the file. Lets users open a `.elpx` that has been re-saved with the wrong extension (or any zip they suspect is an eXeLearning project) without making the viewer the default for every archive in their account. Navigates to the existing `view?fileId=` route, which surfaces the legacy / invalid branches from #20 cleanly. Verified in browser: - `.zip` → gray archive icon, kebab includes "Open as eXeLearning". - `.elp` → distinct legacy icon (the green eXe + dark globe X). - `.elpx` → modern teal mark (unchanged).
Contributor
Author
|
Follow-up after testing on the running stack — pushed in c18dc6e. Real cause of the icon not changing: The dev Plus a new kebab action Verified in the browser:
|
…re one icon
Manual testing surfaced two facts:
1. The `.elp` icon distinguished cleanly in list-view but not in
grid-view, because grid-view uses the preview provider's
fallback PNG (the modern brand mark) for both file types. A full
fix would have meant shipping a second fallback PNG, branching
the provider, and managing two assets in sync — extra surface for
a feature that is more "nice to have" than load-bearing now that
issue #20 already gives the user a clear in-app migration path.
2. The user just wants `.elp` to look like `.elpx` (and `.zip` to
stop looking like an eXeLearning file).
So strip the legacy-MIME branch:
- `Application::LEGACY_MIME_TYPE` removed; `VENDOR_MIME_TYPES` is
now just the modern + the `application/x-exelearning` alias.
- `ElpxPreviewProvider::MIME_REGEX` no longer matches the
`-legacy` variant, and `getThumbnail` no longer branches on
extension.
- `make up` writes `.elp → application/vnd.exelearning.elpx` (same
as `.elpx`) and a single-icon alias file. Only `exelearning.svg`
is copied into `core/img/filetypes/`.
- `img/filetypes/exelearning-legacy.svg` deleted.
- README and constants test updated to match.
The two parts of issue #21 that *did* land stay in place:
- The `isElpxFile()` tightening that stops `.zip` from inheriting
the eXeLearning preview (the original bug).
- The "Open as eXeLearning" kebab action on plain `.zip` files.
Verified in browser (grid + list): `.elp` and `.elpx` show the
modern teal mark, plain `.zip` shows the default archive icon.
51/51 vitest, 9/9 PHPUnit, typecheck/lint/build/cs-check green.
Contributor
Author
|
Simplified per request — pushed in 1851d37:
Verified in browser: list and grid views both show the modern teal mark for |
…s eXeLearning" works on `.zip` Without this, opening a plain `.zip` via the new kebab action 404'd in the controller (`Not an eXeLearning package`), which the page renders as "No file selected. Back to Files". The MIME / extension check stays in `PermissionService::isElpxFile()` because the preview provider still relies on it (we do NOT want every ZIP in Files to inherit our preview thumbnail). The viewer / editor flow, on the other hand, is reached only when the user explicitly clicks an action — at that point bouncing them with a generic 404 is worse than letting the downstream validator surface a clear "not an eXeLearning project" message. Auth + ownership are still enforced by Nextcloud's `getById()` resolving against the user's home folder, and the size limit gate stays in place. So the only thing that changes is: the controller hands a permitted-but-non-vendor file to the frontend, which then runs `validatePackage()` from issue #20 and shows the legacy / invalid branches as appropriate.
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.
Closes #21.
Two-part fix:
1. Stop leaking the eXeLearning thumbnail onto every
.zipRoot cause was in
PermissionService::isElpxFile()(lib/Service/PermissionService.php:36): it acceptedapplication/zipandapplication/octet-streamas standalone MIME signals. That meantElpxPreviewProvider::isAvailable()returnedtruefor any plain ZIP in the user's Files, so the provider's fallback PNG (the eXeLearning doc outline) was rendered as the thumbnail — exactly what the issue reported.Split
Application::ALLOWED_MIME_TYPESinto two named lists:ALLOWED_MIME_TYPES(broad — still includesapplication/zip/application/octet-stream). Used by routing code that already knows the request is for an.elpx.VENDOR_MIME_TYPES(narrow). Onlyapplication/vnd.exelearning.elpx,application/x-exelearning, and the newapplication/x-exelearning-legacy.isElpxFile()now uses the narrow list for the MIME branch. The extension branch (.elpx/.elp) is unchanged, so files we know are ours are still recognised even when Nextcloud reports them as plain ZIP.2. Visually distinguish
.elpfrom.elpxNew MIME
application/x-exelearning-legacyis registered for.elpviamake up'smimetypemapping.json. A newmimetypealiases.json(also written bymake up) maps the two vendor MIMEs to their respective icons:application/vnd.exelearning.elpx→exelearning(existing modern teal doc-with-brand-mark icon)application/x-exelearning-legacy→exelearning-legacy(new asset, see below)application/zipstays out of the alias file on purpose — aliasing it was what caused the original symptom on installs that had once done so.Legacy icon asset
img/filetypes/exelearning-legacy.svgis a 32x32 SVG that embeds (as base64 PNG) the pre-.elpxupstream eXeLearning logo — the green "eXe" circles over the dark globe-X you shared as reference. The doc-shape wrapper uses a cream fill (#fdf6e3) and an olive border (#7a6b3f) instead of the modern brand-blue, so even at 16 px the modern and legacy icons are immediately distinguishable from each other:.elpx.elpPreview provider
ElpxPreviewProvider::MIME_REGEXwidens to also matchapplication/x-exelearning-legacyso the preview generator still runs on legacy archives that happen to ship ascreenshot.png. The bug-fix inisElpxFile()keeps it from running on unrelated ZIPs.Tests
tests/Unit/AppInfo/ApplicationConstantsTest.phpgains two new assertions that pin the bug down so it cannot regress silently:VENDOR_MIME_TYPESmust not containapplication/ziporapplication/octet-stream.PRIMARY_MIME_TYPE) and legacy (LEGACY_MIME_TYPE) vendor MIMEs.10/10 PHPUnit, 51/51 vitest, typecheck / lint / build /
make cs-checkall green.Manual test plan
make down && make up. Upload a random.zip(non-eXeLearning). The Files list shows the default archive icon, no longer the eXeLearning glyph.tests/fixtures/un-contenido-…elpx(PR dev: seed .elp/.elpx fixtures into admin's Files onmake up#23 fixture). It shows the modern teal eXeLearning icon (or itsscreenshot.pngthumbnail if present).tests/fixtures/old_elp_modelocrea.elp. It shows the legacy cream/olive icon with the classic eXe logo — clearly different from the modern one.Follow-ups
img/filetypes/exelearning.svg(modern) still has the embedded PNG in its centre too. If we ever want to slim those files down, both could be reworked as pure SVG paths.