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.

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

Firefox: Mixed Mode Themed Favicon Issue #72

Merged
merged 101 commits into from
Oct 10, 2021
Merged

Firefox: Mixed Mode Themed Favicon Issue #72

merged 101 commits into from
Oct 10, 2021

Conversation

ZimCodes
Copy link
Collaborator

@ZimCodes ZimCodes commented Sep 29, 2021

  1. Create a solution mitigation solution against the creating of tabs very quickly.
  2. Appropriate theme for options page for each tab loads correctly in mixed mode.
  3. Fix tabs not being randomized when creating new tabs really quickly in mixed mode
  4. Fix text selection & scrollbar not loading appropriately with their individual tab in mixed mode

Description

The focus of this pull request is to address the issue involved with creating a new tab rapidly in mixed mode. #66

I try to create a series of new tabs at the speed of light, then the default favicon will be displayed for now onwards

There were many different possible solutions, all of them failing right in front of my eyes. Therefore, I decided to mitigate the issue instead. And eventually developed a different solution.

Solution/Mitigation

While creating new tabs, some tabs were glitched, meaning any previously unglitched tab was the theme of the glitched tab. To clarify, if I venture from an unglitched tab to a glitched tab, the last unglitched tab I was on is now the temporary theme of the glitched tab.

Glitched tabs are tabs not added to the mixed tabs list. Since the creation of tabs are happening very quickly, and majority of operations are async (handled lazily, browser.storage.local.set) there's not enough time to be placed in the list before the page is fully loaded. This also causes problems with applying individual theming to each tab in mixed mode, causing some to be glitched. Some problems that occurred were:

  • Favicon no longer themed
  • Toolbar is a different theme from the background image, favicon and action button.
  • Background image is different from the rest of the theme (until refreshed)

The solution is to purge all glitched tabs. However, you will also have to purge them at a decent time interval. Purge too quickly and the favicon problem arises. Purge too slowly, the tab I might be using to login will suddenly close, as that tab might have been marked for purging initially.

Appropriate Theme

The options page theme has been fixed to match with the current active tab's theme.

Text Select & Scrollbar

The scrollbar & text selection content scripts now load appropriately with their individual tabs.

Mix Mode Theme Pool

Previously when creating a series of tabs very quickly, each new tab would conform to the same theme. Now, themes will inherit their own theme.

Motivation and Context

#66

Screenshots (if appropriate):

speed_tabs.mp4

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Non-Functional Change (non-user facing changes)

Checklist:

  • I updated the version (base package.json and re-built the master extension).
  • I updated the changelog with the new functionality.

Removes settings and logo icons from new custom tab page;
Decreases the size of doki logo in popup;
Updates Dark Ishtar theme;
Updates manifest;
Live updating when selecting a new waifu;
Adds documentation;
Removes debugs;
Waifus are now loaded to a Waifu Tab using JS instead of CSS. This is more convenient than making a CSS for each waifu;
Adds a new background script, resources.js. This script points to theme resources & provides utility functions for the other background script. It also provides an easier way to activate a theme without using a switch statement;
All of Firefox's New Tabs are replaced with a Waifu Tab.
Refactors code for more readability;
Adds more documentation comments for new classes & functions;
… purpose of each permission needed to use the extension
…ces in the popup menu dynamically with less boilerplate; Refactor code;Update permissions
…making a custom new tab HTML page for each theme, doki theme now reuses one HTML page to load all themes;

It is also easier to add more doki themes to the current collection;
Permissions file is updated with more clarification on the Storage section;
Updates manifest;
…r each tab loads correctly when opening tabs really quickly in mixed mode;

Fix Tabs not being randomized when opening tabs really quickly in mixed mode;
Fix text selection & scrollbar not loading appropriately with their individual tab in mixed mode;
Scripts loading the page theme is now more modular;
Fix initialization browser if the last option chosen was mix mode;
@ZimCodes ZimCodes linked an issue Sep 29, 2021 that may be closed by this pull request
1 task
@Unthrottled
Copy link
Member

I'll try to dedicate some time tomorrow to looking over this, because my weekend is going to be pretty busy.

