Re: [PATCH proxmox-widget-toolkit v7 22/41] notifications: matcher: add better calendar editor

[Dominik Csapak <d.csapak@proxmox.com>]

On 11/3/26 3:40 PM, Lukas Wagner wrote:
> On Tue Nov 3, 2026 at 2:56 PM CET, Dominik Csapak wrote:
>> a few high level ux things i noticed (some are pre-existing)
>>
>> same start/end time is valid (mon 12-12), not sure if that makes sense
> 
> I chose to allow it because the DailyDuration type in rust also allows
> it, meaning that there could be existing configs (although not likely)
> that have this -- I wanted to be able to properly round-trip these.
> Semantically it of course does not make much sense to create such a
> calendar matcher, but it also does not really hurt.
> 
>> ranges are only ever from mon to sun
>> e.g. checking sun,mon,tue could be sun..tue but instead it's
>> mon,tue,sun (no biggie)
> 
> Fair! Good catch. There are other cases where non-optimal timerange
> strings are produced, e.g. when checking mon,tue,wed,fri,sat,sun  --
> it then enumerates the days instead of producing two ranges
> (mon..wed,fri..sun).
> 
> Ultimately I think this is fine as is, maybe worth revisiting as a
> followup. The produced strings are 'correct' after all, just not
> optimal, if optimal is meaning 'shortest'.
> 
>>
>> one issue though: the old input accepted times like: '0:1' e.g.
>> `mon 0:1-12:2`
> 
> I'm cursing at how lenient the DailyDuration parser is, hehe.
> 
> 
>>
>> this will not be correctly parsed and hold up the window
>> though it is quetsionable if that should be accepted in the
>> first place, as it's a big ambiguous what is meant
>>
>> e.g. 12:1 -> 12:01 or 12:10?
> 
> I just checked DailyDuration, 12:1 would be in fact 12:01.
> 
> Since the improved calendar editor now uses proper timefields for the
> time input, and no pre-defined time format seems to support this
> single-digit minute format, I'd be tempted to just leave it as is. I
> don't really want to add code to special-case this (hopefully)
> unlikely format. If we get any complaints in the forum/bugtracker, we
> can still improve this later.
> 
> Existing matcher using this odd format will continue to work (since the
> format is accepted by the backend DailyDuration type), it's just that on
> the first edit of the particular rule in the tree, it would
> 'break'/marked invalid, requiring the user to re-enter the time/days.
> 
> Does this seem reasonable?


yep, fine by me :)


> 
>>
>>
>> code looks good to me
>>
> 
> Thanks for the review!