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

feat: add notification body message & text min-width (#7) #8

Merged
merged 4 commits into from
Sep 25, 2024

Conversation

d3cryptofc
Copy link
Contributor

@d3cryptofc d3cryptofc commented Sep 24, 2024

Done! It solves #7 issue. Review the PR please.

Added Features:

  • Notification body message on a new line when present.

  • Notification minimum width, consequently promoting symmetry and left alignment for text.

    I used an invisible character (\u205F) at the end of the spaces to prevent hyprland from trimming the spaces and making them disappear.

1. Without Body

image

2. With Body

The notification without an icon is subtly larger than the notifications that have an icon, unfortunately I didn't find better ways to compensate for the lack of an icon.

image

Important

Hyprland default font-family must be monospace on hyprland.conf misc section. See here: https://wiki.hyprland.org/Configuring/Variables/#misc

@d3cryptofc d3cryptofc changed the title feat: shows notification body message too (#7) feat: add notification body message & text min-width (#7) Sep 24, 2024
internal/dbus.go Outdated

// Using RegExp to add padding for all lines
nf.message = regexp.
MustCompile("^\\s*|(\n)\\s*(.)").
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: you could store this regex in a package-level variable to not recompile it every time this function is ran

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I'll do it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done! 024ba8b

@codelif
Copy link
Owner

codelif commented Sep 25, 2024

Hey, Thanks for showing interest in "hyprnotify". Love the work you have done.

I wanted to add body to the text as well but never figured out a way it would fit well like this. So, I nuked it completely.

A nitpick:
Could you add "body" in the cap array in the GetServerCapabilities function?

https://specifications.freedesktop.org/notification-spec/latest/protocol.html#id-1.10.3.2.5

@codelif
Copy link
Owner

codelif commented Sep 25, 2024

As for the font-family requirement, I don't think there is a way to change it just for the notification. Otherwise we could make it automatically change on startup through IPC.

Maybe a fallback mode could be implemented if font-family is not a monospace font.

@d3cryptofc
Copy link
Contributor Author

A nitpick: Could you add "body" in the cap array in the GetServerCapabilities function?

https://specifications.freedesktop.org/notification-spec/latest/protocol.html#id-1.10.3.2.5
Supports body text. Some implementations may only show the summary (for instance, onscreen displays, marquee/scrollers)

This way?

func (n DBusNotify) GetCapabilities() ([]string, *dbus.Error) {
	cap := []string{"body"}
	return cap, nil
}

@codelif
Copy link
Owner

codelif commented Sep 25, 2024

This way?

Yes!

@d3cryptofc
Copy link
Contributor Author

Maybe a fallback mode could be implemented if font-family is not a monospace font.

Any idea for an alternative? The problem with non-monospaced fonts is that each character takes up different space in a text, the amount of padding could have to increase or decrease based on the character choices.

@codelif
Copy link
Owner

codelif commented Sep 25, 2024

Maybe a fallback mode could be implemented if font-family is not a monospace font.

Any idea for an alternative? The problem with non-monospaced fonts is that each character takes up different space in a text, the amount of padding could have to increase or decrease based on the character choices.

I am creating an issue to track this, meanwhile I am merging this (though seems like a dead-end). Thanks for your contribution!

@codelif codelif merged commit 0e3378b into codelif:main Sep 25, 2024
1 check passed
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.

3 participants