-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
thunderbird: add message filters option #6558
base: master
Are you sure you want to change the base?
Conversation
This would not have been possible without this PR as a reference That said, I think I've made some improvements giving the option a fleshed out type rather than a freeform attrset. I also went a different direction on the example/test rule. |
ee33fe0
to
b3325ae
Compare
tests/modules/programs/thunderbird/thunderbird-expected-msgFilterRules.dat
Show resolved
Hide resolved
8eb8041
to
55d7cfc
Compare
I would always prefer a submodule with freeform support and use defined known options as a guide, though. Allows us to not restrict user's if we haven't configured something explicitly and reduce maintenance burden. |
You make a good point. Would extraConfig be sufficient here? |
Add option to declare account-specific message filters.
@khaneliman I went ahead and added a text option to provide a completely custom one, or extraConfig to expand on the existing options. |
(getEmailAccountsForProfile name enabledAccountsWithId)); | ||
in (builtins.listToAttrs (map (a: { | ||
name = | ||
"${thunderbirdConfigPath}/${name}/ImapMail/${a.id}/msgFilterRules.dat"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure this is right? In testing locally with rules I see it all being generated inside the thunderbirdProfilesPath
. Seems to be a per email thing.
I have changes locally to fix some stuff and noticed that while testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this is what the prior PR had used and in my testing it does work
I'm not actually sure to be perfectly honest. Profiles path seems like it should be more right... 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, location it generates definitely doesn't look right and the filters aren't being picked up for me (testing on darwin). When I change it to point to the correct location, I see the rules.
"${thunderbirdConfigPath}/${name}/ImapMail/${a.id}/msgFilterRules.dat"; | |
"${thunderbirdProfilesPath}/${name}/ImapMail/${a.id}/msgFilterRules.dat"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious if you make these changes if it works for you. I've been doing this locally to make it function.
@@ -99,5 +106,13 @@ | |||
assertFileExists home-files/${profilesDir}/first/chrome/userContent.css | |||
assertFileContent home-files/${profilesDir}/first/chrome/userContent.css \ | |||
<(echo "* { color: red !important; }") | |||
|
|||
assertFileExists home-files/.thunderbird/first/ImapMail/${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertFileExists home-files/.thunderbird/first/ImapMail/${ | |
assertFileExists home-files/${profilesDir}/first/ImapMail/${ |
assertFileExists home-files/.thunderbird/first/ImapMail/${ | ||
builtins.hashString "sha256" "[email protected]" | ||
}/msgFilterRules.dat | ||
assertFileContent home-files/.thunderbird/first/ImapMail/${ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assertFileContent home-files/.thunderbird/first/ImapMail/${ | |
assertFileContent home-files/${profilesDir}/first/ImapMail/${ |
(getEmailAccountsForProfile name enabledAccountsWithId)); | ||
in (builtins.listToAttrs (map (a: { | ||
name = | ||
"${thunderbirdConfigPath}/${name}/ImapMail/${a.id}/msgFilterRules.dat"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, location it generates definitely doesn't look right and the filters aren't being picked up for me (testing on darwin). When I change it to point to the correct location, I see the rules.
"${thunderbirdConfigPath}/${name}/ImapMail/${a.id}/msgFilterRules.dat"; | |
"${thunderbirdProfilesPath}/${name}/ImapMail/${a.id}/msgFilterRules.dat"; |
Add option to declare account-specific message filters.
Description
Checklist
Change is backwards compatible.
Code formatted with
./format
.Code tested through
nix-shell --pure tests -A run.all
or
nix build --reference-lock-file flake.lock ./tests#test-all
using Flakes.Neither of these are working for me currently, even against master, happy to re-run if there's something I'm missingTest cases updated/added. See example.
Commit messages are formatted like
See CONTRIBUTING for more information and recent commit messages for examples.
If this PR adds a new module
Maintainer CC
@d-dervishi @hakan-demirli @ethorsoe @javaes