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

hyprpm: Add option to notify on fail and keep original notify #8167

Merged
merged 2 commits into from
Oct 21, 2024

Conversation

KAGEYAM4
Copy link
Contributor

@KAGEYAM4 KAGEYAM4 commented Oct 18, 2024

Hyprpm fail/pass notification are mutually exclusive.

Describe your PR, what does it fix/add?

By only generating fail notifications, we reduce UI clutter.
Add additional -nn option to generate notification only on fail events.

Is it ready for merging, or does it need work?

Yes

Hyprpm fail/pass notification are mutually exclusive.
@littleblack111
Copy link
Contributor

littleblack111 commented Oct 19, 2024

i would like this too, but i think we should still keep the option for user too. check out a MR in ur fork

@KAGEYAM4
Copy link
Contributor Author

KAGEYAM4 commented Oct 19, 2024

My problem was that on Hyprland first boot i was getting notification and it was ruining the startup animation produced by swww.

I see how still keeping this can be good. But i am confused wether in UI case do we need to produce notification for pass state. Like UI should be clutter free, and generating notification on fail would be optimal.

@littleblack111 @vaxerski What you wanna do, keep both or keep one.


Also if we are going to keep both fail and pass, i thought some changes -->

  1. -nn should be for both pass & fail and -n should be for fail. Maybe similar to how we have -vv for more verbose. ( But it's going to be breaking change ) OR maybe we can think of another flag.
  2. -nn and -n help needs to be changed to specify that -nn will only produce notification for failed events. -n will for both fail&pass.
  3. Maybe we can improve the code but at the cost of complexity -->
Code

At start

          } else if (ARGS[i] == "--notify-fail" || ARGS[i] == "-nn") {
                notifyFail = true;

we could instead initialize both

 notifyFail = true; notify = true;

Then every (notify || notifyFail) can be replaced with (notify) reducing one check.

And at the end we can do

        } else if (notify && !notifyFail) {
           g_pPluginManager->notify(ICON_OK, 0, 4000, "[hyprpm] Loaded plugins");

The reason is that, user will only provide one or the other. notifyfail and notify will produce notification in failed state. Only at the end when we have to generate notification for pass state we can check if notifyFail is true or not. if its true we don't generate pass notification.

@littleblack111
Copy link
Contributor

littleblack111 commented Oct 19, 2024

or we can maybe just use the notify var but instead of bool, we can use int or char to represent those. But that would make different then other vars since they're all bool

@KAGEYAM4
Copy link
Contributor Author

KAGEYAM4 commented Oct 19, 2024

or we can maybe just use the notify var but instead of bool, we can use int or char to represent those. But that would make different then other vars since they're all bool

Yes, now we are adding two complexities. The one you suggested (int) and the one mine where we still being implicit about checking only one condition. And if we checking both condition ( var1 == <> || var2 == <>), we are back to where we started.

@vaxerski
Copy link
Member

ye but then what will nnn be for? no nut november?

jk.

-n and -nn sounds reasonable to me.

* Add option to notify on fail and keep original notify
---------

Co-authored-by: KAGEYAM4 <75798544+KAGEYAM4@users.noreply.github.com>
@KAGEYAM4 KAGEYAM4 changed the title Only generate notification on fail hyprpm: Add option to notify on fail and keep original notify Oct 21, 2024
@KAGEYAM4
Copy link
Contributor Author

@vaxerski please review

@littleblack111

This comment has been minimized.

@vaxerski
Copy link
Member

you dont need to ping me assholes I have all notifications on

@vaxerski vaxerski merged commit 5e96d73 into hyprwm:main Oct 21, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants