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

xdg-mime: add module #905

Closed
wants to merge 1 commit into from
Closed

xdg-mime: add module #905

wants to merge 1 commit into from

Conversation

Kha
Copy link
Contributor

@Kha Kha commented Nov 11, 2019

This is a direct translation of the NixOS module.

If enabled, this module will create the XDG mime database
and desktop file database caches from programs installed
via home-manager. The 'XDG_DATA_DIRS' environment variable
is extended so that programs like 'xdg-open' can find and
make use of these databases.

Not sure if it makes sense to add a test here; the files are auto-generated, big, and one of them even uses a binary format.

@Kha
Copy link
Contributor Author

Kha commented Nov 11, 2019

Note that using xdg-open specifically with this depends on NixOS/nixpkgs#72426

Copy link
Member

@rycee rycee left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I've added a few comments.

];

home.sessionVariables = {
XDG_DATA_DIRS = "$XDG_DATA_DIRS:${config.home.profileDirectory}/share";
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 this belongs in nix.sh or some place where it is set for all people using Nix on non-NixOS, not just people using Home Manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to agree; unfortunately Eelco seems to think otherwise: NixOS/nix#2443
Perhaps the simplest solution here would be to wrap just xdg-open etc. instead in nixpkgs, analogously to NixOS/nixpkgs#54525?

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough, but it should only be set for people on non-NixOS since otherwise the ${config.home.profileDirectory}/share will show up twice. I would suggest to remove this for now and I'll make sure to re-add it in a later commit once I've figured out how to do it nicely. I would also place this somewhere else, perhaps in misc/xdg.nix since the variable isn't specifically related to xdg-mime.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, removed. I'll open a PR against xdg-open in nixpkgs to include XDG_DATA_DIRS there for everyone.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure how to do this in a nice way in a xdg-open wrapper since you can't at package build time know where the user profile will be found.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, you're right of course.

message = ''
A new module is available: 'xdg.mime'.

If enabled, this module will create the XDG mime database
Copy link
Member

Choose a reason for hiding this comment

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

Should indicate that the module is enabled by default. Perhaps something like

Suggested change
If enabled, this module will create the XDG mime database
If enabled, which it is by default, this module will create the XDG mime database

suffices?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. I think I wasn't sure whether it should even be enabled by default, but it seems you don't mind. The commands' execution time does seem to be negligible.

Copy link
Member

Choose a reason for hiding this comment

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

My spontaneous reaction was that it should be disabled by default but I think it should be fine since the NixOS module seems to be enabled by default and I haven't heard any complaints about that.

Copy link
Contributor Author

@Kha Kha left a comment

Choose a reason for hiding this comment

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

Thank you for the in-depth review!

@rycee
Copy link
Member

rycee commented Dec 1, 2019

Thanks! I've rebased this to master in 571989f.

@rycee rycee closed this Dec 1, 2019
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.

2 participants