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

Allow hiding badge counter #230

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Allow hiding badge counter #230

wants to merge 1 commit into from

Conversation

maximbaz
Copy link
Member

@maximbaz maximbaz commented Sep 9, 2020

Note this implementation doesn't just prevent entering updateMatchingPasswordsCount() function (as I personally initially expected), but only prevents doing what is hopefully the most expensive computation: getting a huge array of files and processing them.

This way, when a user toggles badge option we would refresh the badge instantly, and also if a user puts hideBadge in their .browserpass.json we would detect it within a minute. This also means that tab update handler still executes and it still refreshes settings every minute.

I would be really interested to know if this solves #224, @elprans if you could try this PR it would be much appreciated.

@erayd what do you think in general?

Fixes #224
Fixes #103
Fixes #342
Fixes #348

@erayd
Copy link
Collaborator

erayd commented Sep 11, 2020

@maximbaz Do you need me to do performance testing on this, or have you verified that bit already and just need code review?

@maximbaz
Copy link
Member Author

Performance test would be appreciated, no I don't know for sure that it solves #224

@erayd
Copy link
Collaborator

erayd commented Sep 11, 2020

Sweet, I'll test that aspect first then.

@erayd erayd added feature request New feature request bugfix Fixes something that doesn't work correctly. labels Sep 11, 2020
@maximbaz maximbaz mentioned this pull request Feb 12, 2024
@maximbaz
Copy link
Member Author

This is very requested feature recently, and multiple people have tested this PR. I've just rebased it on latest master. I think it makes sense to merge it, @erayd would you like to review this before then?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes something that doesn't work correctly. feature request New feature request
2 participants