Re: [pve-devel] [PATCH manager] fix #4318: ui: qemu/Monitor: rework output retention mechanism

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 31/10/2022 13:53, Dominik Csapak wrote:
> instead of saving maximum 500 lines, count commands + lines, and only
> if both are over the limit, truncate the command list
> 
> this way, at least the last 10 commands + output are always visible, and
> no visible output is truncated, while still not letting the log
> grow infinitely
> 

looks ok in base idea and semantics, a few nits inline, no hard feelings for most
but the ones regarding code comments.

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/manager6/qemu/Monitor.js | 29 +++++++++++++++++------------
>  1 file changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/www/manager6/qemu/Monitor.js b/www/manager6/qemu/Monitor.js
> index d85018e6..a8ecfe97 100644
> --- a/www/manager6/qemu/Monitor.js
> +++ b/www/manager6/qemu/Monitor.js
> @@ -3,7 +3,10 @@ Ext.define('PVE.qemu.Monitor', {
>  
>      alias: 'widget.pveQemuMonitor',
>  
> -    maxLines: 500,
> +    // minimum number of commands + output saved

minimum makes no sense to me here?

> +    commandLimit: 10,
> +    // minimum number of lines saved

same here.

Maybe a more general comment talking about both params in their interaction
would be better? As otherwise this is confusing.

IOW.: Saved output is trimmed if there are *both*, more commands than 10 and
more input+output lines than 500.

> +    lineLimit: 500,

I'd up this also to a bit more, this is just text that the user actually wanted
to produce, nothing automatically generated, also, memory is quite cheap nowadays
so 2 to 5k seem like a better limit that will be still quite small compare to
all other resources of the web app.

>  
>      initComponent: function() {
>  	var me = this;
> @@ -20,7 +23,7 @@ Ext.define('PVE.qemu.Monitor', {
>  
>  	var history = [];
>  	var histNum = -1;
> -	var lines = [];
> +	var commands = [];
>  
>  	var textbox = Ext.createWidget('panel', {
>  	    region: 'center',
> @@ -45,19 +48,23 @@ Ext.define('PVE.qemu.Monitor', {
>  	};
>  
>  	var refresh = function() {
> -	    textbox.update('<pre>' + lines.join('\n') + '</pre>');
> +	    textbox.update('<pre>' + commands.flat(2).join('\n') + '</pre>');

nit: could update to template string if touching the line anyway. 

>  	    scrollToEnd();
>  	};
>  
> -	var addLine = function(line) {
> -	    lines.push(line);
> -	    if (lines.length > me.maxLines) {
> -		lines.shift();
> +	var addCommand = function(line) {

nit: arrow fn and maybe slightly different name (addComand is a bit odd, as it's
more for adding (recording?) an entry or input)

lef recordInput = line => {
    // ...
};

> +	    commands.push([line]);

would add a comment here:

// drop all lines until below limit, but always keep the output of the last 10 commands around

> +	    while (commands.length > me.commandLimit && commands.flat(2).length > me.lineLimit) {
> +		commands.shift();
>  	    }>  	};
>  
> +	var addResponse = function(lines) {
> +	    commands[commands.length - 1].push(lines);
> +	}

nit, modern arrow fn style for shorter expression:

let addResponse = line => commands[commands.length - 1].push(lines);

> +
>  	var executeCmd = function(cmd) {
> -	    addLine("# " + Ext.htmlEncode(cmd));
> +	    addCommand("# " + Ext.htmlEncode(cmd), true);
>  	    if (cmd) {
>  		history.unshift(cmd);
>  		if (history.length > 20) {
> @@ -74,9 +81,7 @@ Ext.define('PVE.qemu.Monitor', {
>  		waitMsgTarget: me,
>  		success: function(response, opts) {
>  		    var res = response.result.data;
> -		    Ext.Array.each(res.split('\n'), function(line) {
> -			addLine(Ext.htmlEncode(line));
> -		    });
> +		    addResponse(res.split('\n').map((line) => Ext.htmlEncode(line)));

nit: we normally drop the parenthesis for a single argument arrow fn,
at least if its body isn't a ternary expression.

>  		    refresh();
>  		},
>  		failure: function(response, opts) {
> @@ -102,7 +107,7 @@ Ext.define('PVE.qemu.Monitor', {
>  		    listeners: {
>  			afterrender: function(f) {
>  			    f.focus(false);
> -			    addLine("Type 'help' for help.");
> +			    addCommand("Type 'help' for help.");
>  			    refresh();
>  			},
>  			specialkey: function(f, e) {