Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactors TwaManifest.shortcuts #99

Merged
merged 4 commits into from
Feb 10, 2020
Merged

Refactors TwaManifest.shortcuts #99

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
90f55c4
Refactors TwaManifest.shortcuts
andreban Feb 7, 2020
7bdc905
Moves short_name size to a variable
andreban Feb 7, 2020
4f4e8a0
Fixes Review Notes
andreban Feb 7, 2020
4210d4a
Removes Undeeded code
andreban Feb 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/cli/cmds/init.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ async function confirmTwaConfig(twaManifest: TwaManifest): Promise<TwaManifest>
}, {
name: 'shortcuts',
type: 'confirm',
message: 'Include app shortcuts?\n' + twaManifest.shortcuts,
message: 'Include app shortcuts?\n' + JSON.stringify(twaManifest.shortcuts, null, 2),
default: true,
}, {
name: 'packageId',
Expand Down Expand Up @@ -122,7 +122,7 @@ async function confirmTwaConfig(twaManifest: TwaManifest): Promise<TwaManifest>
twaManifest.startUrl = result.startUrl;
twaManifest.iconUrl = result.iconUrl;
twaManifest.maskableIconUrl = result.maskableIconUrl;
twaManifest.shortcuts = result.shortcuts ? '[]' : twaManifest.shortcuts;
twaManifest.shortcuts = result.shortcuts ? twaManifest.shortcuts : [];
twaManifest.packageId = result.packageId;
twaManifest.signingKey = {
alias: result.keyAlias,
Expand Down
7 changes: 2 additions & 5 deletions src/lib/TwaGenerator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import * as fs from 'fs';
import fetch from 'node-fetch';
import {template} from 'lodash';
import {promisify} from 'util';
import {TwaManifest} from './TwaManifest';
import {TwaManifest, ShortcutInfo} from './TwaManifest';
import Jimp = require('jimp');
import Log from './Log';

Expand Down Expand Up @@ -232,10 +232,7 @@ export class TwaGenerator {
await this.generateIcons(twaManifest.iconUrl, targetDirectory, IMAGES);
}

// TODO(andreban): TwaManifest.shortcuts is a string, which is being parsed into an Object.
// Needs to be transformed into a proper Class.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PEConn I think this is the last any

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎊

await Promise.all(JSON.parse(twaManifest.shortcuts).map((shortcut: any, i: number) => {
await Promise.all(twaManifest.shortcuts.map((shortcut: ShortcutInfo, i: number) => {
const imageDirs = SHORTCUT_IMAGES.map(
(imageDir) => ({...imageDir, dest: `${imageDir.dest}shortcut_${i}.png`}));
return this.generateIcons(shortcut.chosenIconUrl, targetDirectory, imageDirs);
Expand Down
80 changes: 44 additions & 36 deletions src/lib/TwaManifest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import * as fs from 'fs';
import fetch from 'node-fetch';
import * as util from './util';
import Color = require('color');
import {WebManifestIcon, WebManifestJson, WebManifestShortcutJson} from './types/WebManifest';
import {WebManifestIcon, WebManifestJson} from './types/WebManifest';

// Regex for disallowed characters on Android Packages, as per
// https://developer.android.com/guide/topics/manifest/manifest-element.html#package
Expand Down Expand Up @@ -61,34 +61,19 @@ function generatePackageId(host: string): string {
/**
* A wrapper around the WebManifest's ShortcutInfo.
*/
class ShortcutInfo {
name: string;
shortName: string;
url: string | undefined;
chosenIconUrl: string | undefined;

export class ShortcutInfo {
/**
* @param {Object} the WebManifest's ShortcutInfo.
* @param {URL} webManifestUrl the URL where the webmanifest is available.
* @param {string} name
* @param {string} shortName
* @param {string} url target Url for when the shortcut is clicked
* @param {string} chosenIconUrl Url for the icon
*/
constructor(shortcutInfo: WebManifestShortcutJson, webManifestUrl: URL) {
this.name = shortcutInfo['name'] || '';
this.shortName = shortcutInfo['short_name'] || this.name;
this.url = shortcutInfo['url'] ?
new URL(shortcutInfo['url'], webManifestUrl).toString() : undefined;

if (shortcutInfo.icons === undefined) {
return;
}

const suitableIcon = util.findSuitableIcon(shortcutInfo.icons, 'any');
if (suitableIcon !== null) {
this.chosenIconUrl = new URL(suitableIcon.src, webManifestUrl).toString();
}
}

isValid(): boolean {
return this.name != undefined && this.url != undefined && this.chosenIconUrl != undefined;
constructor(readonly name: string, readonly shortName: string, readonly url: string,
readonly chosenIconUrl: string) {
this.name = name;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're using Parameter Properties here right? I don't think you need lines 76 to 79.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great! Updated!

this.shortName = shortName;
this.url = url;
this.chosenIconUrl = chosenIconUrl;
}
}

Expand Down Expand Up @@ -132,7 +117,7 @@ export class TwaManifest {
signingKey: SigningKeyInfo;
appVersionCode: number;
appVersionName: string;
shortcuts: string;
shortcuts: ShortcutInfo[];
generatorApp: string;
webManifestUrl?: URL;

Expand Down Expand Up @@ -199,7 +184,7 @@ export class TwaManifest {
}

generateShortcuts(): string {
return '[' + JSON.parse(this.shortcuts).map((s: ShortcutInfo, i: number) =>
return '[' + this.shortcuts.map((s: ShortcutInfo, i: number) =>
`[name:'${s.name}', short_name:'${s.shortName}', url:'${s.url}', icon:'shortcut_${i}']`)
.join(',') +
']';
Expand All @@ -222,16 +207,39 @@ export class TwaManifest {

const fullStartUrl: URL = new URL(webManifest['start_url'] || '/', webManifestUrl);

const shortcuts: ShortcutInfo[] = (webManifest.shortcuts || [])
.map((s: WebManifestShortcutJson) => new ShortcutInfo(s, webManifestUrl))
.filter((s: ShortcutInfo) => s.isValid())
.filter((_: ShortcutInfo, i: number) => i < 4);
const shortcuts: ShortcutInfo[] = [];

if (webManifest.shortcuts) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: for (const s of (webManifest.shortcuts || [])) {
to reduce indentation.

I'm not too familiar with TS but it seems to me that webManifest.shortcuts is guaranteed to be a list instead of a null or undefined though?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a WebManifest shortcut, defined here: https://github.com/GoogleChromeLabs/llama-pack/blob/master/src/lib/types/WebManifest.ts#L40

It can be undefined.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changed the for loop.

for (const s of webManifest.shortcuts) {
if (!s.icons || !s.url || (!s.name && !s.short_name)) {
continue;
}

const suitableIcon = util.findSuitableIcon(s.icons, 'any');
if (!suitableIcon) {
continue;
}

const name = s.name || s.short_name;
const shortName = s.short_name || s.name!.substring(0, 12);
const url = new URL(s.url, webManifestUrl).toString();
const iconUrl = new URL(suitableIcon.src, webManifestUrl).toString();
const shortcutInfo = new ShortcutInfo(name!, shortName!, url, iconUrl);

shortcuts.push(shortcutInfo);

if (shortcuts.length === 3) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

break;
}
}
}

const twaManifest = new TwaManifest({
packageId: generatePackageId(webManifestUrl.host),
host: webManifestUrl.host,
name: webManifest['name'] || webManifest['short_name'] || DEFAULT_APP_NAME,
launcherName: webManifest['short_name'] || webManifest['name'] || DEFAULT_APP_NAME,
launcherName:
webManifest['short_name'] || webManifest['name']?.substring(0, 12) || DEFAULT_APP_NAME,
themeColor: webManifest['theme_color'] || DEFAULT_THEME_COLOR,
navigationColor: DEFAULT_NAVIGATION_COLOR,
backgroundColor: webManifest['background_color'] || DEFAULT_BACKGROUND_COLOR,
Expand All @@ -246,7 +254,7 @@ export class TwaManifest {
},
splashScreenFadeOutDuration: DEFAULT_SPLASHSCREEN_FADEOUT_DURATION,
enableNotifications: DEFAULT_ENABLE_NOTIFICATIONS,
shortcuts: JSON.stringify(shortcuts, undefined, 2),
shortcuts: shortcuts,
webManifestUrl: webManifestUrl.toString(),
});
return twaManifest;
Expand Down Expand Up @@ -295,7 +303,7 @@ export interface TwaManifestJson {
signingKey: SigningKeyInfo;
appVersionCode?: number; // Older Manifests may not have this field.
appVersion: string; // appVersionName - Old Manifests use `appVersion`. Keeping compatibility.
shortcuts: string;
shortcuts: ShortcutInfo[];
generatorApp?: string;
webManifestUrl?: string;
}
Expand Down
28 changes: 15 additions & 13 deletions src/spec/lib/TwaManifestSpec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,15 @@ describe('TwaManifest', () => {
expect(twaManifest.splashScreenFadeOutDuration).toBe(300);
expect(twaManifest.enableNotifications).toBeFalse();
expect(twaManifest.webManifestUrl).toEqual(manifestUrl);
expect(JSON.parse(twaManifest.shortcuts)).toEqual([
{
name: 'shortcut name',
shortName: 'short',
url: 'https://pwa-directory.com/launch',
chosenIconUrl: 'https://pwa-directory.com/shortcut_icon.png',
},
]);
expect(twaManifest.shortcuts.length).toBe(1);
expect(twaManifest.shortcuts[0].name).toBe('shortcut name');
expect(twaManifest.shortcuts[0].shortName).toBe('short');
expect(twaManifest.shortcuts[0].url).toBe('https://pwa-directory.com/launch');
expect(twaManifest.shortcuts[0].chosenIconUrl)
.toBe('https://pwa-directory.com/shortcut_icon.png');
expect(twaManifest.generateShortcuts())
.toBe('[[name:\'shortcut name\', short_name:\'short\',' +
' url:\'https://pwa-directory.com/launch\', icon:\'shortcut_0\']]');
});

it('Sets correct defaults for unavailable fields', () => {
Expand All @@ -94,7 +95,8 @@ describe('TwaManifest', () => {
expect(twaManifest.splashScreenFadeOutDuration).toBe(300);
expect(twaManifest.enableNotifications).toBeFalse();
expect(twaManifest.webManifestUrl).toEqual(manifestUrl);
expect(twaManifest.shortcuts).toBe('[]');
expect(twaManifest.shortcuts).toEqual([]);
expect(twaManifest.generateShortcuts()).toBe('[]');
});

it('Uses "name" when "short_name" is not available', () => {
Expand All @@ -104,7 +106,7 @@ describe('TwaManifest', () => {
const manifestUrl = new URL('https://pwa-directory.com/manifest.json');
const twaManifest = TwaManifest.fromWebManifestJson(manifestUrl, manifest);
expect(twaManifest.name).toBe('PWA Directory');
expect(twaManifest.launcherName).toBe('PWA Directory');
expect(twaManifest.launcherName).toBe('PWA Director');
});
});

Expand All @@ -128,7 +130,7 @@ describe('TwaManifest', () => {
},
splashScreenFadeOutDuration: 300,
enableNotifications: true,
shortcuts: '[{name: "name", url: "/", icons: [{src: "icon.png"}]}]',
shortcuts: [{name: 'name', shortName: 'shortName', url: '/', chosenIconUrl: 'icon.png'}],
webManifestUrl: 'https://pwa-directory.com/manifest.json',
generatorApp: 'test',
} as TwaManifestJson;
Expand Down Expand Up @@ -173,7 +175,7 @@ describe('TwaManifest', () => {
},
splashScreenFadeOutDuration: 300,
enableNotifications: true,
shortcuts: '[{name: "name", url: "/", icons: [{src: "icon.png"}]}]',
shortcuts: [{name: 'name', url: '/', chosenIconUrl: 'icon.png'}],
generatorApp: 'test',
} as TwaManifestJson;
const twaManifest = new TwaManifest(twaManifestJson);
Expand Down Expand Up @@ -205,7 +207,7 @@ describe('TwaManifest', () => {
},
splashScreenFadeOutDuration: 300,
enableNotifications: true,
shortcuts: '[{name: "name", url: "/", icons: [{src: "icon.png"}]}]',
shortcuts: [{name: 'name', url: '/', chosenIconUrl: 'icon.png'}],
} as TwaManifestJson);
expect(twaManifest.validate()).toBeTrue();
});
Expand Down
2 changes: 1 addition & 1 deletion template_project/app/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ task generateShorcutsFile {
'intent'(
'android:action': 'android.intent.action.MAIN',
'android:targetPackage': twaManifest.applicationId,
'android:targetClass': 'android.support.customtabs.trusted.LauncherActivity',
'android:targetClass': 'com.google.androidbrowserhelper.trusted.LauncherActivity',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure about this? I'm guessing you tested it and the previous targetClass did not work.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a left-over when we migrated to androix.browser-1.2.0, which doesn't have a LauncherActivity anymore. This should have been changed with the other references across the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've tested this, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, tested against https://contentindex.dev/manifest.json. It works :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the previous value not work though? It's still working for me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using the android-browser-helper-1.1.0? The old class doesn't exist anymore in androidx.browser, so I wouldn't expect it to work.

'android:data': s.url)
'categories'('android:name': 'android.intent.category.LAUNCHER')
}
Expand Down