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

Add notification on daily challenge conclusion & start of new one #29188

Merged
merged 9 commits into from
Jul 31, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented Jul 29, 2024

2024-07-29.14-27-39.mp4

Because I wish to stop seeing "DAILY CHALLENGE WHERE" every day on #general.

The notifications are constrained to the daily challenge screen only to not spam users who may not care.

Because I wish to stop seeing "DAILY CHALLENGE WHERE" every day
on #general.

The notifications are constrained to the daily challenge screen only to
not spam users who may not care.
@Walavouchey
Copy link
Member

idk how useful the notification is if you're supposed to be looking at the daily challenge screen anyway (and see the beatmaps switch)

@bdach
Copy link
Collaborator Author

bdach commented Jul 29, 2024

The beatmap doesn't switch if you stay on the screen.

Comment on lines 430 to 437
var roomRequest = new GetRoomRequest(change.NewValue.Value.RoomID);

roomRequest.Success += room =>
{
waitForNextChallengeNotification?.Close(false);
notificationOverlay?.Post(new NewDailyChallengeNotification(room));
};
API.Queue(roomRequest);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Thoughts on having this notification appear even if you aren't on the daily challenge screen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See last line of OP:

The notifications are constrained to the daily challenge screen only to not spam users who may not care.

I'd rather avoid accusations of us 'railroading' users into this feature, but maybe I'm overly sensitive.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

fair i guess. we'll see if people request it :)

Copy link
Member

Choose a reason for hiding this comment

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

Maybe only show for those who participated?

The current behavior doesn't work for people who log in straight at 0 UTC, when the button doesn't show.

Copy link
Sponsor Member

@peppy peppy Jul 30, 2024

Choose a reason for hiding this comment

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

I'm honestly thinking the vast majority of users are not going to be annoyed by a notification once a day at a specific time of day and we should just go for it and wait for complaints, rather than the other way around.

Obviously only for the "new daily challenge" notification, not the other one.

I'm also fine with a toggle for this being added, something like "Receive notifications for game-wide events"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok well I'll have a go I guess, although now that I think about it figuring out a place to put the notification logic may prove to be a right pain in the ass.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've done this in 1b57a2a. No toggle for now unless you think it is imperative for one to exist.

Not sure you'll like the placement, but it does work globally because of some implementation details (even if the button is offscreen, the bindable change callbacks fire, because that stuff is being invoked from MetadataClient which is everpresent).

@Layendan
Copy link
Contributor

Layendan commented Jul 29, 2024

I would love to have the notification being available everywhere!

If you don't want to spam people who don't care (which I think is fine since it's just 2 notifications only if they're on during that moment), a setting that would allow them to turn it off would be nice.

Edit: maybe a good compromise would be show the notification for the daily challenge ending only on the daily challenge screen, while having the daily challenge starting notification be global.

@peppy peppy self-requested a review July 30, 2024 10:00
@peppy
Copy link
Sponsor Member

peppy commented Jul 31, 2024

@bdach can you confirm you're okay with the changes I made? if so i'll bundle this in a release today.

@bdach
Copy link
Collaborator Author

bdach commented Jul 31, 2024

They seem fine looking at source.

@peppy peppy merged commit e9ed9ff into ppy:master Jul 31, 2024
9 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants