[pve-devel] [PATCH manager] fix 1928: ct snapshot: changed rollback message to warning

[Thomas Lamprecht]

On 11/16/18 10:26 AM, Tim Marx wrote:
>> Thomas Lamprecht <t.lamprecht at proxmox.com> hat am 16. November 2018 um 08:30 geschrieben:
>> On 11/13/18 1:24 PM, Tim Marx wrote:
>>> Signed-off-by: Tim Marx <t.marx at proxmox.com>
>>
>> I find the result quite nice! Some higher level notes inline.
>>
>>> ---
>>>  www/manager6/lxc/SnapshotTree.js | 11 +++++++++--
>>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/www/manager6/lxc/SnapshotTree.js b/www/manager6/lxc/SnapshotTree.js
>>> index 4996e2e3..8d76f13c 100644
>>> --- a/www/manager6/lxc/SnapshotTree.js
>>> +++ b/www/manager6/lxc/SnapshotTree.js
>>> @@ -156,11 +156,18 @@ Ext.define('PVE.lxc.SnapshotTree', {
>>>  	var rollbackBtn = new Proxmox.button.Button({
>>>  	    text: gettext('Rollback'),
>>>  	    disabled: true,
>>> +	    dangerous: true,
>>>  	    selModel: sm,
>>>  	    enableFn: valid_snapshot_rollback,
>>>  	    confirmMsg: function(rec) {
>>> -		return Proxmox.Utils.format_task_description('vzrollback', me.vmid) +
>>> -		    " '" +  rec.data.name + "'";
>>> +		var taskdescription = Proxmox.Utils.format_task_description('vzrollback', me.vmid);
>>> +		var snaptime = Ext.Date.format(rec.data.snaptime,'Y-m-d H:i:s');
>>> +		var snapname = rec.data.name;
>>> +		var msg = Ext.String.format(gettext('{0} to {1} - {2}'+
>>
>> I'd mabye transform this into '{0} to {1} ({2})' (0 the same, 1 is snapname, 2 is date)
>> and use it's own gettext for this. But no strong feelings on the format, yours is OK too.
>>
> 
> I will send a v2 anyway, because of your other comment.
> Will swap it, but actually no strong feelings either.
> 
>>> +		'<p>Note: Data between now and latest snapshot will be lost,
> '+
>>> +		'running CT will be suspended for rollback!</p>'),
>>
>> Maybe a question like:
>>
>> 'Suspend CT for rollback?' would be enough here.. less to translate and a question may
>> bring a bit more sense to the yes/no choices. What do you think?
>>
> 
> Wouldn't that make it a little bit like: 
> "If you press no we will rollback the container without suspending it?" 

hmm, true.

> Would be nonsense and anyway results in no harm, but I'm just curios if someone not that experienced would be confused.
> Maybe something like:
> "Suspend needed, suspend CT now to perform rollback?"

Not sure, sounds rather more confusing to me. Then just 'Note: Rollback suspends CT' (or stops)?
short and on point, IMO.

> 
> Don't know if we should really omit the data loss part, compared to the inexperienced example before that would be much more relevant in my opinion.

Dominik commented off-list somewhat like: 'If one rolls-back he needs to expect
the loss of the (not snapshoted) current state, so not sure if this isn't
superfluous - to much text just makes an user read no text at all, sadly ^^

> 
> But yeah, if no one else stand up for it, I will ditch it in favor for shortness.
> 
> Thanks for looking at this!
> 
> 
>>> +		taskdescription, snaptime, snapname);
>>> +		return msg;
>>>  	    },
>>>  	    handler: function(btn, event) {
>>>  		var rec = sm.getSelection()[0];
>>>
>>