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

Add vivaldi to chromium.nix #2483

Closed
wants to merge 1 commit into from
Closed

Conversation

robertodr
Copy link
Contributor

@robertodr robertodr commented Nov 17, 2021

Description

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.

@@ -112,9 +112,13 @@ let
google-chrome-beta = "Google/Chrome Beta";
google-chrome-dev = "Google/Chrome Dev";
brave = "BraveSoftware/Brave-Browser";
vivaldi = "vivaldi";
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not, but I left it in for consistency with the other browsers. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think it's best to only record differences from the default value (like in linuxDirs below).

linuxDirs = { brave = "BraveSoftware/Brave-Browser"; };
linuxDirs = {
brave = "BraveSoftware/Brave-Browser";
vivaldi = "vivaldi";
Copy link
Member

Choose a reason for hiding this comment

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

Same, really necessary?

@rycee
Copy link
Member

rycee commented Nov 17, 2021

Thanks for the contribution! Added a few comments.

@robertodr
Copy link
Contributor Author

I think this is ready to go.

@sumnerevans
Copy link
Member

@robertodr still need to address the two comments from @rycee about removing the unnecessary entries in darwinDirs and linuxDirs.

@@ -156,6 +158,7 @@ in {
google-chrome-dev =
browserModule pkgs.google-chrome-dev "Google Chrome Dev" false;
brave = browserModule pkgs.brave "Brave Browser" false;
vivaldi = browserModule pkgs.vivaldi "Vivaldi Browser" false;
Copy link
Member

Choose a reason for hiding this comment

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

@rycee what is the reasoning for hiding other browsers from the documentation? (the last argument to browserModule controls the visibility)

Copy link
Member

Choose a reason for hiding this comment

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

I believe it was just to avoid bloating the documentation with the same options over and over just with a slightly different attribute path. Maybe it's not too bad to allow generation of this documentation. Will have to try and see.

Copy link
Member

@berbiche berbiche left a comment

Choose a reason for hiding this comment

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

Sorry, this should have been merged a long time ago!

cc @sumnerevans

edit: After fixing the conflict in modules/programs/chromium.nix

@rycee
Copy link
Member

rycee commented Apr 17, 2022

I fixed up the remaining issues and rebased to master in 742c6cb.

@rycee rycee closed this Apr 17, 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.

None yet

4 participants