[Notifications] Don't show do not disturb end date beyond 100 days
ClosedPublic

Authored by broulik on Apr 11 2020, 10:50 AM.

Details

Summary

When setting "until turned off" it effectively sets it to today in a year. Now with leap year this appears to have broken.
Instead, just use 100 days, which is plenty, even if we come up with a "for 1 month" option maybe.

Test Plan

5.18

  • No longer shows "do not disturb until 2021-04-11" when I choose "until turned off"

Diff Detail

Repository
R120 Plasma Workspace
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
broulik created this revision.Apr 11 2020, 10:50 AM
Restricted Application added a project: Plasma. · View Herald TranscriptApr 11 2020, 10:50 AM
Restricted Application added a subscriber: plasma-devel. · View Herald Transcript
broulik requested review of this revision.Apr 11 2020, 10:50 AM
pino added a subscriber: pino.Apr 11 2020, 10:56 AM

Wouldn't be better instead to not display a date in case the "until turned off" option is chosen? I personally found it confusing the first time I chose this option, and yet a date was shown.

That is exactly what it should do and it did originally but I just noticed it still showed a date, presumable for leap year reasons. With this patch no date is shown again when "until turned off" is chosen.

broulik updated this revision to Diff 79818.Apr 11 2020, 11:01 AM
  • Also change tooltip

Note to future me: put this logic in a shared location

bport added a subscriber: bport.Apr 20 2020, 9:12 AM

According to settings.h documentation
When invalid or in the past, do not disturb mode should be considered disabled.

So why not setting date to invalid date or one year in the past and do a test if date is in the future

This is for displaying whether do not disturb is on. The "until disabled" option just sets it to one year in the future. But it's odd to show "until 2021", so we don't show the date when it's too far in the future.

bport added a comment.Apr 20 2020, 9:15 AM

This is for displaying whether do not disturb is on. The "until disabled" option just sets it to one year in the future. But it's odd to show "until 2021", so we don't show the date when it's too far in the future.

Why not change what the option do ?

Why not change what the option do ?

so how would you reflect a "forever" as a date?

ngraham added a subscriber: ngraham.May 2 2020, 5:43 PM

Alternative proposal: since showing any date in the UI will be weird, how about we say "until turned off" instead? That would mirror the text in the menu item.

Technically we don't have a "until turned off" state, just a date.

I know, but that's what the menu item says.

I'm saying we should make then consistent somehow: either both should say "until turned off" (even if that's a lie) or they should both say "for a year", or "for three months", or whatever.

I find "for a year" odd. Also, this is a bug fix, so can we get this in or not. Changing strings can then be done in master, if we want to.

ngraham accepted this revision.May 4 2020, 1:56 PM

All right let's do that on the stable branch to fix the bug and then change the string in master to be consistent.

This revision is now accepted and ready to land.May 4 2020, 1:56 PM
This revision was automatically updated to reflect the committed changes.