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

Attachments should honor overflow=truncate setting (and not send more then 1) #1091

Closed
pomeloy opened this issue Apr 2, 2024 · 5 comments
Closed

Comments

@pomeloy
Copy link
Contributor

pomeloy commented Apr 2, 2024

💡 The Idea
Pushover does not support messages with multiple attachments. As a workaround, the Apprise plugin currently splits a given notification into multiple messages when passed a list of attachments. This is a great way to circumvent this limitation, but sometimes results in confusion when receiving multiple messages with more than one attachment in a short period of time.

There should be a new, optional parameter turning this behavior off. This would enhance user experience especially when using the Pushover plugin in parallel to another notification service that natively supports multiple attachments.

🔨 Breaking Feature
No, as the new parameter should be optional.

@caronc
Copy link
Owner

caronc commented Apr 6, 2024

I think you really stumbled onto somthing here. I'd like you to get credit for the fix but i don't quite think what you submitted is what we need. It's still a bit too complicated. Attached is a .patch file.
apprise-truncate-attachment.patch.gz

gunzip  /path/to/apprise-truncate-attachment.patch.gz
git apply /path/to/apprise-truncate-attachment.patch

It's based on our discussion, but a more clean lightweight version. I also added some unit tests to verify it works correctly. Please make a PR with this and i'll gladly accept it. It's a great find on your part.

This will enforce that ?upstream=truncate applied to any URL (a per-url basis) doesn't allow a chain of attachments to roll through beyond the first.

@caronc caronc changed the title Add parameter to send just one of multiple attachments when using Pushover plugin Attachments should honor overflow=truncate setting (and not send more then 1) Apr 6, 2024
@pomeloy
Copy link
Contributor Author

pomeloy commented Apr 6, 2024

Thanks so much for helping me out with the PR! I'm still not too versed in all of this.
Just to clarify: would you still define a new attachment_maxcount (or however it should be called) variable for the plugin? And with the patch the SPLIT option would not get honored concerning attachments, is that something you would still let the specific notifier service plugin handle? (such as is the case right now with the Pushover plugin)

@caronc
Copy link
Owner

caronc commented Apr 7, 2024

I would roll back and just apply the attached patch as it will solve your problem. Or just close your PR and start a fresh one.

While the is nothing wrong with your solution, it's a bit more code then what is needed to solve the problem unless there is something I'm missing?

@pomeloy
Copy link
Contributor Author

pomeloy commented Apr 8, 2024

Obviously your solution is much cleaner. I just thought that maybe you were keen on also having the overflow=split option respected when handling the attachments. Personally, your patch is totally sufficient for me.

@caronc
Copy link
Owner

caronc commented Apr 8, 2024

Closing this off as i think we nailed this! Thank you again for your issue and help! Don't ever hesitate to reach out if you find anything else! 🙂

@caronc caronc closed this as completed Apr 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants