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.

Sign up for GitHub

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

Jump to bottom

libnotify: use 3rd party patch to support replacing a notification #68407

Closed
wants to merge 1 commit into from
Closed

libnotify: use 3rd party patch to support replacing a notification #68407

wants to merge 1 commit into from
+15 −5

Conversation

peterhoeg
Copy link
Member

Motivation for this change

While the dbus notification interface supports replacing an existing notification, notify-send does not.

This uses a very simple 3rd party patch to add this functionality.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @jtojnar

Sorry, something went wrong.

@peterhoeg peterhoeg requested a review from jtojnar September 10, 2019 08:33
@jtojnar
Copy link
Member

jtojnar commented Sep 10, 2019

What is the upstream status of the patch?

@jtojnar
Copy link
Member

jtojnar commented Sep 10, 2019

In general, I think we should remain as close to upstream as possible, both to reduce maintenance effort and to avoid confusion. For that reason, I am not very fond of applying random feature patches downstream.

@peterhoeg
Copy link
Member Author

What is the upstream status of the patch?

It hasn't been submitted.

I am not very fond of applying random feature patches downstream.

I'm with you on this. I would argue though that this patch is so simple that there really isn't much of a maintenance burden.

@jtojnar
Copy link
Member

jtojnar commented Sep 10, 2019

Could you try submitting it?

@peterhoeg
Copy link
Member Author

Done - pinged you as well.

@lheckemann lheckemann added this to the 20.03 milestone Sep 12, 2019
@bjornfor
Copy link
Contributor

Can link to the upstream submission in here?

@peterhoeg
Copy link
Member Author

Sure, https://gitlab.gnome.org/GNOME/libnotify/merge_requests/10

@ofborg ofborg bot requested review from hedning and worldofpeace January 14, 2020 05:02
worldofpeace
worldofpeace suggested changes Jan 14, 2020
View reviewed changes
pkgs/development/libraries/libnotify/default.nix
Comment on lines -28 to 39
})
(
fetchpatch {
url = "https://gitlab.gnome.org/GNOME/libnotify/commit/55eb69247fe2b479ea43311503042fc03bf4e67d.patch";
sha256 = "1hlb5b7c5axiyir1i5j2pi94bm2gyr1ybkp6yaqy7yk6iiqlvv50";
}
)

# add support for replacing an existing notification
(
fetchpatch {
url = "https://launchpadlibrarian.net/105791133/print-and-replace-id-v3.patch";
sha256 = "1lfk5msv81brz7f7srxi4s4l02nasskzmpq0b9ch8hbn79z86g0b";
}
)
];
Copy link
Contributor

Choose a reason for hiding this comment

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

can we format this like how it was before? This looks like what nixpkgs-fmt tries to do which is a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, upstream is open to accepting the change there, so I'd rather handle this properly where it belongs.

@peterhoeg
Copy link
Member Author

Will be handled upstream, see: https://gitlab.gnome.org/GNOME/libnotify/merge_requests/10

@peterhoeg peterhoeg closed this Jan 20, 2020
@worldofpeace
Copy link
Contributor

Great 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace

@jtojnar jtojnar

@hedning hedning

Assignees
No one assigned
Labels
10.rebuild-darwin: 11-100 10.rebuild-linux: 101-500
Projects
None yet
Milestone
20.03
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@peterhoeg @jtojnar @bjornfor @worldofpeace @lheckemann