From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 868C91FF0AA for ; Tue, 03 Nov 2026 15:57:50 +0100 (CET) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 0176B21891; Tue, 03 Nov 2026 15:57:50 +0100 (CET) Message-ID: Date: Tue, 3 Nov 2026 15:57:45 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH proxmox-widget-toolkit v7 22/41] notifications: matcher: add better calendar editor To: Lukas Wagner , pbs-devel@lists.proxmox.com, pve-devel@lists.proxmox.com References: <20261103104859.172466-1-l.wagner@proxmox.com> <20261103104859.172466-23-l.wagner@proxmox.com> <8f8b7a19-02f9-4505-bad2-271631301381@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1793717865937 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.326 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: 7DCZLHNILO77QSFOI4JPOB53EGYCPSEO X-Message-ID-Hash: 7DCZLHNILO77QSFOI4JPOB53EGYCPSEO X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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!