From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 ; 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 ; 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 ; 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 ; 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 , Proxmox VE development discussion 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 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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