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

firefox: support setting search engine & adding custom engines #2984

Merged
merged 1 commit into from
Oct 22, 2022

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented May 29, 2022

Description

With this change, it's now possible to configure the default search engine in Firefox with programs.firefox.profiles.<name>.search.default and add custom engines with programs.firefox.profiles.<name>.search.engines.

It's also recommended to enable programs.firefox.profiles.<name>.search.force = true since Firefox
will replace the symlink for the search configuration on every launch, but note that you'll loose any existing configuration by enabling this.

Here's an example of how I'm using this in my home-manager config: https://gitlab.com/kira-bruneau/home-config/-/blob/028c1a50c82f8cad2be3d6693e6c53c67bf7e7c2/package/firefox/default.nix#L9-93.

NOTE: The hash verification is only required for custom search engines, it's ignored when setting your default engine to a builtin engine, but I set this up to still generate the hash anyway to be consistent with Firefox.

Closes #2318 @pshirshov

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • 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.

  • Added myself and the module files to .github/CODEOWNERS.

@kira-bruneau kira-bruneau requested a review from rycee as a code owner May 29, 2022 03:32
@kira-bruneau kira-bruneau force-pushed the firefox-search branch 3 times, most recently from 98d25b1 to 16dc28a Compare May 29, 2022 19:21
@pshirshov
Copy link

Excited to see this, thank you! Any chance it can be merged soon?

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented May 31, 2022

I decided to rename the enable option to force and make it optional, so it's clearer that it isn't "enabling" search, but overriding the existing search config that Firefox writes.

@pshirshov
Copy link

Is there any blocker for this to be merged? :(

@pshirshov
Copy link

@peterhoeg

@pshirshov
Copy link

@rycee , sorry about bothering you, but could you have a look at this please?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/firefox-search-engine-shortcuts/22177/5

modules/programs/firefox.nix Outdated Show resolved Hide resolved
modules/programs/firefox.nix Outdated Show resolved Hide resolved
modules/programs/firefox.nix Outdated Show resolved Hide resolved
@pshirshov
Copy link

@Atemu , @ncfavier : any chance you may accept it?

@pshirshov
Copy link

@kira-bruneau seems like the P/R doesn't work on macOS:

error: Package ‘mozlz4a-2018-08-23’ in /nix/store/qr489cwcx2l5b9m234g5ipqxan0mr7fd-source/pkgs/tools/compression/mozlz4a/default.nix:27 is not supported on ‘x86_64-darwin’, refusing to evaluate.

Though I don't see anything linux-specific in the python script (formula, script), do you think you may just incorporate that script into your PR?

@pshirshov
Copy link

NixOS/nixpkgs#196018

@pshirshov
Copy link

NixOS/nixpkgs#196021 - I guess that may help

@Atemu
Copy link

Atemu commented Oct 14, 2022

@pshirshov neither of us are maintainers here; we only provided feedback on the code.

@pshirshov
Copy link

@rycee : please...

@rycee rycee force-pushed the firefox-search branch 2 times, most recently from 5c956ea to 71397e5 Compare October 22, 2022 18:25
With this change, it's now possible to configure the default search
engine in Firefox with

  programs.firefox.profiles.<name>.search.default

and add custom engines with

  programs.firefox.profiles.<name>.search.engines.

It's also recommended to enable

  programs.firefox.profiles.<name>.search.force = true

since Firefox will replace the symlink for the search configuration on
every launch, but note that you'll loose any existing configuration by
enabling this.
@rycee rycee merged commit 69d19b9 into nix-community:master Oct 22, 2022
@rycee
Copy link
Member

rycee commented Oct 22, 2022

Thanks! Fixed a few minor issues and merged to master now.

I think some of the code could benefit by having more let bindings to make it a bit more understandable, see e.g. the definition of orderedEngineList, but it's OK for now.

@pshirshov
Copy link

To be honest, even as it is it's totally understandable and it's a real game changer. Many thanks!

@rycee
Copy link
Member

rycee commented Oct 22, 2022

Indeed, this is a very nice feature and @kira-bruneau did a great job implementing it! So, I hope my comment above does not seem too negative, it was meant purely as constructive criticism.

@kira-bruneau
Copy link
Contributor Author

@rycee Yeah... I was worried it was too messy / hard to follow 😅, but there's also just a lot going on to get this to work. I'll try to clean it up some more. Would it make sense to try to split this into a separate file?

@rycee
Copy link
Member

rycee commented Oct 24, 2022

@kira-bruneau No need for a separate file, I think. It seems to me the thing that would help readability the most in this case is "naming things".

For example, instead of having, loosely modeled on orderedEngineList,

imap (i: x: long expression) xs ++ map (y: long expression) (expression)

one could try doing something like

let
  descriptiveName1 = i: x: long expression;
  descriptiveName2 = y: long expression;
  descriptiveName3 = expression;
in
  imap descriptiveName1 xs ++ map descriptiveName2 descriptiveName3

the long expressions themselves may benefit from let bindings, in which case the variable could be placed either within the function definition or on the same level as descriptiveNameX. Typically I would move the definition out of the function unless it uses a function argument.

hpfr added a commit to hpfr/system that referenced this pull request Nov 14, 2022
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.

Support search.json.mozlz4 customization
6 participants