Fix inconsistency: Use themed manifest for PWA installations to avoid breaking user theming - #58557
Fix inconsistency: Use themed manifest for PWA installations to avoid breaking user theming#58557crowetic wants to merge 2 commits into
Conversation
…ll follow theme. Signed-off-by: crowetic <jason@crowetic.com>
| $route = $this->urlGenerator->linkToRoute('theming.Icon.getTouchIcon', ['app' => $app]); | ||
| } | ||
| if ($image === 'manifest.json') { | ||
| // Force instance branding for the installable shell manifest. |
There was a problem hiding this comment.
Sorry for the ambiguous comment, by 'shell manifest' I meant the Web App Manifest used when the browser installs Nextcloud as a PWA.
This ensures manifest.json is served through the theming route so the installed PWA reflects the instance’s configured branding (icons, name, colors).
I can update the comment to avoid the ambiguous wording if needed.
| if ($route !== false) { | ||
| return $route . '?v=' . $this->util->getCacheBuster(); | ||
| } |
There was a problem hiding this comment.
In which case would $route be false?
There was a problem hiding this comment.
Here... $route can remain false when no themed route is resolved and the logic falls back to the original asset path.
The guard keeps the behavior consistent with the surrounding logic so the cache-buster is only appended when a themed route is actually returned.
$route is initialized to false and stays false whenever no theming route applies. For manifest.json, we also explicitly return false when the app ships a static img/manifest.json (meaning 'don’t replace'). So the guard is consistent with the existing function semantics.
There was a problem hiding this comment.
Right, I inverted my question. From what I understand, it can only be not false if the line above returns something else than false. Shouldn't we move that block in the previous if statement?
There was a problem hiding this comment.
Yes, you're right. In this block $route is only used for the manifest route, and linkToRoute() should always return a string here. The extra guard doesn't add value.
I will inline the return (with cache buster) directly in manifest.json branch after the static manifest check, and drop the redundant contition.
Sound okay?
artonge
added
enhancement
feature: theming
php
|
This PR, however, only handles the manifest issue for the 'overall PWA' functionality I'm attempting to bring back for NC33. In previous versions you could accomplish the 'overall PWA' by installing in a pre-login state (before authenticating). You would then end up with a PWA for the entire cloud instance, that wouldn't show the annoying path bar when accessing various apps inside nextcloud. However, given NC33's shift toward a per-app PWA installation (whichever app you happen to be accessing when you install the PWA) - the 'overall PWA' functionality is broken. (I opened an issue for this and a few others replied saying they prefer the overall PWA setup as well. When the issue wasn't fixed, I decided to fix it myself.) This PR takes care of the manifest side, so the PWA installation has the correct naming, however, I didn't want to mess with the icon side directly in the core nextcloud code. This PR wording can be modified to - 'Fix inconsistency: core/settings ship static manifests, which breaks instance branding in PWA installs. Force them to use the theming manifest so instance branding is consistent.' So it is still a fix overall, and it will be a requirement for those that would like to have a PWA installed that doesn't install explicitly to an app inside the cloud instance. I am also working on a very simple plugin that will expose a page to install the 'overall PWA' with the branded cloud icon and branding naming, but this PR/fix is required as well. Hopefully this makes sense. Let me know if I can help with anything. |
crowetic
changed the title
crowetic
changed the title
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
4 participants
…ll follow theme.
Resolves - #56135
Summary - Fix inconsistency: core/settings ship static manifests, which breaks instance branding in PWA installs. Force them to use the theming manifest so instance branding is consistent.
This PR also makes it possible to install an 'overall PWA' that stays with the instance branding applied in theme. (NOTE - the icon may still have an icon from the path if the path supplies a specific app icon, a plugin to setup an 'overall PWA' like those that were possible in older versions of NC will be made available in the near future. But this PR still fixes the naming issue being defaulted to Nextcloud and default icon.)
(Tested on NC33 installed server running installation from nextcloud/vm and updated to 33 via updater in the vm scripts. However, in theory any installation should work just fine. )