I've glanced over your change notes about extra bugs. They appear to be related to mixed mode though.

@ZimCodes Have you noticed if you completely close firefox and bring it back, the background art stays, but the theme (tabs n stuff) is not applied? It's happened to me a couple of times. I'd like to get it fixed (preferable in another change request) before submitting the new themes to the firefox marketplace.

@ZimCodes
Copy link
Collaborator Author

@ZimCodes Have you noticed if you completely close firefox and bring it back, the background art stays, but the theme (tabs n stuff) is not applied? It's happened to me a couple of times. I'd like to get it fixed (preferable in another change request) before submitting the new themes to the firefox marketplace.

@Unthrottled Yes, I was able to replicate it. It is also a quick fix. Are you sure it should not be apart of this pull request?

The issue is the loadTheme function wasn't imported to the background script, so it didn't exist there.
Here's the error:
error
Here's the code. No import of loadTheme in sight.
code

Copy link
Member

@Unthrottled Unthrottled left a comment

Choose a reason for hiding this comment

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

Yes, I was able to replicate it. It is also a quick fix. Are you sure it should not be apart of this pull request?

Ah, Yeah, that fix seem simple. go for it.

Glitched tabs are tabs not added to the mixed tabs list. Since the creation of tabs are happening very quickly, and majority of operations are async (handled lazily, browser.storage.local.set) there's not enough time to be placed in the list before the page is fully loaded. This also causes problems with applying individual theming to each tab in mixed mode, causing some to be glitched.

Something I am curious about is could this be fixed if we debounce and collect tabs, then apply them all at once? Would that fix the issue? If we acted on all of the tabs created at once, we could have a complete list, then apply all the themes, once tabs stop spawning.

Example

export const CollectAndDebounce = (toDebounce, interval,) => {
  let lastTimeout = undefined;
  let collection = [];
  return (t) => {
    if (lastTimeout) {
      clearTimeout(lastTimeout);
    }

    collection.push(t);

    lastTimeout = setTimeout(() => {
      lastTimeout = undefined;
      toDebounce(collection)
      collection = [];
    }, interval);
  }
}

const debouncedMixedTabCreated = CollectAndDebounce(
  MixTabsCreated,
  250 // wait 250 ms after last tab was created
)

/*EVENT: When a new tabs are collected add a random theme to them*/
function MixTabsCreated(tabs) {
  const newThemeTabs = tabs.filter(tab.title === "New Tab");
  if(!newThemeTabs.length) return;
 // Grab themes and Update all tabs at once 
}

//........

