Fix review points

- Fix headers hierarchy
- Improve accessibility:
 	- set mouse pointer
	- make rows focusable
	- open on key press
	- add tooltip with "title"
	- add role and aria attributes
- Rename `ParsedLog` to `ParsedLogLine`
- Add docs to `ReversedFileReader`'s implementation of `Iterator`'s methods
- Add docs to `ParsedLogIterator`'s implementation of `Iterator`'s methods
- Remove unnecessary comment
- Add more test for parsing log lines and fix some edge cases
- Fix function name in snake-case to camelCase
- Remove `DIRECTORY_SEPARATOR`

src/Model/Log/ParsedLogIterator.php

@@ -22,10 +22,10 @@
 namespace Friendica\Model\Log;
 
 use Friendica\Util\ReversedFileReader;
-use Friendica\Object\Log\ParsedLog;
+use Friendica\Object\Log\ParsedLogLine;
 
 /**
- * An iterator which returns `\Friendica\Objec\Log\ParsedLog` instances
+ * An iterator which returns `\Friendica\Objec\Log\ParsedLogLine` instances
  *
  * Uses `\Friendica\Util\ReversedFileReader` to fetch log lines
  * from newest to oldest.
@@ -35,7 +35,7 @@ class ParsedLogIterator implements \Iterator
 	/** @var \Iterator */
 	private $reader;
 
-	/** @var ParsedLog current iterator value*/
+	/** @var ParsedLogLine current iterator value*/
 	private $value = null;
 
 	/** @var int max number of lines to read */
@@ -100,19 +100,19 @@ class ParsedLogIterator implements \Iterator
 	 * Check if parsed log line match filters.
 	 * Always match if no filters are set.
 	 *
-	 * @param ParsedLog $parsedlog
+	 * @param ParsedLogLine $parsedlogline
 	 * @return bool
 	 */
-	private function filter($parsedlog)
+	private function filter($parsedlogline)
 	{
 		$match = true;
 		foreach ($this->filters as $filter => $filtervalue) {
 			switch ($filter) {
 				case "level":
-					$match = $match && ($parsedlog->level == strtoupper($filtervalue));
+					$match = $match && ($parsedlogline->level == strtoupper($filtervalue));
 					break;
 				case "context":
-					$match = $match && ($parsedlog->context == $filtervalue);
+					$match = $match && ($parsedlogline->context == $filtervalue);
 					break;
 			}
 		}
@@ -123,13 +123,13 @@ class ParsedLogIterator implements \Iterator
 	 * Check if parsed log line match search.
 	 * Always match if no search query is set.
 	 *
-	 * @param ParsedLog $parsedlog
+	 * @param ParsedLogLine $parsedlogline
 	 * @return bool
 	 */
-	private function search($parsedlog)
+	private function search($parsedlogline)
 	{
 		if ($this->search != "") {
-			return strstr($parsedlog->logline, $this->search) !== false;
+			return strstr($parsedlogline->logline, $this->search) !== false;
 		}
 		return true;
 	}
@@ -138,8 +138,8 @@ class ParsedLogIterator implements \Iterator
 	 * Read a line from reader and parse.
 	 * Returns null if limit is reached or the reader is invalid.
 	 *
-	 * @param ParsedLog $parsedlog
-	 * @return ?ParsedLog
+	 * @param ParsedLogLine $parsedlogline
+	 * @return ?ParsedLogLine
 	 */
 	private function read()
 	{
@@ -149,22 +149,34 @@ class ParsedLogIterator implements \Iterator
 		}
 
 		$line = $this->reader->current();
-		return new ParsedLog($this->reader->key(), $line);
+		return new ParsedLogLine($this->reader->key(), $line);
 	}
 
+
+	/**
+	 * Fetch next parsed log line which match with filters or search and
+	 * set it as current iterator value.
+	 * 
+	 * @see Iterator::next()
+	 * @return void
+	 */
 	public function next()
 	{
 		$parsed = $this->read();
 
-		// if read() has not retuned none and
-		// the line don't match filters or search
-		// 	read the next line
 		while (is_null($parsed) == false && !($this->filter($parsed) && $this->search($parsed))) {
 			$parsed = $this->read();
 		}
 		$this->value = $parsed;
 	}
 
