From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <l.wagner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits))
(No client certificate requested)
by lists.proxmox.com (Postfix) with ESMTPS id C2519EA4E
for <pve-devel@lists.proxmox.com>; Wed, 19 Jul 2023 17:25:33 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 9A2F4A09C
for <pve-devel@lists.proxmox.com>; Wed, 19 Jul 2023 17:25:03 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
[94.136.29.106])
(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256)
(No client certificate requested)
by firstgate.proxmox.com (Proxmox) with ESMTPS
for <pve-devel@lists.proxmox.com>; Wed, 19 Jul 2023 17:25:03 +0200 (CEST)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
by proxmox-new.maurer-it.com (Proxmox) with ESMTP id C8B81416E8
for <pve-devel@lists.proxmox.com>; Wed, 19 Jul 2023 17:25:02 +0200 (CEST)
Message-ID: <36d0d16a-3cdf-4a0c-f149-d247615d497c@proxmox.com>
Date: Wed, 19 Jul 2023 17:25:01 +0200
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Content-Language: de-AT
To: Dominik Csapak <d.csapak@proxmox.com>,
Proxmox VE development discussion <pve-devel@lists.proxmox.com>
References: <20230717150051.710464-1-l.wagner@proxmox.com>
<20230717150051.710464-58-l.wagner@proxmox.com>
<93843348-c48b-d86f-da04-c5d36404aa87@proxmox.com>
From: Lukas Wagner <l.wagner@proxmox.com>
In-Reply-To: <93843348-c48b-d86f-da04-c5d36404aa87@proxmox.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.100 Adjusted score from AWL reputation of From: address
BAYES_00 -1.9 Bayes spam probability is 0 to 1%
DMARC_MISSING 0.1 Missing DMARC policy
KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record
SPF_PASS -0.001 SPF: sender matches SPF record
T_SCC_BODY_TEXT_LINE -0.01 -
Subject: Re: [pve-devel] [PATCH v3 pve-manager 57/66] ui: allow to configure
notification event -> target mapping
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Wed, 19 Jul 2023 15:25:33 -0000
Hi,
On 7/19/23 14:45, Dominik Csapak wrote:
>> +
>> +Ext.define('PVE.dc.NotificationEventsTargetSelector', {
>> + alias: ['widget.pveNotificationEventsTargetSelector'],
>> + extend: 'PVE.form.NotificationTargetSelector',
>> + fieldLabel: gettext('Notification Target'),
>> + allowBlank: true,
>> + editable: true,
>> + autoSelect: false,
>> + deleteEmpty: false,
>> + emptyText: `${Proxmox.Utils.defaultText} (${gettext("mail-to-root")})`,
>> +});
>> +
>> +Ext.define('PVE.dc.NotificationEvents', {
>> + extend: 'Proxmox.grid.ObjectGrid',
>> + alias: ['widget.pveNotificationEvents'],
>> +
>> + // Taken from OptionView.js, but adapted slightly.
>
> what exactly was adapted? i can ofc diff it myself, but it would
> be nicer to have that info either in a comment or the commit message.
> also we should factor this out and reuse it in OptionView and here?
> maybe just adding it to the ObjectGrid itself?
> (if possible)
I added some more comments to explain what has been changed. However I'm not sure
if there is an easy way to reuse the changes anywhere. Basically the changes were needed
because I'm trying to render multiple rows in the ObjectGrid from a single
key in datacenter.cfg (notify), which contains the target/policy settings.
>
>> + addInputPanelRow: function(name, propertyName, text, opts) {
>> + let me = this;
>> +
>> + opts = opts || {};
(...)
>
> you should be able to reuse the render_value if you add this there also
> it can't trigger for the others anyway?
>
Originally, I did not reuse render_value because `package-updates` has a different default,
`auto` instead of `always`. However, with an additional parameter for `render_value` I was
able to consolidate both variants.
>> + break;
>> + case 'never':
>> + template = gettext('Never');
>> + break;
>> + default:
>> + template = gettext('{1} (Automatically), notify via target \'{0}\'');
>> + break;
>> + }
>> +
>> + return Ext.String.format(template, target, Proxmox.Utils.defaultText);
>> + },
>> + url: "/api2/extjs/cluster/options",
>> + items: [
>> + {
>> + xtype: 'pveNotificationEventsPolicySelector',
>
> as said above i'd simply make this a KVComboBox to indicate it's
> basically a seperate component
I actually decided to keep the separate component due to some other changes,
mainly the addition of a custom listener that shows/hides the warning textfield.
That would have led to a lot of code duplication that is now avoided by the
component.
However, I decided to move the `comboItems` to where the component is actually used.
>
>> + name: 'package-updates',
>> + fieldLabel: gettext('Notify'),
>> + comboItems: [
>> + ['__default__', Proxmox.Utils.defaultText + ' (auto)'],
>> + ['auto', gettext('Automatically')],
>> + ['always', gettext('Always')],
>> + ['never', gettext('Never')],
>> + ],
>> + },
>> + {
>> + xtype: 'pveNotificationEventsTargetSelector',
>> + name: 'target-package-updates',
>> + },
>> + ],
>> + });
>> +
>> + // Hack: Also load the notify property to make it accessible
>> + // for our render functions. initComponents later hides it.
>> + me.add_text_row('notify', gettext('Notify'), {});
>
> it should be possible to simply add it directly here with something like:
>
> me.rows.notify = {
> visible: false,
> };
>
>>
> we e.g. do something like this in dc/Optionsview and qemu/HardwareView
> (the latter is a PendingObjectGrid, but still inherits from ObjectGrid)
Thanks, that did work rather nicely. It also fixed a graphical glitch where the 'notify' row
would be visible for a split second after a page refresh.
--
- Lukas