PWA manifest and new @ini_theme_color replacement #2241

micgro42 · 2018-01-24T13:37:37Z

Hi,

this is another go at adding an app manifest to DokuWiki in order to provide PWA functionality.

It supports the config cascade and adds a new event MANIFEST_SEND.

I'm currently not entirely happy with how I select the default images. It is just a list of possible locations and I take the ones I can get.
Further, Google Lighthouse reports that we need an icon with at least 192px to support the automatic installer prompt and an icon of at least 512px for a custom splash screen.
Also, wep apps should have a theme color. I would like to take it from the style.ini. Should I use an existing one (which?) or create a new guranteed placeholder in the style.ini for this?
The general code structure could still be improved. Maybe move the Manifest class to inc/ and the jsonToArray() method to conutils.php?

replaces #2163
fixes #2156

This should have been part of the previous commit 5e0255e Some simple static defaults for the PWA manifest

The goal is to make the css replacement accessible in other contexts, for example for the manifest.

This also adds the default theme color for the DokuWiki template as the green from the existing links.

micgro42 · 2018-02-01T10:56:43Z

I have extracted some functions from lib/exe/css.php, so we can use replacements in the manifest.

Further, I have added the green from the existing links (#008800) as DokuWiki's default theme color. Is that alright or should we choose a different color?

michitux · 2018-02-13T20:04:28Z

lib/exe/manifest.php

+    define('DOKU_INC', __DIR__ . '/../../');
+}
+require_once(DOKU_INC . 'inc/init.php');
+


Shouldn't this check if the action is enabled before sending the manifest?

Good catch! I'll look into it. If it isn't enabled we send a 400?

Should be a 404 I'd say.

Fixed in ac6ceee

splitbrain

I like this a lot already. I think the way images are added is still a bit weird. I don't know what formats are required but maybe we could start to use SVG only here? Eg. look for wiki:logo.svg, :logo.svg, wiki:dokuwiki.svg and use whichever we find first only.

splitbrain · 2018-03-06T18:24:52Z

inc/load.php

@@ -82,6 +82,7 @@ function load_autoload($name){
        'RemoteAPI'             => DOKU_INC.'inc/remote.php',
        'RemoteAPICore'         => DOKU_INC.'inc/RemoteAPICore.php',
        'Subscription'          => DOKU_INC.'inc/subscription.php',
+        'StyleUtil'             => DOKU_INC.'inc/StyleUtil.php',


This shouldn't be needed because the file is namespaced and the autoloader should take care of this.

Also the class seems to be named StyleUtils?

splitbrain · 2018-03-06T18:27:22Z

lib/exe/manifest.php

+
+if (!actionOK('manifest')) {
+    http_status(404, 'Manifest has been disabled in DokuWiki configuration.');
+    return;


return in global scope feels weird. Should this be exit()?

splitbrain · 2018-03-06T18:32:07Z

inc/confutils.php

+        }
+
+        trigger_error('JSON decoding error "' . $jsonErrorText . '" for file ' . $file, E_USER_WARNING);
+        return [];


This error handling seems excessive. First of all it could probably be better handled with all the error codes in a lookup array (with the JSON* constants as the key).

Then I'm wondering about using trigger_error(). Have you tried what happens when you pass invalid json to json_decode()? I would assume that it already does trigger an error that would need to be supressed with @. When we do hande this ourselves I'd think an exception would be the better way?

splitbrain · 2018-03-06T18:37:07Z

inc/Manifest.php

+                ':wiki:favicon.svg',
+                ':favicon.svg',
+                'images/favicon.svg',
+                ':wiki:favicon.ico',


are .ico files supported?

I haven't read the spec yet, but MDN has them in their example: https://developer.mozilla.org/en-US/docs/Web/Manifest#icons

splitbrain · 2018-03-06T18:38:24Z

inc/Manifest.php

+                ':favicon.ico',
+                'images/favicon.ico',
+                ':wiki:logo',
+            ];


We're basically adding all of these if we find them, right? It would be nice to fall back to the DokuWiki logo if none could be found.

splitbrain · 2018-03-06T18:40:05Z

inc/Manifest.php

