ansh / jiffyreader.com

A Browser Extension for faster reading on ANY website!
https://www.jiffyreader.com/
GNU General Public License v3.0
3.73k stars 200 forks source link

keep textnode alive #115

Closed paxelpixel closed 2 years ago

paxelpixel commented 2 years ago

attempt to address #114

issues these address I think? are:

I've added some detailed comments and super curious what you guys think as I particularly don't like having to create an observer for each text node.

the main Idea I had for this is to keep the text node alive because I noticed youtube has some internal reference to the text node. and if we removed it. it won't update the text anymore causing the stale titles.

asieduernest12 commented 2 years ago

@jhonalino i think you solved it these are fixed while i was testing #114 #

asieduernest12 commented 2 years ago

attempt to address #114

issues these address I think? are:

  • thumbnail names not updated on the homepage
  • never ending duplicate labels and titles as you navigate and traverse youtube
  • suggestions video title not updating on the side bar

I've added some detailed comments and super curious what you guys think as I particularly don't like having to create an observer for each text node.

the main Idea I had for this is to keep the text node alive because I noticed youtube has some internal reference to the text node. and if we removed it. it won't update the text anymore causing the stale titles.

we can go with this so that a fix can be pushed while still keeping options open for a cleaner solution but i believe it is good to go, i tested in performance tab but did not see any negative impact really unless anyone has a different insight into seeing how multiple observers could impact performance especially around memory utilization.

this can be merged as is if @jhonalino is satisfied

paxelpixel commented 2 years ago

@asieduernest12 I realized that the per textNode mutation observer was looking at the target Node, so I added that check for the root mutation observer with characterData type.

And I removed the per text node observer so we are back to one root mutation observer, and it seems to still be working and I believe this is so much better than having multiple observers. I've added comments to my changes. I'm curious what you think?

asieduernest12 commented 2 years ago

@asieduernest12 I realized that the per textNode mutation observer was looking at the target Node, so I added that check for the root mutation observer with characterData type.

And I removed the per text node observer so we are back to one root mutation observer, and it seems to still be working and I believe this is so much better than having multiple observers. I've added comments to my changes. I'm curious what you think?

  • unrelated, I noticed youtube has a lot of svg tags and it was being bolded, so I added that to ignored tag list,

great catch on the youtube and certainly having less mutation observers i think is better. Happy to merge this now.

Would you mind making a short explanation of how Preferences.init works, It looks like a spacex rocket engine in there. I nearly burnt myself looking at all the codes but it would help new folks or anyone wanting to contribute or just learn from whats happening there cause it sure looks fancy. Cheers

paxelpixel commented 2 years ago

Would you mind making a short explanation of how Preferences.init works

yeah definitely! sorry I didn't mean that to be complicated.

Preference contains methods to manage preferences and Preference.init returns these methods. And one of its requirement is to know origin of the current tab. so it can work with "per site" or local preferences

So first you want to pass that to init like:

Preferences.init({
  getOrigin: async () => { /* provide a function that returns the current tab origin */ }
}),

Now what if you want to know everytime there is a change in preference? You can do that by passing subscribe function on the init as well:

Preferences.init({
  getOrigin: async () => { /* provide a function that returns the current tab origin */ },
  subscribe: (newPreferences) => { /* you can update UI here based on the new preference */ },
}),

Now that we initialized init. it will return methods we can use to manage the preferences. One of the most important method it returns, is the start()

const { start, setPrefs } = Preferences.init({
  getOrigin //... 
  subscribe //...
}),

start() will make sure the preference have default values if they are null and will also call the subscribe function you added via init with the latest preference.

setPrefs() is also returned via Preference.init and will allow you to set the preferences with whatever the current scope is. so for example: the user is currently on "per site" preference and you want to update fixationStrength and calling setPrefs will also call the subscribe function you initialized via init.

Examples:

To set saccadesInterval:

setPrefs({
    saccadesInterval: 2,
});

To set fixationStrength

setPrefs({
  fixationStrength: 1,
});

To change scope to global and set lineheight on that global scope

setPrefs({
  lineHeight: 1,
});

You can also pass it a function that returns the preferences you want to set, a good use case for this is if you want to decide the new preference based on the current preference. so for toggling stuff. if a current pref is false, then you can check for that value and return true

setPrefs(({ onPageLoad }) => ({
  onPageLoad: !onPageLoad,
}));

Use Case: maybe if we can find an optimal setting and provide that as a template? so a user would click a template and it will set the preference of that template.

for example: one of the many combinations.

setPrefs(({ onPageLoad }) => ({
  saccadesInterval: 2,
  lineHeight: 3,
  fixationStrength: 3,
}));
paxelpixel commented 2 years ago

another example for getPrefs method return by Preference.init is on this pr #117