Netflix plugin #23

Kelerchian · 2020-10-06T15:56:41Z

No description provided.

Kelerchian · 2020-10-06T16:23:00Z

Context related code in this commit is disabled due to this weird context-related behavior in Lipsurf #24
It is possibly a bug.

mikob

Good job! Some questions and requests in the comments. Also, the plugin in dist/ should be built with build:prod

mikob · 2020-10-08T20:22:00Z

src/Netflix/Netflix.ts

+            return handleChangeAudioAnswer(command);
+        }
+      }
+    } catch {


Better to not catch in that case if anything to let the error propagate.

I notice you catch errors in many places below. Sometimes it makes more sense to let the error propagate up. Right now, the page will catch pageFn errors and log them so we know which commands are failing where. But also we will likely give some sort of user feedback when a command fails (eg. make the live transcript turn red) in a subsequent version.

I actually intend to catch only the JSON.parse part. Not the other.
I think I'll make that clearer. The usage will be something like this.

consumeCommand(maybeCommand, proofKey, (command) => { ... })

mikob · 2020-10-08T20:22:54Z

src/Netflix/Netflix.ts

+
+// Context
+
+/**


comment can be removed now

Removing this

src/Netflix/Netflix.ts

mikob · 2020-10-08T20:37:34Z

src/Netflix/Netflix.ts

+    contexts: {
+      [NetflixPluginContextEnum.watch]: {
+        commands: [
+          "Watch::Pause",


These don't match the format of existing commands. You should take a look at the other plugins so things are consistent.

Sure. I'll rename them

mikob · 2020-10-08T20:57:40Z

src/Netflix/Netflix.ts

+  const tracks = sub.texts;
+  const [index] = await PluginBase.util.fuzzyHighScore(
+    sub.query,
+    tracks.map(track => track.displayName).map(stripNonAlphaNumericAndTrim),


hmm, fuzzyHighScore should do both invalid char stripping and trimming internally. Did you have problems without this stripNonAlphaNumericAndTrim? Can you give me examples?

I actually did this on a whim. I did not run it without that stripNonAlphaNumericAndTrim. Should I remove it?

yes. Also, the version of this that platform uses is language-aware so it's "smarter"

src/Netflix/Netflix.ts

mikob · 2020-10-08T21:04:09Z

src/Netflix/Netflix.ts

+// Messaging
+
+const LIPSURF_BOOT_SCRIPT_ID = "lipsurf-netflix-script";
+const TO_PAGE_PROOF_KEY = String(Math.floor(Math.random() * 1000));


This is cool! Any reason to not just use a static string prepended with PluginBase.util.getNoCollisionUniqueAttr() ?

1.) I didn't realize it is there when I skimmed the docs and 2.) I didn't expect that function to exists in the platform utils (probably the latter influence the former).
I'll change it to that

mikob · 2020-10-08T21:12:01Z

src/Netflix/Netflix.ts

+
+      {
+        name: "Watch::Title",
+        match: ["watch *"],


This is fine, but ideally we would only match valid shows. So you could cache the shows and then use a matchFn with the util's fuzzy matching algorithm to pick the right one. It might be performance prohibitive depending on how big the show db is.

I am optimistic about devices that can run Chrome, can withhold fuzzy matching for tens of thousands less-than-100-bytes utf8 string. And shows stored on Netflix's cache is probably can reach just about hundreds.

I also found out that LipSurf reads Aria-Label so I kind of utilize that rather than the fuzzy matching, but I guess I can pull out the data from Netflix cache back to the extension context ala "ChangeText" and "ChangeAudio".

I ended up combining fuzzyHighScore with 0.8 minimum confidence and falling back to search if no matching videos. The overall UX feels better that way. I found a weird behavior with fuzzyHighScore though.

When I manually map an array of videos to fuzzyHighScore with the video as the only source and then sort the result in descending order, I got a different result to when I call the fuzzyHighScore with all videos put to sources.

const [index,title] = await fuzzyHighScores(query, videoTitles, 0, true);

returns a different result (highest score) than the code below

cosnt {index, title} = (await Promise.all( videoTitles.map(async title => ({ score: await fuzzyHighScores(query, [title], 0, true) })) )) // Maps array index as the index, rather than the fuzzyIndex which is always 0 .map(([_indexFromFuzzy, score], arrayIndex) => ({ score, index: arrayIndex })) // Sort descending .sort(({scoreA},{scoreB}) => scoreB - scoreA)[0]

"I ended up combining fuzzyHighScore with 0.8 minimum confidence and falling back to search if no matching videos. The overall UX feels better that way." that sounds good.
"I got a different result" -- that doesn't seem right. Can you give me the inputs you got those results with?

Here you go.
There's also the code and result.
https://gist.github.com/Kelerchian/df50a53a9ef08cece06fdc326a9c1cfe

src/Netflix/Netflix.ts

mikob

First PR always gets a very thorough review!

mikob · 2020-10-12T04:55:01Z

src/Netflix/Netflix.ts

+  }
+};
+
+const consumeMessageStringAsCommand = (


If the JSON parsing fails, then message is null and the consumeMessageStringAsCommand cb never gets called. That wouldn't be straightforward to track down. On the other hand, if you don't catch the JSON.parse error, it propogates up, gets caught by bugsnag and/or the developer looking at the console will see it right away. Hence I think it would be better to just call JSON.parse without any try-catch.

I have a counter-argument for this.

In this plugin's context, a valid inter-context command message is defined as:

a JSON

has a proofKey field with a certain value determined at the start of the javascript runtime

has a payload field with type Command

consumeMessageStringAsCommand is used to consume string as a command message and is used in the "message" event on windows. There's a chance Netflix have inter-frame message-passing mechanisms somewhere in their app and those message, while probably are also JSON, can be in other format too because JSON is not in the spec.

So my argument is if a string coming from window.addEventListener("message", ...) is not a valid Message then the plugin should not be concerned with it. Because the plugin should not be concerned with it, any error that comes from it should not be handled, because it is irrelevant. Therefore, in this case, it is better for consumeMessageStringAsCommand to silently swallow any errors that come from invalid incoming data.

If it's the debug-friendliness you are concerned with, this scenario fits more into unit-tests rather than runtime-debugging, staying close to "type error should be resolved on compile-time" principle.

That's my two cents, but the final decision will stay in your hands.

What do you think?

Actually, I understand what you mean better now. This is good and fine. You can leave it as-is.

mikob · 2020-10-12T05:04:42Z

src/Netflix/Netflix.ts

@@ -333,7 +373,7 @@ export default <IPluginBase & IPlugin>{
        normal: false
      },
      {
-        name: "ChangeText",
+        name: "Change Text",


Didn't catch this last time, but "Change Text" could be clearer. Perhaps "Change Subtitle Language" Also, the match phrase could have an "[/language]" after "[text/subtitle]"

Changing "Change Text" to "Change Subtitle Language". FYI Text is Netflix's formal definition of subtitle.

Skipping the "[/language]". The command "Change Audio" also deals with language. That makes the word "language" ambiguous to the user.

mikob · 2020-10-12T05:05:39Z

src/Netflix/Netflix.ts

@@ -333,7 +373,7 @@ export default <IPluginBase & IPlugin>{
        normal: false
      },
      {
-        name: "ChangeText",
+        name: "Change Text",
        match: ["[change/switch] [text/subtitle] to *", "[text/subtitle] to *"],


this can just be "[/change /switch ][text/subtitle] to *"

mikob · 2020-10-12T05:20:09Z

src/Netflix/Netflix.ts

+
+      {
+        name: "Watch::Title",
+        match: ["watch *"],


"I ended up combining fuzzyHighScore with 0.8 minimum confidence and falling back to search if no matching videos. The overall UX feels better that way." that sounds good.
"I got a different result" -- that doesn't seem right. Can you give me the inputs you got those results with?

src/Netflix/Netflix.ts

mikob · 2020-10-12T05:35:03Z

src/Netflix/Netflix.ts

+  const results = (
+    await Promise.all(
+      filteredVideos.map(video =>
+        PluginBase.util.fuzzyHighScore(strippedQuery, [video.title], 0, true)


you shouldn't need to strip the query, that string gets preprocessed the same way the sources do.

mikob · 2020-10-12T05:36:28Z

src/Netflix/Netflix.ts

+
+  // Filter out videos which title are empty string
+  const filteredVideos = videos.filter(
+    video => !!stripNonAlphaNumericAndTrim(video.title)


shouldn't need to do the stripping here

Ok. Keeping the trim because videos sometimes has empty string as title

Don't need trim either.

Same as below
Notice that this is not .map. This does not change the title by trimming, this removes video data which title is empty.
This relates more to data cleanup rather than data processing itself (the fuzzyHighScore).

I initially wrote this because of the fuzzyHighScore returns different result when mapped vs unmapped.

I think this is unecessary with it written mapped (result sorted manually)

mikob · 2020-10-12T05:37:56Z

src/Netflix/Netflix.ts

+  const tracks = sub.texts;
+  const [index] = await PluginBase.util.fuzzyHighScore(
+    sub.query,
+    tracks.map(track => track.displayName).map(stripNonAlphaNumericAndTrim),


yes. Also, the version of this that platform uses is language-aware so it's "smarter"

mikob

I couldn't reproduce the context dupes issue. I put this in init. Can you give me a minimal sample to reproduce the issue?

        // prints: before Normal
        console.log('before ' + PluginBase.util.getContext());
        PluginBase.util.addContext('new context');
        PluginBase.util.addContext('new context');
        // prints: after Normal, new context
        console.log('after ' + PluginBase.util.getContext());

mikob · 2020-10-13T01:11:35Z

src/Netflix/Netflix.ts

@@ -360,7 +357,7 @@ export default <IPluginBase & IPlugin>{
      },
      {
        name: "Change Audio",
-        match: ["[change/switch] audio to *", "audio to *"],
+        match: ["[/change/switch] audio to *"],


this will create a match string with a leading space. You should include the space like this: "[/change /switch ]audio to *"

(other match strings below also need to be adjusted for this leading space)

I see.
Saying "audio to *" works though with this. Will change to that.

src/Netflix/Netflix.ts

mikob · 2020-10-13T02:14:07Z

Sorry, haven't replied to some of your previous comments yet but closed the review. Will reply to them soon.

Kelerchian · 2020-10-13T02:28:07Z

Sorry, haven't replied to some of your previous comments yet but closed the review. Will reply to them soon.

Take it easy.

I couldn't reproduce the context dupes issue. I put this in init. Can you give me a minimal sample to reproduce the issue?

        // prints: before Normal
        console.log('before ' + PluginBase.util.getContext());
        PluginBase.util.addContext('new context');
        PluginBase.util.addContext('new context');
        // prints: after Normal, new context
        console.log('after ' + PluginBase.util.getContext());

I'll give you this later.

Kelerchian · 2020-10-13T13:10:13Z

I couldn't reproduce the context dupes issue. I put this in init. Can you give me a minimal sample to reproduce the issue?

        // prints: before Normal
        console.log('before ' + PluginBase.util.getContext());
        PluginBase.util.addContext('new context');
        PluginBase.util.addContext('new context');
        // prints: after Normal, new context
        console.log('after ' + PluginBase.util.getContext());

Sorry, it turns out I made a noob mistake. This is a false alarm.

mikob

This is basically ready. Just some minor things left. Thanks for you patience.

mikob · 2020-10-16T03:29:59Z

src/Netflix/Netflix.ts

+    switch (true) {
+      case context === NetflixPluginContextEnum.browse: {
+        PluginBase.util.removeContext(NetflixPluginContextEnum.watch);
+        PluginBase.util.addContext(NetflixPluginContextEnum.browse);


Can you try using enterContext to be more explicit about the context ordering? Also I think it will fix the "search" homophone issue.

It will need a quite complex sequence to ensure tag works, though. I'll try using enterContext

It works. I'm not sure it is ideal though.

mikob · 2020-10-16T03:33:06Z

src/Netflix/Netflix.ts

+ * TODOs:
+ * - Remove ContextPatcher stuff when context duplication caused by PluginBase.util.addContext is fixed
+ * - Remove homophone `"search": "search"` after homophone priority issue is fixed on the Platform-land
+ *   https://discuss.lipsurf.com/t/homophones-must-be-a-lower-priority-than-other-plugins-command/297/3


I think the issue is that the Normal context is taking priority because it's appearing first in the context order. If you explicitly enter the context with Normal coming after the Netflix-specific contexts, I think this will be fixed. Sorry, docs didn't mention this this so I've updated them: https://docs.lipsurf.com/contexts.html#context-order

mikob · 2020-10-16T03:36:10Z

src/Netflix/Netflix.ts

+  }
+};
+
+const consumeMessageStringAsCommand = (


Actually, I understand what you mean better now. This is good and fine. You can leave it as-is.

src/Netflix/Netflix.ts

mikob · 2020-10-16T23:58:08Z

Merged into monorepo. Nice work!

Kelerchian force-pushed the master branch from 596977e to 7c89dbc Compare October 6, 2020 16:20

Kelerchian marked this pull request as draft October 6, 2020 16:23

Kelerchian force-pushed the master branch from 7c89dbc to d18edd4 Compare October 8, 2020 14:31

Kelerchian marked this pull request as ready for review October 8, 2020 14:34

Netflix plugin

d023d1a

Kelerchian force-pushed the master branch from d18edd4 to d023d1a Compare October 8, 2020 14:55

mikob requested changes Oct 8, 2020

View reviewed changes

Fixes for PR review

f03fbf2

Kelerchian requested a review from mikob October 9, 2020 16:04

mikob requested changes Oct 12, 2020

View reviewed changes

Fixes for PR review (2)

46350ee

mikob requested changes Oct 13, 2020

View reviewed changes

Fixes for PR review (3)

4cc6ae6

mikob requested changes Oct 16, 2020

View reviewed changes

Fixes for PR review (4)

64bb030

mikob added a commit that referenced this pull request Oct 16, 2020

Merged PR: Added Netflix plugin (#23)

aa751ad

mikob closed this Oct 16, 2020


		// Context

		/**

Netflix plugin #23

Netflix plugin #23

Conversation

Kelerchian commented Oct 6, 2020

Kelerchian commented Oct 6, 2020 • edited Loading

mikob 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

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

Choose a reason for hiding this comment

mikob 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

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

mikob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kelerchian Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

mikob commented Oct 13, 2020

Kelerchian commented Oct 13, 2020

Kelerchian commented Oct 13, 2020

mikob 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

mikob commented Oct 16, 2020

Kelerchian commented Oct 6, 2020 •

edited

Loading

Kelerchian Oct 13, 2020 •

edited

Loading