+            $look = [
+                ':wiki:logo.png',
+                ':logo.png',
+                'images/logo.png',


what does this refer to? There is no images folder in dokuwiki. There might be one in the template but you wouldn't want that one if you already found one in wiki:logo.png?

micgro42 · 2018-03-06T19:42:58Z

@splitbrain I would like to limit the included icons to a small set of sensible defaults. The user has already many opportunities to overwrite these defaults.

The official spec https://www.w3.org/TR/2018/WD-appmanifest-20180222/#icons-member states explicitly that both .ico and .svg are valid and hints some use-cases. Do we have a SVG version of the DokuWiki logo?

splitbrain · 2018-03-06T19:57:00Z

Yes we have a SVG version (see https://www.dokuwiki.org/logo). We could include it next to the png version in the wiki namespace. We could also adjust the template to make use of it?

Defining SVG both as 17x17 and 512x512 is intended to support custom splash screens, though the documentation is somewhat unclear. This currently satisfies the Google Lighthouse auditing tool.

This color is for example shown in Chrome on Android as menu-bar coloring.

Since StyleUtils has a namespace, it is already handled correctly by the autoloader. This is further underlined by the fact that the removed line isn't actually working because of the missing `s` in the classname.

This isn't really the best place for that error handling.

A recent DokuWiki merge request changed how the style.ini can be accessed: dokuwiki/dokuwiki#2241

feat: provide web app manifest (dynamically geneated)

5e0255e

micgro42 changed the title ~~feat: provide web app manifest (dynamically geneated)~~ provide dynamically geneated web app manifest Jan 24, 2018

fix: add default manifest.json

ff1e4ea

This should have been part of the previous commit 5e0255e Some simple static defaults for the PWA manifest

micgro42 added the Needs Feedback/Discussion label Jan 25, 2018

micgro42 added 3 commits February 1, 2018 11:52

refactor: extract css utils to an autoloaded class

fb1f908

The goal is to make the css replacement accessible in other contexts, for example for the manifest.

feat: use style replacements for background and theme color

6f85823

This also adds the default theme color for the DokuWiki template as the green from the existing links.

feat: ensure that the cope is always only the current wiki

10b4c84

micgro42 changed the title ~~provide dynamically geneated web app manifest~~ PWA manifest and new @ini_theme_color replacement Feb 1, 2018

refactor: move manifest functions to more intuitive places

c907183

michitux reviewed Feb 13, 2018

View reviewed changes

fix(manifest): check if manifest action enabled before sending

ac6ceee

albacoretuna mentioned this pull request Feb 25, 2018

Add manifest.json for PWA functionality #2163

Closed

splitbrain reviewed Mar 6, 2018

View reviewed changes

micgro42 added 6 commits March 7, 2018 11:24

feat(PWAManifest): Use sensible defaults for icons

2d8226d

Defining SVG both as 17x17 and 512x512 is intended to support custom splash screens, though the documentation is somewhat unclear. This currently satisfies the Google Lighthouse auditing tool.

feat: add DokuWiki SVG logo

b52493c

feat(metaheaders): Set theme-color meta header

40ca854

This color is for example shown in Chrome on Android as menu-bar coloring.

fix: StyleUtils is already handled by the autoloader

6c4fa3b

Since StyleUtils has a namespace, it is already handled correctly by the autoloader. This is further underlined by the fact that the removed line isn't actually working because of the missing `s` in the classname.

fix: use exit instead of return in the global scope

f3d2b6a

fix: drop JSON error handling

dceb2cc

This isn't really the best place for that error handling.

micgro42 added ToDo: needDocUpdate and removed Needs Feedback/Discussion labels Mar 28, 2018

splitbrain approved these changes Mar 29, 2018

View reviewed changes

micgro42 merged commit 430a27b into master Apr 5, 2018

micgro42 deleted the manifest branch April 5, 2018 07:51

micgro42 mentioned this pull request Apr 5, 2018

New Feature Documentation #2299

Closed

4 tasks

micgro42 added a commit to cosmocode/dokuwiki-template-sprintdoc that referenced this pull request Apr 24, 2018

Fix svg replacement in Greebo+

8c8001b

A recent DokuWiki merge request changed how the style.ini can be accessed: dokuwiki/dokuwiki#2241

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PWA manifest and new @ini_theme_color replacement #2241

PWA manifest and new @ini_theme_color replacement #2241

micgro42 commented Jan 24, 2018 •

edited

micgro42 commented Feb 1, 2018

michitux Feb 13, 2018

micgro42 Feb 15, 2018

splitbrain Feb 15, 2018

micgro42 Feb 16, 2018

splitbrain left a comment

splitbrain Mar 6, 2018

splitbrain Mar 6, 2018

splitbrain Mar 6, 2018

splitbrain Mar 6, 2018

splitbrain Mar 6, 2018

micgro42 Mar 6, 2018

splitbrain Mar 6, 2018

splitbrain Mar 6, 2018

micgro42 commented Mar 6, 2018

splitbrain commented Mar 6, 2018

PWA manifest and new @ini_theme_color replacement #2241

PWA manifest and new @ini_theme_color replacement #2241

Conversation

micgro42 commented Jan 24, 2018 • edited

micgro42 commented Feb 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitbrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

micgro42 commented Mar 6, 2018

splitbrain commented Mar 6, 2018

micgro42 commented Jan 24, 2018 •

edited