firefox: split bookmarks into separate submodule file

[kira-bruneau]

Description

This splits the bookmarks submodule into a seperate file, to make it easier to maintain (like how the search module was previously split out in #5697).

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.

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

Maintainer CC

@brckd @rycee

[kira-bruneau]

The app name isn't necessary for this file, so I just removed it to make things simpler.

[brckd]

Thanks for your contribution! I like the direction you're going with splitting everything into selfcontained submodules.

I see that you renamed the bookmarks option to bookmarkSubmodule.bookmarks. I don't think we should put implementation details ("submodule") in the option name, since that's nothing the user would be concerned about. I suggest calling it bookmarks.settings instead, as that would be in line with #6389's extensions.settings.

Apart from that it looks good to me!

[brckd]

Did you add yourself as maintainer for the module?

[kira-bruneau]

I see that you renamed the bookmarks option to bookmarksSubmodule.bookmarks. I don't think we should put implementation details ("submodule") in the option name, since that's nothing the user would be concerned about. I suggest calling it bookmarks.settings instead, as that would be in line with #6389's extensions.settings.

Oh yep, that's why I kept that option as internal so it won't be exposed to the user. The alias I added for bookmarks -> bookmarkSubmodule.bookmarks should keep the existing structure, to avoid a breaking change.

Did you add yourself as maintainer for the module?

Oh nope, I'm not too familiar with the bookmarks code and I don't use it myself. I was just going through other options that could be broken out into submodules (without any existing related PRs to avoid merge conflicts). I'm not quite sure who'd be best to add here 🤔 - would it be ok to add you and @rycee?

@ethorsoe, would you be interested in being the maintainer since you originally added bookmarks support in #2355?

[kira-bruneau]

Oh wait, actually I like the idea of the submodule matching NixOS/rfcs#42.

I'll rename bookmarksSubmodule.bookmarks -> bookmarksSubmodule.settings & bookmarksSubmodule.file -> bookmarksSubmodule.configFile, but I'll still keep the alias to avoid a breaking change.

[brckd]

Oh yep, that's why I kept that option as internal so it won't be exposed to the user.

Oh, I didn't catch that! In that case it shouldn't matter what the option containing the submodule is called.

[kira-bruneau]

Hmmmm, I'm realizing that like the search submodule, this could potentially override any existing bookmarks.

I was planning on making the enable option for the search module act as the new force option, so I think it would make sense to do the same here.

That would be a breaking change though 😅 - I guess there's no point trying to avoid it.

[kira-bruneau]

Ok it should be good now!

[kira-bruneau]

I used (listOf anything) here because I couldn't find a nice way to pull out the original type from the submodule.

I don't believe this is an issue since this will just redirect you to the new submodule format.

[kira-bruneau]

Ok, I decided to add myself as a maintainer, since this change ended up becoming more than just a simple refactor, and I feel familiar enough with the code now.

Ideally it'd be nice to have a maintainer who actually uses this feature though!

I'm realizing, I might actually reconsider adding myself back as maintainer for all of firefox once we have everything organized out into smaller submodule files.

[ethorsoe]

does this change the compatibility, with existing configs and if so why is it done?

[brckd]

I was planning on making the enable option for the search module act as the new force option, so I think it would make sense to do the same here.

That would be a breaking change though 😅 - I guess there's no point trying to avoid it.

I still prefer force since IMO the module should be enabled implicitly and force be used only in case something needs to be overridden. It's also more explicit about overriding something.