Add Indicator if website is Unsupported. #134

Sryvkver · 2017-12-29T20:11:45Z

Changes the Icon to a gray version if the website is unsupported.

maciex · 2017-12-29T21:22:34Z

I like the idea.

Maybe it would be better to store some kind of "isCompatible" flag for each tab?
We won't need to iterate over all the modules every time the tab is activated.

Added flags for each tab so we dont need to recheck all modules. Thanks to maciex for the suggestion ♥.

maciex · 2017-12-31T12:49:47Z

js/background_scripts/background.js

    })
 });

-//Recheck the selected Tab when any Tab updates
+//Set needCheck flag to true when Tab Updates and recheck currently selected Tab
 chrome.tabs.onUpdated.addListener(function(tabID, changes){


This event is fired when a Tab is updated. We get the updated tab ID.
Could you please explain why you query the tabs to get the current one and update it?
Isn't it redundant?

The currently selected Tab gets rechecked, because when you press a link it only sets the needCheck flag to true, it doesn´t actually recheck the updated tab.

Will try to change it today though.

maciex · 2017-12-31T12:52:14Z

js/background_scripts/background.js

 chrome.tabs.onActivated.addListener(function(info){
-    chrome.tabs.getSelected(null,function(tab){
-        var tabURL = tab.url;
+    chrome.tabs.query({active: true,lastFocusedWindow: true}, function (tab) {


This content of tabs.query seems exactly the same as in tab.onUpdated.

maciex · 2017-12-31T12:56:37Z

js/background_scripts/background.js

        }
+
+        //Check if Tab is Compatible
+        checkIfTabIsCompatible(tabURL,tabID);


Why checkIfTabIsCopatible is run at the end?
It won't set the icon during this run when the tab needChec = true

I think this should rather look like this:

//Check if Tab needs to be Checked if(allTabs[tabID].needCheck) { //Check if Tab is Compatible checkIfTabIsCompatible(tabURL,tabID); } if(allTabs[tabID].isCompatible) { chrome.browserAction.setIcon({path: CompatibleIconPath}); } else { chrome.browserAction.setIcon({path: notCompatibleIconPath}); }

checkIfTabIsCompatible is run at the end, because i added the function in the end and didn´t really think about where to put it.

The function itself changes the icon after the check.

I really like the way you did it, it looks a lot cleaner than mine.
I also really like all the help and suggestions you have ♥ Thank you for that.

maciex

Two more comments.
All in all it looks OK for me, but I didn't try to run this yet.

maciex · 2018-01-02T20:08:21Z

js/background_scripts/background.js

+        }
+
+        //Check if Tab needs to be Checked
+        if(allTabs[tabID].needCheck){


I would suggest to change this part to the code below.
I think it looks cleaner and does less operations.

//Check if Tab needs to be Checked if(allTabs[tabID].needCheck){ allTabs[tabID].isCompatible = false; //console.info("Checking ID", tabID, "Url", tabURL); for (let i = 0; i < allModules.length; i++) { var module = allModules[i]; if (module.canHandleUrl(tabURL)){ allTabs[tabID].isCompatible = true; break; } } allTabs[tabID].needCheck = false; }

maciex · 2018-01-02T20:40:00Z

js/background_scripts/background.js

+            }
+        }
+
+        if(allTabs[tabID].isCompatible && tabID == currentTabId)


This part could be separated into new function (ex. updateAddOnIcon), this way both function will do one thing, exactly what the name says.

I would suggest to change this part to the code below.
I think it looks cleaner and does less operations.

if(tabID == currentTabId) { if (allTabs[tabID].isCompatible) { chrome.browserAction.setIcon({path: CompatibleIconPath}); } else { chrome.browserAction.setIcon({path: notCompatibleIconPath}); } }

Sryvkver · 2018-01-03T16:38:29Z

@maciex Thank you for taking your time and reviewing my code ♥. I hope everything works fine with it, and that there are no problems.

maciex · 2018-01-06T21:15:03Z

@felixire I merged your changes into my fork and I found there's a problem with Youtube site.

Steps to reproduce:

Open a not supported page (ex. http://google.com)
Expected icon: Not Compatible icon
Actual icon: Not Compatible icon
In the same tab open YouTube playlist link (ex. https://www.youtube.com/playlist?list=PLZDyxLlNKRY8Bb-vnoX6r0HoOyPzmViBX)
Expected icon: Not Compatible icon
Actual icon: Not Compatible icon
In the same tab open one of the videos from the playlist.
Expected icon: Compatible icon
Actual icon: Not Compatible icon

From my investigation the problem is caused by the tabs.onUpdated event being fired twice in that scenario. One of the events has the old URL and when checking it first sets needCheck = false and isCompatible = false.
To solve this you would need to somehow limit the event to only one (with the final url) or somehow distinguish the events.

maciex · 2018-01-06T21:20:04Z

@felixire Wait... is it reasonable and possible to get the tabURL when in checkIfTabIsCompatible inside chrome.tabs.query instead of passing this value to this function?

I quickly checked and this seems to be working as the tab[0].url is already the new one.
What do you think?

maciex · 2018-01-06T22:02:46Z

So the changes I'm suggesting:

diff --git a/js/background_scripts/background.js b/js/background_scripts/background.js
index 2dc65b7..316b54d 100755
--- a/js/background_scripts/background.js
+++ b/js/background_scripts/background.js
@@ -79,11 +79,10 @@ var allTabs = {}
 //Check if the newly Selected Tab is compatible
 chrome.tabs.onActivated.addListener(function(info){
     chrome.tabs.query({active: true,lastFocusedWindow: true}, function (tab) {
-        var tabURL = tab[0].url;
         var tabID = tab[0].id;
 
         //Check if Tab is Compatible
-        checkIfTabIsCompatible(tabURL,tabID);
+        checkIfTabIsCompatible(tabID);
     })
 });
 
@@ -93,18 +92,19 @@ chrome.tabs.onUpdated.addListener(function(tabID, tabChanges, tab){
     if(tabChanges.url == null)
         return;
     allTabs[tabID] = {needCheck: true, isCompatible: false};
-    var tabURL = tab.url;
 
-    checkIfTabIsCompatible(tabURL, tabID);
+    checkIfTabIsCompatible(tabID);
 })
 
-function checkIfTabIsCompatible(tabURL, tabID){
-    chrome.tabs.query({active: true,lastFocusedWindow: true}, function (tab) {
-        var currentTabId = tab[0].id;
+function checkIfTabIsCompatible(tabID){
+    chrome.tabs.query({active: true, lastFocusedWindow: true}, function (tab) {
+        let currentTabId = tab[0].id;
+        let tabURL = tab[0].url;
 
         //Check if Tab has the required infos
-        if(allTabs[tabID] == null)
+        if(allTabs[tabID] == null) {
             allTabs[tabID] = {needCheck: true, isCompatible: false};
+        }
 
         //Check if Tab needs to be Checked
         if(allTabs[tabID].needCheck){
@@ -121,16 +121,18 @@ function checkIfTabIsCompatible(tabURL, tabID){
         }
 
         //If the currently selected Tab was checked, update the icon
-        if(tabID == currentTabId)
+        if(tabID == currentTabId) {
             updateAddOnIcon(allTabs[tabID].isCompatible);
+        }
     })
 }
 
 function updateAddOnIcon(isCompatible){
-    if(isCompatible)
+    if(isCompatible) {
         chrome.browserAction.setIcon({path: CompatibleIconPath});
-    else
+    } else {
         chrome.browserAction.setIcon({path: notCompatibleIconPath});
+    }
 }
 
 /*

maciex · 2018-01-06T22:19:24Z

I got one more suggestion...
Shouldn't chrome.tabs.onActivated.addListener just run checkIfTabIsCompatible istead of running chrome.tabs.query and then running checkIfTabIsCompatible ?

Sryvkver · 2018-01-09T13:52:27Z

@maciex I wasn´t able to reproduce the problem with youtube, but we could check if the updated tab has finished loading before checking if the url is compatible. That way the url should always be the new one.

Currently it´s important to pass the tabURL , because otherwise the isCompatible flag from the passed tabID would be set based on the url of the currently selected tab and not of the actual updated tab.

One way of passing only the tabID would be, by changing the chrome.tabs.query inside of checkIfTabIsCompatible to chrome.tabs.get and receiving the url from the callback, but i don´t know if this would fix the problem.

chrome.tabs.onActivated.addListener runs chrome.tabs.query, so we can pass the url of the newly selected tab to the checkIfTabIsCompatible function, this could be changed with the fix above.

maciex · 2018-01-10T15:06:24Z

@maciex I wasn´t able to reproduce the problem with youtube, but we could check if the updated tab has finished loading before checking if the url is compatible. That way the url should always be the new one.

I'm testing this on my Firefox port, maybe it's because of that.

Currently it´s important to pass the tabURL , because otherwise the isCompatible flag from the passed tabID would be set based on the url of the currently selected tab and not of the actual updated tab.

OK, I missed that. You're right.

One way of passing only the tabID would be, by changing the chrome.tabs.query inside of checkIfTabIsCompatible to chrome.tabs.get and receiving the url from the callback, but i don´t know if this would fix the problem.

I'll check that in my environment...

chrome.tabs.onActivated.addListener runs chrome.tabs.query, so we can pass the url of the newly selected tab to the checkIfTabIsCompatible function, this could be changed with the fix above.

Exactly... why query all tabs, if we already new the ID of the tab we want.

I'll prepare my idea of the code and somehow attach here.

maciex · 2018-01-10T15:22:15Z

@felixire Please look at the changes.

Patch:
improvements_patch2.txt

background.js:
background.js.txt

All seems working for me and the code looks simpler.

EDIT:
I spotted a small mistake in the code and fixed it. Uploaded working patch and background.js file.
I also quickly checked on Chrome and it seems to be working.

Sryvkver · 2018-01-12T18:02:17Z

@maciex Thank you for your code. I checked it and everything seems to be working perfectly.

maciex

Two minor problems.

In chrome.tabs.onUpdated.addListener you should check if the allTabs[tabID] is already defined
On browser start the first page is always marked as supported, until first URL/tab change

maciex · 2018-01-29T21:37:07Z

js/background_scripts/background.js

+    if(tabChanges.url == null) {
+        return;
+    }
+    allTabs[tabID].needCheck = true;


Looks like this can be called when the allTabls[tabID] does not exist yet.
I suggest changing this to:

if (allTabs[tabID]) { allTabs[tabID].needCheck = true; }

Added check on Startup. Added check before setting flag.

maciex · 2018-02-01T21:36:17Z

@felixire The onStartup (responsible for changing the status icon on browser startup) listener does not seem to be run at all. I tested this on Firefox. Can you confirm that it works in your env? I also tried to find a way to fix it, but I failed.

Sryvkver · 2018-02-02T14:31:08Z

@maciex The event runs fine for me. I tested it in Chrome and Firefox Developer Version (don´t know if it makes a difference).

maciex · 2018-02-05T12:17:47Z

@felixire OK You're right. It works.
I'm working on the code using "web-ext run" and it seems it's not exactly the same thing as starting firefox. Also hitting "Refresh" on the "Debug Add-ons" page didn't work.

There's still one issue. Just after the Add-on is installed the icon is in the supported mode.

I'll include all the changes in my for in the beta_features branch.

Added multi-window support. Fixed Bug where the Icon didn´t change when chrome ran in the background. Fixed Bug where the Icon didn´t change when the addon was installed or updated.

Sryvkver · 2018-02-07T14:59:13Z

@maciex Good to hear that it works on your end.
I now added a check when the Add-on is installed or updated, which should correct the issue.

I tested these changes in chrome and firefox and everything worked fine.
If you find any more problems, please tell me.

stuartraetaylor · 2019-06-20T21:52:16Z

This is nice, I've merged it into my fork.

Are you still accepting pull requests @khloke ?

Sryvkver added 2 commits December 29, 2017 20:56

Added incompatible Icon

fa034ac

Update background.js

7f97a31

Update background.js

fc0d234

Added flags for each tab so we dont need to recheck all modules. Thanks to maciex for the suggestion ♥.

maciex reviewed Dec 31, 2017

View reviewed changes

Update background.js

0f00552

maciex reviewed Jan 2, 2018

View reviewed changes

Update background.js

808a725

maciex mentioned this pull request Jan 6, 2018

[Feature request] Indicate on toolbar icon that site in currently active tab is supported or not #104

Open

Sryvkver added 2 commits January 12, 2018 18:30

Update background.js

4fb6420

Update background.js

901ba4f

maciex suggested changes Jan 29, 2018

View reviewed changes

Update background.js

31a5d8b

Added check on Startup. Added check before setting flag.

Sryvkver added 4 commits February 5, 2018 17:43

Fix stuff

edd9c24

Added multi-window support. Fixed Bug where the Icon didn´t change when chrome ran in the background. Fixed Bug where the Icon didn´t change when the addon was installed or updated.

Fixed icon flicker

923fcc3

Deleted useless function

4b1a831

Fix Stuff

746e137

Added multi-window support. Fixed Bug where the Icon didn´t change when chrome ran in the background. Fixed Bug where the Icon didn´t change when the addon was installed or updated.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Indicator if website is Unsupported. #134

Add Indicator if website is Unsupported. #134

Sryvkver commented Dec 29, 2017

maciex commented Dec 29, 2017

maciex Dec 31, 2017

Sryvkver Jan 1, 2018

maciex Dec 31, 2017

maciex Dec 31, 2017

Sryvkver Jan 1, 2018

maciex left a comment

maciex Jan 2, 2018

maciex Jan 2, 2018

Sryvkver commented Jan 3, 2018

maciex commented Jan 6, 2018

maciex commented Jan 6, 2018

maciex commented Jan 6, 2018 •

edited

maciex commented Jan 6, 2018

Sryvkver commented Jan 9, 2018

maciex commented Jan 10, 2018

maciex commented Jan 10, 2018 •

edited

Sryvkver commented Jan 12, 2018 •

edited

maciex left a comment

maciex Jan 29, 2018

maciex commented Feb 1, 2018 •

edited

Sryvkver commented Feb 2, 2018

maciex commented Feb 5, 2018

Sryvkver commented Feb 7, 2018

stuartraetaylor commented Jun 20, 2019 •

edited

Add Indicator if website is Unsupported. #134

Are you sure you want to change the base?

Add Indicator if website is Unsupported. #134

Conversation

Sryvkver commented Dec 29, 2017

maciex commented Dec 29, 2017

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

maciex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sryvkver commented Jan 3, 2018

maciex commented Jan 6, 2018

maciex commented Jan 6, 2018

maciex commented Jan 6, 2018 • edited

maciex commented Jan 6, 2018

Sryvkver commented Jan 9, 2018

maciex commented Jan 10, 2018

maciex commented Jan 10, 2018 • edited

Sryvkver commented Jan 12, 2018 • edited

maciex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maciex commented Feb 1, 2018 • edited

Sryvkver commented Feb 2, 2018

maciex commented Feb 5, 2018

Sryvkver commented Feb 7, 2018

stuartraetaylor commented Jun 20, 2019 • edited

maciex commented Jan 6, 2018 •

edited

maciex commented Jan 10, 2018 •

edited

Sryvkver commented Jan 12, 2018 •

edited

maciex commented Feb 1, 2018 •

edited

stuartraetaylor commented Jun 20, 2019 •

edited