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

merged 4 commits into from
Feb 10, 2020

Conversation

andreban
Copy link
Member

@andreban andreban commented Feb 7, 2020 •
edited
Loading

  • Change TwaManifest.shortcuts to be ShortcutInfo[] instead
    of string.
  • Refactor code to reflect that change
  • Fixes reversed if condition in init leading to not adding
    shortcuts.
  • Test the result of generateShortcut()
  • Uses correct LauncherActivity in build.gradle
  • Truncates short_name to 12 characters, when unavailable and copying from name. Recommendation for 12 chars here

@andreban
Refactors TwaManifest.shortcuts
90f55c4 
- Change `TwaManifest.shortcuts` to be `ShortcutInfo[]` instead
  of `string`.
- Refactor code to reflect that change
- Fixes reversed if condition in `init` leading to not adding
  shortcuts.
- Test the result of `generateShortcut()§
@andreban andreban requested a review from PEConn February 7, 2020 16:06
@andreban
Copy link
Member Author

andreban commented Feb 7, 2020

@rayankans can you take a look as well?

andreban
andreban commented Feb 7, 2020
View reviewed changes
src/lib/TwaGenerator.ts
@@ -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.

🎊

rayankans
rayankans approved these changes Feb 7, 2020
View reviewed changes
Copy link
Contributor

@rayankans rayankans left a comment

Choose a reason for hiding this comment

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

LGTM with a few nits, thanks!

src/lib/TwaManifest.ts Outdated

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

template_project/app/build.gradle
@@ -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.

src/lib/TwaManifest.ts Outdated
.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.

PEConn
PEConn reviewed Feb 7, 2020
View reviewed changes
template_project/app/build.gradle
@@ -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
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.

src/lib/TwaGenerator.ts
@@ -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
Collaborator

Choose a reason for hiding this comment

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

🎊

src/lib/TwaManifest.ts Outdated
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!

@andreban
Fixes Review Notes
4f4e8a0 
- Changes shortcut limit from 3 to 4
- Improves identation
@andreban
Removes Undeeded code
4210d4a
@rayankans rayankans mentioned this pull request Feb 10, 2020
Closed
@andreban andreban merged commit 819d1ef into GoogleChromeLabs:master Feb 10, 2020
@andreban andreban deleted the refactor-shortcuts branch February 13, 2020 08:14
@andreban andreban mentioned this pull request Feb 14, 2020
Closed
@TesheG TesheG mentioned this pull request Mar 19, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rayankans rayankans rayankans approved these changes

@PEConn PEConn PEConn left review comments

Assignees
No one assigned
Labels
internal
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andreban @PEConn @rayankans