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

thunderbird: support setting search engines #5697

Merged
merged 2 commits into from
Oct 14, 2024

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented Aug 1, 2024

Description

  • Splits firefox search engine configuration into a separate submodule so it can be reused in thunderbird
  • Removes myself maintainer for all of firefox, and adds myself as a maintainer for just the search engine submodule

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all or nix develop --ignore-environment .#all using Flakes.

  • Test cases updated/added. See example.

    Submodule already tested in firefox-profile-search tests.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

Maintainer CC

Firefox:
@rycee @brckd

Thunderbird:
@d-dervishi

@github-actions github-actions bot added the mail label Aug 1, 2024
@kira-bruneau kira-bruneau force-pushed the thunderbird-search-engines branch 2 times, most recently from 0cbe471 to 85bcdc2 Compare August 1, 2024 05:45
@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Aug 1, 2024

I was also thinking of splitting out generating user.js & chrome from firefox so changes there would also be shared in thunderbird (see #3808, #5077, #5670), but decided to keep this focused on search for now to simplify the review.

@kira-bruneau
Copy link
Contributor Author

I also just pushed a change that also organizes the firefox tests by submodule.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Aug 1, 2024

Actually, I just decided to split off test refactoring into it's own PR, because the change is easier to review separately: #5698

@kira-bruneau kira-bruneau force-pushed the thunderbird-search-engines branch 2 times, most recently from 8ff99b4 to b646398 Compare August 1, 2024 20:19
@github-actions github-actions bot added mail and removed mail labels Aug 1, 2024
Copy link
Contributor

@brckd brckd left a comment

Choose a reason for hiding this comment

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

Thanks @kira-bruneau! No issues from my side, since it doesn't affect the functionality of the Firefox module. The formatting only conflicts with #5685 so far, which would become your responsibility when you become the maintainer of this module.

Comment on lines +146 to +151
enable = mkOption {
type = with types; bool;
default = config.default != null || config.privateDefault != null
|| config.order != [ ] || config.engines != { };
internal = true;
};
Copy link
Contributor Author

@kira-bruneau kira-bruneau Aug 3, 2024

Choose a reason for hiding this comment

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

I added this internal enable option to make the implicit enable behaviour reusable, but I think ideally users should be required to set enable explicitly. I just didn't want to introduce any breaking changes here.

I plan on folding the force option into enable in a follow-up PR.

teto pushed a commit that referenced this pull request Oct 7, 2024
Split off from #5697, organizes firefox tests by submodule.

This is intended to match directory structure setup for the new search submodule.
Mikilio pushed a commit to Mikilio/home-manager that referenced this pull request Oct 11, 2024
Split off from nix-community#5697, organizes firefox tests by submodule.

This is intended to match directory structure setup for the new search submodule.
@teto teto merged commit e1aec54 into nix-community:master Oct 14, 2024
3 checks passed
@khaneliman
Copy link
Contributor

khaneliman commented Oct 14, 2024

Getting this after this commit. Both files referenced with different definitions are the firefox module in home-manager.

error: The option `home-manager.users.khaneliman.home.file.".mozilla/firefox/khaneliman/search.json.mozlz4".enable' has conflicting definition values:
       - In `/nix/store/i9vkk0yd40vsvn73a1rfi60p2p0dhc8m-source/modules/programs/firefox.nix': true
       - In `/nix/store/i9vkk0yd40vsvn73a1rfi60p2p0dhc8m-source/modules/programs/firefox.nix': false

@teto
Copy link
Collaborator

teto commented Oct 14, 2024

before merging, I managed to rebuild firefox with custom search engines and started thunderbird (that I dont use ) fine hence the merge. I apaprently missed some stuff.

@khaneliman
Copy link
Contributor

Hmm.. I can double check my config.. but I didn't spend too much time looking originally because both files referenced were an internal home-manager module.

@khaneliman
Copy link
Contributor

khaneliman commented Oct 17, 2024

Appears to be issue with multiple profiles sharing a path. One profile doesn't set the search option, the other uses it. This conflict is thrown.

      profiles = {
        "dev-edition-default" = {
          id = 0;
          path = "${config.${namespace}.user.name}";
        };

        ${config.${namespace}.user.name} = {
          inherit (cfg) extraConfig extensions search;
          inherit (config.${namespace}.user) name;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants