all lists on lists.proxmox.com
 help / color / mirror / Atom feed
From: Lukas Wagner <l.wagner@proxmox.com>
To: Dominik Csapak <d.csapak@proxmox.com>,
	Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH many 00/27] overhaul notification system, use matchers instead of filters
Date: Mon, 13 Nov 2023 15:58:21 +0100	[thread overview]
Message-ID: <0fc05816-2b70-43b7-8efd-4e84cd6e651f@proxmox.com> (raw)
In-Reply-To: <a19435b0-334c-4bb9-86bd-9b3232ddba02@proxmox.com>

Hi, thanks for your input.

On 11/13/23 15:34, Dominik Csapak wrote:
> a few high level ui things
> (i did not look too deeply in the code, but i'll send
> probably some comments there too)
> 
Just as a warning, the tree code/data binding is definitely not as clean 
as it could be right now, a cleanup patch will follow. I just wanted to 
get these patches out ASAP ;)

> that probably was already there, but i find the all/any + invert 
> combination
> confusing (i had to think about it for a bit before getting a grasp on it)
> 
> i would propose we can write the four options out what they mean
> and internally convert them to all/any + invert, e.g.
> 
> 'all rule match'
> 'any rule match'
> 'at least one rule does not match' (all + invert)
> 'no rule matches' (any + invert)

I was considering this as well and discussed both approaches with Aaron.
He was slightly in favor of the one I implemented, so I went with that.
But now that I think about it again I think I like this version better, 
I'll send a followup for this (since this is is purely cosmetic).
> 
> (also is the change from and/or to all/any not a breaking change?,
> did we expose this in the api yet ?)

Well, the switch-over from filters to matchers is breaking as a whole, 
but it only ever hit pvetest, it is not a big problem.
(config parsing was adapted so that it will clean out 'old' entries)
The notification stuff was merged a bit too eagerly, this rework 
attempts to fix some of the issues with the first approach.
> 
> second, we already have a very similar interface in the guest wizard for
> the disks, and there we have the remove button inline,
> i guess we should keep the style consistent
Noted, i'll put it on my list for followups.

> 
> third, do we really need the tree? we basically have the
> four options from above, and then a list of the matches
> 
> wouldn't it be enough to seperate them?

The tree was a conscious decision, because at some point I want to 
support 'sub-matchers' - where a matcher can reference another matcher. 
that way users can compose arbitrary boolean formulas, e.g.:

   All of
     match ...
     match ...
     any of
         match ...
         match ...

For best UX, the sub-matchers shall be configurable within a single 
dialog, and this is the reason why I went with a tree widget.


> 
> e.g. have a dropdown with the four options + a list instead of a tree?
> 
> also currently the match options are not really verified before
> i can submit the form
> 

True, will fix this in a followup.

Thx again!

-- 
- Lukas




      parent reply	other threads:[~2023-11-13 14:58 UTC|newest]

Thread overview: 32+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-11-07 10:18 Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox 01/27] notify: introduce Error::Generic Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox 02/27] notify: factor out notification content into its own type Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox 03/27] notify: replace filters and groups with matcher-based system Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox 04/27] notify: add calendar matcher Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox 05/27] notify: matcher: introduce common trait for match directives Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox 06/27] notify: let a matcher always match if it has no matching directives Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-perl-rs 07/27] notify: adapt to new matcher-based notification routing Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-cluster 08/27] notify: adapt to matcher based notification system Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-guest-common 09/27] vzdump: deprecate mailto/mailnotification/notification-{target, policy} Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-ha-manager 10/27] env: switch to matcher-based notification system Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 11/27] api: notification: remove notification groups Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 12/27] api: notification: add new matcher-based notification API Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 13/27] ui: dc: remove unneeded notification events panel Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 14/27] vzdump: adapt to new matcher based notification system Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 15/27] api: apt: adapt to matcher-based notifications Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 16/27] api: replication: adapt to matcher-based notification system Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 17/27] debian: postinst: create notifications.cfg if it does not exist Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 18/27] test: fix vzdump notification test Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 19/27] ui: vzdump: remove left-overs from target/policy based notifications Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH pve-manager 20/27] ui: dc: config: show notification panel again Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-widget-toolkit 21/27] notification ui: add target selector for matcher Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-widget-toolkit 22/27] notification ui: remove filter setting for targets Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-widget-toolkit 23/27] notification ui: remove notification groups Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-widget-toolkit 24/27] notification ui: rename filter to matcher Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-widget-toolkit 25/27] notification: matcher: add UI for matcher editing Lukas Wagner
2023-11-13 15:13   ` Dominik Csapak
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-widget-toolkit 26/27] notification ui: unprotected mailto-root target Lukas Wagner
2023-11-07 10:18 ` [pve-devel] [PATCH proxmox-widget-toolkit 27/27] noficiation: matcher edit: make 'field' an editable combobox Lukas Wagner
2023-11-13 14:34 ` [pve-devel] [PATCH many 00/27] overhaul notification system, use matchers instead of filters Dominik Csapak
2023-11-13 14:54   ` Thomas Lamprecht
2023-11-13 14:58   ` Lukas Wagner [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0fc05816-2b70-43b7-8efd-4e84cd6e651f@proxmox.com \
    --to=l.wagner@proxmox.com \
    --cc=d.csapak@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal