diff mbox series

Remove space after every 80 characters in WebGUI "Logs/System Logs".

Message ID 20240803223627.123287-1-stephen@firemypi.org
State New
Headers
Series Remove space after every 80 characters in WebGUI "Logs/System Logs". |

Commit Message

Stephen Cuka Aug. 3, 2024, 10:36 p.m. UTC 
  Signed-off-by: Stephen Cuka <stephen@firemypi.org>
---

This patch addresses an issue with the IPFire WebGUI adding a space
after every 80 characters in the display of long log entries on the
"Logs/System Logs" page.  (Bug 13735)

The patch removes the "very basic breaking of lines..." code and replaces it
with a direct copy of the log entry $data to the display output variable $d.

 html/cgi-bin/logs.cgi/log.dat | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

Comments

Bernhard Bitsch Aug. 4, 2024, 10:50 a.m. UTC | #1 
Reviewed-by: Bernhard Bitsch <bbitsch@ipfire.org>
Tested-by: Bernhard Bitsch <bbitsch@ipfire.org>

Am 04.08.2024 um 00:36 schrieb Stephen Cuka:
> Signed-off-by: Stephen Cuka <stephen@firemypi.org>
> ---
> 
> This patch addresses an issue with the IPFire WebGUI adding a space
> after every 80 characters in the display of long log entries on the
> "Logs/System Logs" page.  (Bug 13735)
> 
> The patch removes the "very basic breaking of lines..." code and replaces it
> with a direct copy of the log entry $data to the display output variable $d.
> 
>   html/cgi-bin/logs.cgi/log.dat | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/html/cgi-bin/logs.cgi/log.dat b/html/cgi-bin/logs.cgi/log.dat
> index 01c382a0d..ce7de0178 100644
> --- a/html/cgi-bin/logs.cgi/log.dat
> +++ b/html/cgi-bin/logs.cgi/log.dat
> @@ -412,11 +412,12 @@ foreach $_ (@log)
>   	    $sec = 'kernel';
>   	    $data = $2.': '.$data;
>   	}
> -	my $d = substr ($data,0,80);
> -	while (length($data)>80){	#very basic breaking of lines...
> -	    $data = substr ($data,80);	#permit correct display in table cell
> -	    $d .=  ' ' . substr ($data,0,80);
> -	}
> +	#my $d = substr ($data,0,80);
> +	#while (length($data)>80){	#very basic breaking of lines...
> +	#    $data = substr ($data,80);	#permit correct display in table cell
> +	#    $d .=  ' ' . substr ($data,0,80);
> +	#}
> +	my $d = $data; #don't break lines for display
>   	my $col="";
>   
>   	if ($lines % 2) {
Adolf Belka Aug. 5, 2024, 8:01 a.m. UTC | #2 
Reviewed-by: Adolf Belka <adolf.belka@ipfire.org>

Hi Stephen,
Welcome to the list and thanks for the patch.

On 04/08/2024 00:36, Stephen Cuka wrote:
> Signed-off-by: Stephen Cuka <stephen@firemypi.org>
> ---
>
> This patch addresses an issue with the IPFire WebGUI adding a space
> after every 80 characters in the display of long log entries on the
> "Logs/System Logs" page.  (Bug 13735)
>
> The patch removes the "very basic breaking of lines..." code and replaces it
> with a direct copy of the log entry $data to the display output variable $d.
>
>   html/cgi-bin/logs.cgi/log.dat | 11 ++++++-----
>   1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/html/cgi-bin/logs.cgi/log.dat b/html/cgi-bin/logs.cgi/log.dat
> index 01c382a0d..ce7de0178 100644
> --- a/html/cgi-bin/logs.cgi/log.dat
> +++ b/html/cgi-bin/logs.cgi/log.dat
> @@ -412,11 +412,12 @@ foreach $_ (@log)
>   	    $sec = 'kernel';
>   	    $data = $2.': '.$data;
>   	}
> -	my $d = substr ($data,0,80);
> -	while (length($data)>80){	#very basic breaking of lines...
> -	    $data = substr ($data,80);	#permit correct display in table cell
> -	    $d .=  ' ' . substr ($data,0,80);
> -	}
Removing these lines seems fine to me. The code looks to be adding a 
space instead of doing an actual line break. The text is also auto word 
wrapped in the text box in the browser anyway.
> +	#my $d = substr ($data,0,80);
> +	#while (length($data)>80){	#very basic breaking of lines...
> +	#    $data = substr ($data,80);	#permit correct display in table cell
> +	#    $d .=  ' ' . substr ($data,0,80);
> +	#}
My only question is why you left all the removed lines in place as 
comments, rather than just removing them completely?
> +	my $d = $data; #don't break lines for display
I think you could also save a line by not having this line but changing 
the earlier line of
$data = $2.': '.$data;

to

my $d = $2.': '.$data;

Regards,
Adolf
>   	my $col="";
>   
>   	if ($lines % 2) {
Stephen Cuka Aug. 5, 2024, 6:47 p.m. UTC | #3 
Hi Adolf,

Thanks for your help on this.

I left the original code lines in place for context.  They're not really necessary.  If it's better, I can remove them and resubmit the patch.

For the 

my $d = $2.': '.$data;

suggestion, there is some special processing for the display for the RED section logs.

        # correct the cut position, just when section=RED
        if (($cgiparams{'SECTION'} eq 'red' ) && ($sec =~ /(kernel:)(.*)/)) {
            $sec = 'kernel';
            $data = $2.': '.$data;
        }

Using 'my $d = $2. ': ' .$data;' for everything would add the timestamp to the front of the log text displayed for all sections.

I'm not sure if the special processing for $data for RED is necessary.  I think that the point of the 'if' statement is to clean up the Section display for entries from 'kernel: ippp\d' and 'kernel: isdn.*' matches for RED, but I don't know why adding the timestamp to the front of data would be desirable in that specific case.  I don't have any log entries to reference for that though.

Thanks,
Stephen
Adolf Belka Aug. 5, 2024, 8:40 p.m. UTC | #4 
Hi Stephen,

On 05/08/2024 20:47, Stephen Cuka wrote:
> Hi Adolf,
>
> Thanks for your help on this.
>
> I left the original code lines in place for context.  They're not really necessary.  If it's better, I can remove them and resubmit the patch.

I think it would make more sense to remove them.

You will need to make it a v2 patch so it supersedes this one automatically in patchwork.

> For the
>
> my $d = $2.': '.$data;
>
> suggestion, there is some special processing for the display for the RED section logs.
>
>          # correct the cut position, just when section=RED
>          if (($cgiparams{'SECTION'} eq 'red' ) && ($sec =~ /(kernel:)(.*)/)) {
>              $sec = 'kernel';
>              $data = $2.': '.$data;
>          }
>
> Using 'my $d = $2. ': ' .$data;' for everything would add the timestamp to the front of the log text displayed for all sections.
>
> I'm not sure if the special processing for $data for RED is necessary.  I think that the point of the 'if' statement is to clean up the Section display for entries from 'kernel: ippp\d' and 'kernel: isdn.*' matches for RED, but I don't know why adding the timestamp to the front of data would be desirable in that specific case.  I don't have any log entries to reference for that though.

I hadn't seen the special processing for the RED section logs.

I would leave it as you have done it then unless Michael or Arne or Peter come back further on  this.

Let's keep it simple for now.

Regards,

Adolf.

>
> Thanks,
> Stephen
Michael Tremer Aug. 6, 2024, 3:59 p.m. UTC | #5 
Hello Stephen,

Welcome to the list!

> On 5 Aug 2024, at 19:47, Stephen Cuka <stephen@firemypi.org> wrote:
> 
> Hi Adolf,
> 
> Thanks for your help on this.
> 
> I left the original code lines in place for context.  They're not really necessary.  If it's better, I can remove them and resubmit the patch.
> 
> For the 
> 
> my $d = $2.': '.$data;
> 
> suggestion, there is some special processing for the display for the RED section logs.
> 
>        # correct the cut position, just when section=RED
>        if (($cgiparams{'SECTION'} eq 'red' ) && ($sec =~ /(kernel:)(.*)/)) {
>            $sec = 'kernel';
>            $data = $2.': '.$data;
>        }
> 
> Using 'my $d = $2. ': ' .$data;' for everything would add the timestamp to the front of the log text displayed for all sections.
> 
> I'm not sure if the special processing for $data for RED is necessary.  I think that the point of the 'if' statement is to clean up the Section display for entries from 'kernel: ippp\d' and 'kernel: isdn.*' matches for RED, but I don't know why adding the timestamp to the front of data would be desirable in that specific case.  I don't have any log entries to reference for that though.

I think we can safely drop this as we no longer support ISDN.

-Michael

> Thanks,
> Stephen
diff mbox series

Patch

diff --git a/html/cgi-bin/logs.cgi/log.dat b/html/cgi-bin/logs.cgi/log.dat
index 01c382a0d..ce7de0178 100644
--- a/html/cgi-bin/logs.cgi/log.dat
+++ b/html/cgi-bin/logs.cgi/log.dat
@@ -412,11 +412,12 @@  foreach $_ (@log)
 	    $sec = 'kernel';
 	    $data = $2.': '.$data;
 	}
-	my $d = substr ($data,0,80);
-	while (length($data)>80){	#very basic breaking of lines...
-	    $data = substr ($data,80);	#permit correct display in table cell
-	    $d .=  ' ' . substr ($data,0,80);
-	}
+	#my $d = substr ($data,0,80);
+	#while (length($data)>80){	#very basic breaking of lines...
+	#    $data = substr ($data,80);	#permit correct display in table cell
+	#    $d .=  ' ' . substr ($data,0,80);
+	#}
+	my $d = $data; #don't break lines for display
 	my $col="";
 
 	if ($lines % 2) {