/*Cleans up all things relating to the Mixed tab option*/
function mixTabCleanup() {
  browser.storage.local.get("mixedTabs")
    .then((storage) => {
      //Removes all listeners
      if (browser.tabs.onCreated.hasListener(MixTabCreated)) {
        browser.tabs.onCreated.removeListener(debouncedMixedTabCreated);

     /// other stuff
    });
}

/*Initialize the Mixed feature*/
function setupMixedUpdate(msg) {
  browser.tabs.query({})
    .then((tabs) => {
      browser.storage.local.get(["waifuThemes", "mixedTabs"])
        .then((storage) => {
          if (!storage.mixedTabs) {
            storage.mixedTabs = new Map();//Create a new mixed tab list
          }
          //Activate event listeners
          browser.tabs.onCreated.addListener(debouncedMixedTabCreated);//When a tab is created
          // other stuff
        });
    });
}

If we can act on all the tabs at once they are done being created, we might not need to do hackey things. (Provided tabs can all be randomly themed once we've set the mixed tabs list, which is what I gathered you stated was important to be set beforehand.)

@ZimCodes
Copy link
Collaborator Author

ZimCodes commented Oct 1, 2021

@Unthrottled Interesting. Interesting... With some modifications to this code it could work. But this callback on callback business is something else.

Update CHANGELOG.md;
Removes tab creation listener for mix mode;
clean up local mix list when disabled;
rename `mixList to `mixedList`;
Replace glitchy tab algorithm to utilize the message system approach;
`getCurrentTheme` method is now synchronous;
clean up local mix list;
@ZimCodes ZimCodes self-assigned this Oct 6, 2021
@ZimCodes
Copy link
Collaborator Author

ZimCodes commented Oct 6, 2021

The de-bounce technique did not work as the same problems persists except in a more delayed fashion now. Tabs are still not being added to the mixed tab list using this method.

The recent commits above uses the Glitchy Tab Purging Method mentioned before, which solves the unlisted tabs issue as well as the overall #72 issue. One of the problems is the photosensitive seizures you might receive from all the flashing lights & colors if you try to create tabs at the speed of light!

In any case, I was able to devise a different solution based on methods that were more... consistent. To explain this, here's a few details I've found working on this issue;
page script = themeLoader.js scripts

LoadTheme

The loadTheme function, has a chance of throwing errors if used in a page script. Once the error has been thrown, the toolbar will not be themed correctly.

Async functions

If tabs are created rapidly, some of the browser functions, specifically browser.storage.local.get, in the page script might throw.

Promise resolved after context unloaded

When this is thrown that tab becomes a glitchy tab, not added to the list.

Events

Upon setting up mix mode, the browser.tabs.onCreated event listener is add to the scene. When this event is triggered a message containing the theming instructions are sent to a designated page to be applied. Sometimes, however, the theme fails to reach the destination. This is because the browser.tabs.onCreated event listener was triggered before the page itself was fully loaded, thus resulting in not themed pages. In summary, browser.tabs.onCreated event listener can trigger before the page itself is fully loaded AND vice versa. Not consistent.

The solution takes advantage of the browser messaging functions tabs.sendMessage, runtime.sendMessage, and runtime.onMessage. The workflow ->

  1. When the page finishes loading it will send a message to the background script.
  2. The background script will respond with everything needed from browser.storage to apply the theme to the waifu page.
  3. The waifu page script will apply all the necessary themes to the current tab page except the loadTheme function, which contains theming the toolbar.
  4. loadTheme is called in the background script after sending the response to the waifu page. With this system, the tabs.onCreated event listener including the debounce and glitchy tab purging methods are no longer needed. There's also no excessive flashing lights when creating multiple tabs rapidly.

*Note: Sometimes when creating tabs rapidly, the last tab's toolbar theme will be different. Just switch to a different tab and back and it will resume back to normal. Check Video

solution.mp4

Other issue found

If in mixed mode and have multiple tabs open, if you toggle the widget switch, mix mode will deactivate and all tabs will resort to a single theme. Check Video

issue.mp4

.

@ZimCodes
Copy link
Collaborator Author

ZimCodes commented Oct 6, 2021

The new messaging system solution is now apart of this request.

@ZimCodes ZimCodes linked an issue Oct 6, 2021 that may be closed by this pull request
@ZimCodes
Copy link
Collaborator Author

ZimCodes commented Oct 6, 2021

Since #73 is:

  1. A minor bug
  2. Related to Mixed Mode
  3. Bug introduced by the new messaging system by this pull request

The issue #73's solution will be added to this pull request.

@Unthrottled
Copy link
Member

Ah! More changes!

I appreciate the hustle. Please give me till the end of this weekend to take a look over the changes.

Bummer the debounce suggestion didn't work, thanks for trying though.

@Unthrottled Unthrottled linked an issue Oct 7, 2021 that may be closed by this pull request
@Unthrottled Unthrottled mentioned this pull request Oct 10, 2021
3 tasks
Copy link
Member

@Unthrottled Unthrottled left a comment

Choose a reason for hiding this comment

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

I tested things out and there does not appear to be any regressions.

There is a bit of weirdness with switching the search widget when in mixed mode.

bandicam.2021-10-10.10-23-36-479.mp4

However I imagine it has something to do with:

*Note: Sometimes when creating tabs rapidly, the last tab's toolbar theme will be different. Just switch to a different tab and back and it will resume back to normal.

Will approve and merge the PR. If you decide to fix the above issue it can be in another change request, would like to get these changes in.

@Unthrottled Unthrottled merged commit 9302ed4 into doki-theme:master Oct 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No theme on firefox startup Widget Toggle in Mixed Mode Mixed Mode Themed Favicon Issue
2 participants