+
+	/**
+	 * Rewind the iterator to the first matching log line
+	 * 
+	 * @see Iterator::rewind()
+	 * @return void
+	 */
 	public function rewind()
 	{
 		$this->value = null;
@@ -172,16 +184,35 @@ class ParsedLogIterator implements \Iterator
 		$this->next();
 	}
 
+	/**
+	 * Return current parsed log line number
+	 * 
+	 * @see Iterator::key()
+	 * @see ReversedFileReader::key()
+	 * @return int
+	 */
 	public function key()
 	{
 		return $this->reader->key();
 	}
 
+	/**
+	 * Return current iterator value
+	 * 
+	 * @see Iterator::current()
+	 * @return ?ParsedLogLing
+	 */
 	public function current()
 	{
 		return $this->value;
 	}
 
+	/**
+	 * Checks if current iterator value is valid, that is, not null
+	 * 
+	 * @see Iterator::valid()
+	 * @return bool
+	 */
 	public function valid()
 	{
 		return ! is_null($this->value);

src/Object/Log/ParsedLogLine.php

@@ -24,7 +24,7 @@ namespace Friendica\Object\Log;
 /**
  * Parse a log line and offer some utility methods
  */
-class ParsedLog
+class ParsedLogLine
 {
 	const REGEXP = '/^(\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}[^ ]*) (\w+) \[(\w*)\]: (.*)/';
 
@@ -64,16 +64,23 @@ class ParsedLog
 
 	private function parse($logline)
 	{
+		$this->logline = $logline;
+
 		// if data is empty is serialized as '[]'. To ease the parsing
 		// let's replace it with '{""}'. It will be replaced by null later
 		$logline = str_replace(' [] - {', ' {""} - {', $logline);
 
-		// here we hope that there will not be the string ' - {' inside the $jsonsource value
-		list($logline, $jsonsource) = explode(' - {', $logline);
-		$jsonsource                 = '{' . $jsonsource;
+
+		if (strstr($logline, ' - {') === false) {
+			// the log line is not well formed
+			$jsonsource = null;
+		} else {
+			// here we hope that there will not be the string ' - {' inside the $jsonsource value
+			list($logline, $jsonsource) = explode(' - {', $logline);
+			$jsonsource                 = '{' . $jsonsource;
+		}
 
 		$jsondata = null;
-
 		if (strpos($logline, '{"') > 0) {
 			list($logline, $jsondata) = explode('{"', $logline, 2);
 
@@ -82,15 +89,20 @@ class ParsedLog
 
 		preg_match(self::REGEXP, $logline, $matches);
 
-		$this->date    = $matches[1];
-		$this->context = $matches[2];
-		$this->level   = $matches[3];
-		$this->message = trim($matches[4]);
-		$this->data    = $jsondata == '{""}' ? null : $jsondata;
-		$this->source  = $jsonsource;
-		$this->try_fix_json();
+		if (count($matches) == 0) {
+			// regexp not matching
+			$this->message = $this->logline;
+		} else {
+			$this->date    = $matches[1];
+			$this->context = $matches[2];
+			$this->level   = $matches[3];
+			$this->message = $matches[4];
+			$this->data    = $jsondata == '{""}' ? null : $jsondata;
+			$this->source  = $jsonsource;
+			$this->tryfixjson();
+		}
 
-		$this->logline = $logline;
+		$this->message = trim($this->message);
 	}
 
 	/**
@@ -101,7 +113,7 @@ class ParsedLog
 	 * This method try to parse the found json and if it fails, search for next '{'
 	 * in json data and retry
 	 */
-	private function try_fix_json()
+	private function tryfixjson()
 	{
 		if (is_null($this->data) || $this->data == '') {
 			return;
@@ -112,10 +124,15 @@ class ParsedLog
 			// try to find next { in $str and move string before to 'message'
 
 			$pos = strpos($this->data, '{', 1);
+			if ($pos === false) {
+				$this->message .= $this->data;
+				$this->data = null;
+				return;
+			}
 
 			$this->message .= substr($this->data, 0, $pos);
 			$this->data = substr($this->data, $pos);
-			$this->try_fix_json();
+			$this->tryfixjson();
 		}
 	}
 
@@ -124,7 +141,7 @@ class ParsedLog
 	 *
 	 * @return array
 	 */
-	public function get_data()
+	public function getData()
 	{
 		$data = json_decode($this->data, true);
 		if ($data) {
@@ -140,7 +157,7 @@ class ParsedLog
 	 *
 	 * @return array
 	 */
-	public function get_source()
+	public function getSource()
 	{
 		return json_decode($this->source, true);
 	}

src/Util/ReversedFileReader.php

@@ -70,6 +70,11 @@ class ReversedFileReader implements \Iterator
 		return $this;
 	}
 
+	/**
+	 * Read $size bytes behind last position
+	 * 
+	 * @return string
+	 */
 	private function _read($size)
 	{
 		$this->pos -= $size;
@@ -77,6 +82,12 @@ class ReversedFileReader implements \Iterator
 		return fread($this->fh, $size);
 	}
 
+	/**
+	 * Read next line from end of file
+	 * Return null if no lines are left to read
+	 * 
+	 * @return ?string
+	 */
 	private function _readline()
 	{
 		$buffer = & $this->buffer;
@@ -91,12 +102,24 @@ class ReversedFileReader implements \Iterator
 		}
 	}
 
+	/**
+	 * Fetch next line from end and set it as current iterator value.
+	 * 
+	 * @see Iterator::next()
+	 * @return void
+	 */
 	public function next()
 	{
 		++$this->key;
 		$this->value = $this->_readline();
 	}
 
+	/**
+	 * Rewind iterator to the first line at the end of file
+	 * 
+	 * @see Iterator::rewind()
+	 * @return void 
+	 */
 	public function rewind()
 	{
 		if ($this->filesize > 0) {
@@ -108,16 +131,34 @@ class ReversedFileReader implements \Iterator
 		}
 	}
 
+	/**
+	 * Return current line number, starting from zero at the end of file
+	 * 
+	 * @see Iterator::key()
+	 * @return int
+	 */
 	public function key()
 	{
 		return $this->key;
 	}
 
+	/**
+	 * Return current line
+	 * 
+	 * @see Iterator::current()
+	 * @return string
+	 */
 	public function current()
 	{
 		return $this->value;
 	}
 
+	/**
+	 * Checks if current iterator value is valid, that is, we readed all lines in files
+	 * 
+	 * @see Iterator::valid()
+	 * @return bool
+	 */
 	public function valid()
 	{
 		return ! is_null($this->value);

tests/src/Model/Log/ParsedLogIteratorTest.php

@@ -42,12 +42,7 @@ class ParsedLogIteratorTest extends TestCase
 
 	protected function setUp(): void
 	{
-		$logfile = dirname(__DIR__) . DIRECTORY_SEPARATOR .
-			'..' . DIRECTORY_SEPARATOR .
-			'..' . DIRECTORY_SEPARATOR .
-			'datasets' . DIRECTORY_SEPARATOR .
-			'log' . DIRECTORY_SEPARATOR .
-			'friendica.log.txt';
+		$logfile = dirname(__DIR__) . '/../../datasets/log/friendica.log.txt';
 
 		$reader    = new ReversedFileReader();
 		$this->pli = new ParsedLogIterator($reader);

tests/src/Object/Log/ParsedLogLineTest.php

@@ -21,17 +21,17 @@
 
 namespace Friendica\Test\src\Object\Log;
 
-use Friendica\Object\Log\ParsedLog;
+use Friendica\Object\Log\ParsedLogLine;
 use PHPUnit\Framework\TestCase;
 
 /**
  * Log parser testing class
  */
-class ParsedLogTest extends TestCase
+class ParsedLogLineTest extends TestCase
 {
 	public static function do_log_line($logline, $expected_data)
 	{
-		$parsed = new ParsedLog(0, $logline);
+		$parsed = new ParsedLogLine(0, $logline);
 		foreach ($expected_data as $k => $v) {
 			self::assertSame($parsed->$k, $v, '"'.$k.'" does not match expectation');
 		}
@@ -90,4 +90,94 @@ class ParsedLogTest extends TestCase
 			]
 		);
 	}
+
+	/**
+	 * test non conforming log line
+	 */
+	public function testNonConformingLogLine()
+	{
+		self::do_log_line(
+			'this log line is not formatted as expected',
+			[
+				'date'    => null,
+				'context' => null,
+				'level'   => null,
+				'message' => 'this log line is not formatted as expected',
+				'data'    => null,
+				'source'  => null,
+			]
+		);
+	}
+
+	/**
+	 * test missing source
+	 */
+	public function testMissingSource()
+	{
+		self::do_log_line(
+			'2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 {"worker_id":"ece8fc8","worker_cmd":"Cron"}',
+			[
+				'date'    => '2021-05-24T15:30:01Z',
+				'context' => 'worker',
+				'level'   => 'NOTICE',
+				'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10',
+				'data'    => '{"worker_id":"ece8fc8","worker_cmd":"Cron"}',
+				'source'  => null,
+			]
+		);
+	}
+
+	/**
+	 * test missing data
+	 */
+	public function testMissingData()
+	{
+		self::do_log_line(
+			'2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 - {"file":"Worker.php","line":786,"function":"tooMuchWorkers","uid":"364d3c","process_id":20754}',
+			[
+				'date'    => '2021-05-24T15:30:01Z',
+				'context' => 'worker',
+				'level'   => 'NOTICE',
+				'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10',
+				'data'    => null,
+				'source'  => '{"file":"Worker.php","line":786,"function":"tooMuchWorkers","uid":"364d3c","process_id":20754}',
+			]
+		);
+	}
+
+	/**
+	 * test missing data and source
+	 */
+	public function testMissingDataAndSource()
+	{
+		self::do_log_line(
+			'2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10',
+			[
+				'date'    => '2021-05-24T15:30:01Z',
+				'context' => 'worker',
+				'level'   => 'NOTICE',
+				'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10',
+				'data'    => null,
+				'source'  => null,
+			]
+		);
+	}
+
+	/**
+	 * test missing source and invalid data
+	 */
+	public function testMissingSourceAndInvalidData()
+	{
+		self::do_log_line(
+			'2021-05-24T15:30:01Z worker [NOTICE]: Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 {"invalidjson {really',
+			[
+				'date'    => '2021-05-24T15:30:01Z',
+				'context' => 'worker',
+				'level'   => 'NOTICE',
+				'message' => 'Load: 0.01/20 - processes: 0/1/6 (0:0, 30:1) - maximum: 10/10 {"invalidjson {really',
+				'data'    => null,
+				'source'  => null,
+			]
+		);
+	}
 }

view/js/module/admin/logs/view.js

@@ -1,7 +1,33 @@
-function log_show_details(id) {
+(function(){
+	function log_show_details(elm) {
+		const id = elm.id;
+		var hidden = true;
+		document
+			.querySelectorAll('[data-id="' + id + '"]')
+			.forEach(edetails => {
+				hidden = edetails.classList.toggle('hidden');
+			});
+		document
+			.querySelectorAll('[aria-expanded="true"]')
+			.forEach(eexpanded => {
+				eexpanded.setAttribute('aria-expanded', false);
+			});
+		
+		if (!hidden) {
+			elm.setAttribute('aria-expanded', true);
+		}
+	}
+
 	document
-		.querySelectorAll('[data-id="' + id + '"]')
+		.querySelectorAll('.log-event')
 		.forEach(elm => {
-			elm.classList.toggle('hidden')
+			elm.addEventListener("click", evt => {
+				log_show_details(evt.currentTarget);
+			});
+			elm.addEventListener("keydown", evt => {
+				if (evt.keyCode == 13 || evt.keyCode == 32) {
+					log_show_details(evt.currentTarget);
+				}
+			});
 		});
-}
+})();

view/templates/admin/logs/view.tpl

@@ -1,7 +1,7 @@
 <div id="adminpage">
 	<h1>{{$title}} - {{$page}}</h1>
 
-	<h3>{{$logname}}</h3>
+	<h2>{{$logname}}</h2>
 	{{if $error }}
 		<div id="admin-error-message-wrapper" class="alert alert-warning">
 			<p>{{$error nofilter}}</p>
@@ -44,14 +44,18 @@
 				</thead>
 				<tbody>
 					{{foreach $data as $row}}
-						<tr id="ev-{{$row->id}}" onClick="log_show_details('ev-{{$row->id}}')">
+						<tr id="ev-{{$row->id}}" class="log-event"
+						 role="button" tabIndex="0"
+						 aria-label="View details" aria-haspopup="true" aria-expanded="false"
+						 style="cursor:pointer;"
+						 title="Click to view details">
 							<td>{{$row->date}}</td>
 							<td>{{$row->level}}</td>
 							<td>{{$row->context}}</td>
 							<td>{{$row->message}}</td>
 						</tr>
 						<tr class="hidden" data-id="ev-{{$row->id}}"><th colspan="4">Data</th></tr>
-						{{foreach $row->get_data() as $k=>$v}}
+						{{foreach $row->getData() as $k=>$v}}
 							<tr class="hidden" data-id="ev-{{$row->id}}">
 								<th>{{$k}}</th>
 								<td colspan="3">
@@ -60,7 +64,7 @@
 							</tr>
 						{{/foreach}}
 						<tr class="hidden" data-id="ev-{{$row->id}}"><th colspan="4">Source</th></tr>
-						{{foreach $row->get_source() as $k=>$v}}
+						{{foreach $row->getSource() as $k=>$v}}
 							<tr class="hidden" data-id="ev-{{$row->id}}">
 								<th>{{$k}}</th>
 								<td colspan="3">{{$v}}</td>

view/theme/frio/css/style.css

@@ -98,6 +98,13 @@ details summary {
 	display: list-item;
 }
 
+/**
+ * clickable table rows
+ */
+.table > tbody > td[role="button"] {
+	cursor: pointer;
+}
+
 /**
  * mobile aside
  */

view/theme/frio/js/module/admin/logs/view.js

@@ -21,6 +21,12 @@ $(function(){
 	$(".log-event").on("click", function(ev) {
 		show_details_for_element(ev.currentTarget);
 	});
+	$(".log-event").on("keydown", function(ev) {
+		if (ev.keyCode == 13 || ev.keyCode == 32) {
+			show_details_for_element(ev.currentTarget);
+		}
+	});
+
 
 	$("[data-previous").on("click", function(ev){ 
 		var currentid = document.getElementById("logdetail").dataset.rowId;
@@ -37,9 +43,15 @@ $(function(){
 	});
 
 
-	function show_details_for_element(element) {
-		var $modal = $("#logdetail");
+	const $modal = $("#logdetail");
 
+	$modal.on("hidden.bs.modal", function(ev){
+		document
+			.querySelectorAll('[aria-expanded="true"]')
+			.forEach(elm => elm.setAttribute("aria-expanded", false))
+	});
+
+	function show_details_for_element(element) {
 		$modal[0].dataset.rowId = element.id;
 
 		var tr = $modal.find(".main-data tbody tr")[0];
@@ -64,6 +76,7 @@ $(function(){
 		$("[data-next").prop("disabled", $(element).next().length == 0);
 		
 		$modal.modal({})
+		element.setAttribute("aria-expanded", true);
 	}
 
 	function recursive_details(s, data, lev=0) {

view/theme/frio/templates/admin/logs/view.tpl

@@ -1,7 +1,7 @@
 <div id="adminpage">
 	<h1>{{$title}} - {{$page}}</h1>
 
-	<h3>{{$logname}}</h3>
+	<h2>{{$logname}}</h2>
 	{{if $error }}
 		<div id="admin-error-message-wrapper" class="alert alert-warning">
 			<p>{{$error nofilter}}</p>
@@ -59,7 +59,9 @@
 			</thead>
 			<tbody>
 				{{foreach $data as $row}}
-				<tr id="ev-{{$row->id}}" class="log-event"
+				<tr id="ev-{{$row->id}}" class="log-event" 
+					role="button" tabIndex="0"
+					aria-label="View details" aria-haspopup="true" aria-expanded="false"
 					data-data="{{$row->data}}" data-source="{{$row->source}}">
 					<td>{{$row->date}}</td>
 					<td class="