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

Release v3.3.2 #92

Closed
wants to merge 3 commits into from
Closed

Release v3.3.2 #92

wants to merge 3 commits into from

Conversation

LukasLen
Copy link
Owner

@LukasLen LukasLen commented Sep 9, 2024

* After changing the structure of the settings JSON it will now update to the correct structure
Copy link
Collaborator

@aryomuzakki aryomuzakki left a comment

Choose a reason for hiding this comment

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

I'm not quite understand specifically about this pr, but in src/popup/popup.js you are changing this code

    if (!result.hasOwnProperty(settingsIdentifier)) return;

into this code

    if (!result.hasOwnProperty(settingsIdentifier)) {
      browser.runtime.reload();
      return;
    }

and as I take a look, there are 2 more similar lines, lines 68 or 71, and lines 80 or 83. If this code is to reset the extension / browser data, this should also be changed right?

@LukasLen
Copy link
Owner Author

LukasLen commented Sep 19, 2024

This will reload the extension and trigger the install event, changes in those other lines are not necessarily needed as toggling the extension on/off should trigger the reload.

Copy link
Collaborator

@Tgentil Tgentil left a comment

Choose a reason for hiding this comment

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

@LukasLen, thank you for addressing the issue with the settings structure change. The updates you've made in popup.js and background.js to reload the extension when settings are missing are appropriate and enhance the functionality and I belive fixes #89 .

However, as @aryomuzakki pointed out, there are other instances in popup.js where we check for missing settings using:

if (!result.hasOwnProperty(settingsIdentifier)) return;

For consistency and to prevent users from encountering issues elsewhere in the extension, I recommend updating these instances to include the reload logic as well. By uniformly handling missing settings throughout the extension, we can provide a more robust solution and prevent potential edge cases.

That said, the pull request is ready for use and approval. The suggested changes are minor and aimed at enhancing consistency and reliability.

@Tgentil
Copy link
Collaborator

Tgentil commented Sep 20, 2024

Changes that I suggest in popup.js:

Starting from line 50:

...
// Toggle open/close blur amount settings
const showBlurSettings = (ev) => {
  ev.currentTarget.classList.toggle("active");
  ev.currentTarget.parentNode.querySelector(".collapsible").classList.toggle("show");
}
const revealButtons = document.querySelectorAll(".reveal-btn");
revealButtons.forEach((revealBtn) => {
  revealBtn.addEventListener("click", showBlurSettings)
})

// Track form save/submit for variable style settings
const forms = document.querySelectorAll("form.var-style");

forms.forEach((form) => {
  form.addEventListener("submit", saveFormSettings);
})
function saveFormSettings(ev) {
  ev.preventDefault();
  const [key, val] = Object.entries(Object.fromEntries(new FormData(ev.target)))[0];

  browser.storage.sync.get([settingsIdentifier]).then((result) => {
    if (!result.hasOwnProperty(settingsIdentifier)) {
      browser.runtime.reload();
      return;
    }
    if (key === "itBlur") {
      result.settings.blurOnIdle.idleTimeout = val;
    } else {
      result.settings.varStyles[key] = val + "px";
    }
    browser.storage.sync.set(result);
  });
}

// Load settings and update switches
browser.storage.sync.get([settingsIdentifier]).then((result) => {
  if (!result.hasOwnProperty(settingsIdentifier)) {
    browser.runtime.reload();
    return;
  }

  switches.forEach((checkbox) => {
    let id = checkbox.dataset.style;
    if (id == "on") {
      checkbox.checked = result.settings.on;
    } else if (id === "blurOnIdle") {
      checkbox.checked = result.settings?.blurOnIdle?.isEnabled;
    } else {
      checkbox.checked = result.settings.styles[id];
    }
  });

  // Set variable input values
  forms.forEach((form) => {
    const numInput = form.querySelector(`input[type="number"]`)
    const varName = numInput.dataset.varName;
    if (varName === "itBlur") {
      numInput.value = parseInt(result.settings?.blurOnIdle?.idleTimeout || 15);
    } else {
      numInput.value = parseInt(result.settings.varStyles[varName]);
    }
  })
  
});

@LukasLen LukasLen changed the title Fix settings structure change error Release v3.3.2 Nov 8, 2024
@LukasLen LukasLen closed this Nov 8, 2024
@LukasLen
Copy link
Owner Author

LukasLen commented Nov 8, 2024

Will wait for other fixes before releasing new version

@LukasLen LukasLen deleted the v3.3.1-hotfix branch November 8, 2024 14:37
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.

It stopped working in Chrome version 128.0.6613.86 (Official version) 64 bits - PT BR
4 participants