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

Allow using custom indicators for actions and URLs #1150

Closed
wants to merge 1 commit into from

Conversation

elamperti
Copy link

This PR introduces two new settings (action_indicator and url_indicator) which could be set to any string, and then be placed wherever the user finds it useful in the format string (using %A or %U).

There's also the possibility to use %D in the format to show how many duplicates a given notification has.

I've tested this locally using emoji instead of the classic "(A)" and "(U)". It works, so it should be usable with nerd fonts and similar icon fonts too.

ℹ️ As it is now, this PR is a draft to gather feedback. Things that are still to be done or defined:

  • Tests
  • Should we use a single format token (e.g. %X) to show all possible indicators in it? (actions, URLs, duplicates)
  • How could this deprecate the old implementation? (or how should they live together to avoid duplicate indicators?)
  • Get feedback from other users (specially those who participated in Custom indicators for URL and Actions #509)

@codecov-commenter
Copy link

Codecov Report

Merging #1150 (57bcb82) into master (5ad645c) will increase coverage by 0.00%.
The diff coverage is 50.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@           Coverage Diff           @@
##           master    #1150   +/-   ##
=======================================
  Coverage   66.03%   66.03%           
=======================================
  Files          46       46           
  Lines        7595     7607   +12     
=======================================
+ Hits         5015     5023    +8     
- Misses       2580     2584    +4     
Flag Coverage Δ
unittests 66.03% <50.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
test/notification.c 100.00% <ø> (ø)
src/notification.c 60.77% <50.00%> (+0.16%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@fwsmit
Copy link
Member

fwsmit commented Mar 14, 2023

Thanks for submitting this PR

Should we use a single format token (e.g. %X) to show all possible indicators in it? (actions, URLs, duplicates)

I don't think this is necessary. If you have a use case, then I wouldn't be against it.

How could this deprecate the old implementation? (or how should they live together to avoid duplicate indicators?)

show_indicators is not really useful anymore with this implementation, so it should be marked deprecated. Then we could remove it in the next major version (which will probably not come any time soon). In the mean time, show_indicators should just prepend (%A%U) or something similar to the notification text, before it's being processed by your new code. If you do that right, nothing should change for existing users, but they are able to change the indicator after that.

hide_duplicate_count=false should also just append (%D) or similar to the notification text.

@fwsmit
Copy link
Member

fwsmit commented Jul 1, 2023

Hi, have you looked at my above comment? Let's figure out together what to do to finish this PR

@elamperti
Copy link
Author

Hi, sorry for leaving this PR hanging. I'll get back to it this week. Thank you!

@fwsmit
Copy link
Member

fwsmit commented Sep 6, 2023

Hey, thanks for your update. Let me know when you get to it!

@bynect
Copy link
Member

bynect commented Oct 22, 2023

It would be nice to add a format parameter for the number of actions

@fwsmit
Copy link
Member

fwsmit commented Oct 23, 2023

Yeah, that should definitely be possible

@fwsmit
Copy link
Member

fwsmit commented Oct 23, 2023

For organizational purposes I'm going to close this pull request. If you or anyone else wants to work on this, feel free to reopen this pull request or start a new one (referencing this PR).

@elamperti
Copy link
Author

Sorry for dropping the ball. I've just opened a new PR with the missing changes + added the option to show action count using %C.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants