Excluding containers from sync with RegExp

[stoically]

Introduces an input box that allows to specify a RegExp pattern which filters containers by name before storing them to sync storage.

I'm not sure how exactly the new locale strings should get introduced in the submodule, this PR points to my fork of multi-account-containers-l10n, which surely isn't how it should be done. Would I open a PR in the multi-account-containers-l10n repo?

The default pattern ^tmp\d+$ is a really narrow and specific pattern that should not interfere with regular usage of MAC, but helps Temporary Containers users to be able to also use sync without having to manually adjust preferences and potentially borking their Firefox Sync account. In the future more sophisticated solutions could be sought after, but this relieves the biggest pain point. Originally it was also planned to support the combination of MAC + TC when sync was introduced (#1611 (comment)), but it unfortunately didn't work out.

Fixes #1675
Fixes #1623
Fixes #1748
Fixes #1838
Fixes #1912
Fixes #2076
Fixes stoically/temporary-containers#371
Fixes stoically/temporary-containers#509
Fixes stoically/temporary-containers#571

@bakulf @groovecoder Would be great if could give this a look, thanks!

[FlorianWendelborn]

LGTM, surprised how minimalistic the implementation can be for this feature

[zwarag]

Hello @FlorianWendelborn. Just out of curiosity. You wrote LGTM but did not add a review. Since then the code changed, and now it's failing. Is this on purpose? I'm just curious and not trying to pressure :)

[FlorianWendelborn]

You wrote LGTM but did not add a review
— @zwarag

I did, I approved.

Since then the code changed, and now it’s failing
— @zwarag

I neither changed the code, made it fail nor have the ability to do either of this

[PixsaOJ]

This only fixes prefix "tmp" what if I use prefix "anon" ?

[stoically]

This only fixes prefix "tmp" what if I use prefix "anon" ?

You can change the regexp in the options accordingly.

[dannycolin]

Instead of filtering out any container that match a regex, why not implementing the opposite solution. What I mean is MAC could add a prefix to the containers it creates (e.g. Personal -> MAC_Personal). We could easily split the string to only show the part after the _ in the UI. And, when we sync the containers, we only do so for those starting with the MAC_ prefix. This way we don't need to add an input box that the user needs to deal.

[PixsaOJ]

How long does this take to merge?

[stoically]

@PixsaOJ There isn't any reaction from the maintainers - so likely never.

[dannycolin]

@PixsaOJ There isn't any reaction from the maintainers - so likely never.

As a contributors, I do understand your frustration that it takes more time than you'd expect to get this issue moving forward. However, I don't think comment like this will help us.

[stoically]

@PixsaOJ There isn't any reaction from the maintainers - so likely never.

As a contributors, I do understand your frustration that it takes more time than you'd expect to get this issue moving forward. However, I don't think comment like this will help us.

Didn't want to sound frustrated, sorry. Last I checked the project was only in maintenance mode, new features were mostly related to VPN stuff - and my one line change from 2018 is still open as well: #1141

Edit: Oh, and btw, I'm not sure your comment did help either - and I do understand that maintainers in the FOSS space have a though life.

[PixsaOJ]

@PixsaOJ There isn't any reaction from the maintainers - so likely never.

As a contributors, I do understand your frustration that it takes more time than you'd expect to get this issue moving forward. However, I don't think comment like this will help us.

Can you share your opinion about this feature, and how important is it to merge?

[dannycolin]

@stoically I pinged you on Matrix to see if I can help you with this.

[PixsaOJ]

Btw, if anyone wants a solution: disable sync in extension settings and install separate Container sync extension.

[dannycolin]

Looks good to me :). But could you remove src/_locales from the commit and open a PR for it on the MAC l10n repo. See https://github.com/mozilla/multi-account-containers#addupdate-messages-for-translation=.

For the translation, I'd also change:

  "syncExclude": {
    "message": "Firefox accounts sync exclude pattern:"
  },

to "Exclude containers from sync"

[stoically]

@dannycolin Thanks. Made the changes and moved the locale change to mozilla-l10n/multi-account-containers-l10n#16. The mentioned string is no longer required as it's now under the "firefox sync" heading.

[dannycolin]

Looks good to me.

[gpoole]

@dannycolin I can see you've rolled back the fix for this in #2450. Will it be back? This is a long term frustration for users of this setup so it would be awesome to have a fix in place.

[dannycolin]

Stoically is the author of the first patch. I'm still open to review a new patch for this issue but I personally won't work on it.

[gpoole]

I'm happy to work on it; I found the patch as I was submitting a PR for my own version 🤦. Is there a reason it was rolled back that needs to be fixed before it can be released?

[dannycolin]

The patch doesn't work as intented. See: #2439

[dwillis77]

Any update on when this fix might get re-implemented? This has been an issue for a long time, and it appears the reason for backing out the original fix ( #2439 ) has itself been fixed (sorry I misunderstood the first time I read through the other thread not realizing this was the cause of the other issue).

But, in any case it would be great if we could get this implemented somehow.

Thanks!

[lkraider]

Syncing temporary containers is still an issue.
@gpoole do you believe you can work on this issue?