Re: [pve-devel] [PATCH manager 3/3] Storage GUI: Rewrite backup content view as TreePanel.

[Fabian Ebner <f.ebner@proxmox.com>]

Am 11.03.22 um 12:33 schrieb Matthias Heiserer:
> On 09.03.2022 13:39, Fabian Ebner wrote:
>> Am 04.03.22 um 12:52 schrieb Matthias Heiserer:
> 8< --------->
>> It'd be great if you could also remove the now unused stuff from the
>> ContentView base class.
>>
>> Did you think about a way to re-use this new class for the guest's
>> backup view already?
> Not yet, as I wasn't certain whether there is a need. Will do both.

If it's possible to do without too much overhead, it's certainly nicer
than the current approach of having duplicated code to keep in sync.

>>
>>> +Ext.define('pve-data-store-backups', {
>>
>> Nit: datastore is not typical for PVE, and I'd prefer singular 'backup'.
>>
>>> +    extend: 'Ext.data.Model',
>>> +    fields: [
>>> +    {
>>> +        name: 'ctime',
>>> +        date: 'date',
>>> +        dateFormat: 'timestamp',
>>> +    },
>>> +    'format',
>>> +    'volid',
>>> +    'content',
>>> +    'vmid',
>>> +    'size',
>>> +    'protected',
>>> +    'notes',
>>
>> Does not mention 'text' which is assigned later. And ContentView has
>> special handling to not display the full volid. Please also use
>> PVE.Utils.render_storage_content() for that here. Or we might want to
>> switch to using only the date for the leafs? But that needs some
>> consideration for filtering, and renamed backups which we currently
>> support..
>>
> For now, I would stay with displaying the full date and time, as it is
> the simplest solution.

I meant 'date and time' ;) Just not happy with displaying the full volid.

> 8< ---------
>> @Thomas: in PBS it's also ascending by date. Should that be changed or
>> do we switch back in PVE?
>>
>>> +        'backup-time',
>>
>> Isn't it ctime? That said, ContentView has some special handling for the
>> 'vdate' column, so maybe we should continue using that. It seems like
>> the back-end automatically sets the ctime to be the one from the archive
>> name via archive_info() in case it's a standard name, so maybe it
>> doesn't actually matter.
>>
> Yes, should be ctime.
> From what I understand, vdate in ContentView isn't something from the
> API, but just some convoluted way of displaying ctime. Functionally,
> there shouldn't be any differences.

Before pve-storage commit 9c629b3e76a6c70314a385982944fe7ca051947c, the
API didn't return the ctime calculated from the backup name. The code
for the vdate calculation is older. Using ctime from the API should be fine.

> 
> 8< ---------
>>
>>> +        let latest = c.reduce((l, r) => l.ctime > r.ctime ? l : r);
>>> +        let num_verified = c.reduce((l, r) => l + r.verification ===
>>> 'ok', 0);
>>
>> Does adding a boolean work reliably? Feels hacky.
> According to ECMA standard 7.1.4, which I believe is the relevant
> section, it works just intended and converts true to 1 and false to 0.
> 
> 8< ---------
> 
> Agree with everything else. Thanks!