search

NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.07k stars 14.08k forks source link

bug: `xdg.portal.config.sway.default` should be list #352188

Open midirhee12 opened 6 days ago

midirhee12 commented 6 days ago

Describe the bug

Fix #348792 as described here.

@teutat3s

Add a :+1: reaction to issues you find important.

midirhee12 commented 6 days ago

General practice in the future. If there's mkDefault, there's generally a reason for it.

SuperSandro2000 commented 6 days ago

Using mkDefault in a list is often wrong, as specifying the value will overwrite everything.

SuperSandro2000 commented 6 days ago

You meant to link https://github.com/NixOS/nixpkgs/pull/348792#issuecomment-2445299968 , right?

That doesn't explain why mkDefault should be used, it is quite the opposite.

teutat3s commented 6 days ago

@midirhee12 Is there any error you are seeing? I'm typing this from sway with the change in the PR you linked and screensharing, camera and FF idle inhibit all work as expected. What functionality is missing for you? lib.mkForce is your friend btw.

midirhee12 commented 6 days ago

Using mkDefault in a list is often wrong, as specifying the value will overwrite everything.

What would you consider as the more idiomatic way to set defaults then? Still, it should be overridable without having to mkForce some how.

midirhee12 commented 6 days ago

@midirhee12 Is there any error you are seeing? I'm typing this from sway with the change in the PR you linked and screensharing, camera and FF idle inhibit all work as expected. What functionality is missing for you? lib.mkForce is your friend btw.

For example, in a project of mine, kde is needed for some Qt applications. Recently, we one of our contributors had to make a fork and change this to mkForce which they are about to PR. With that said, I think this is far from what should be default.

But regardless, this introduced a breaking change and shouldn't have been backported to a stable release.

midirhee12 commented 6 days ago

@SuperSandro2000 I'll change the issue title once you let me know what is the more idiomatic way to go about this since mkDefault is probably not good for lists.

midirhee12 commented 6 days ago

Ohhhhh, you're saying you can just use merge semantics! Yeah, you're right. That would be much better.