Problem/Motivation

The unified fields got line numbers.

I fear that the Strip Tags filter could change line numbers.

Proposed resolution

We need to check if line numbers are preserved.

If not, we should better remove the line numbers when applying that filter.

Or we could split source by newline and run split_tags per line.
This however would break tags that are spread across multiple tags.
Changes are, that at least if a tag is multiline, the lines are dropped.

I would add test coverage and decide for a simple solution here.
In a follow-up we can discuss if there is a way to implement strip_tags on our own with preserving line numbers.

User interface changes

Only show line numbers if they are correct.

CommentFileSizeAuthor
#18 hide_line_numbers-2802835-18.patch4.55 KBginovski
#18 interdiff-2802835-16-18.txt675 bytesginovski
#16 hide_line_numbers-2802835-16.patch4.81 KBginovski
#14 hide_line_numbers-2802835-13.patch7.99 KBginovski
#14 interdiff-2802835-7-13.txt7.96 KBginovski
#10 hide_line_numbers_when-2802835-10-array-slice.patch3.27 KBginovski
#10 hide_line_numbers_when-2802835-10-if-else.patch7.36 KBginovski
#7 hide_line_numbers.png12.77 KBginovski
#7 hide_line_numbers_when-2802835-7.patch3.11 KBginovski
#7 interdiff-2802835-4-7.txt3.24 KBginovski
#4 strip_tags.png12.6 KBginovski
#4 hide_line_numbers_when-2802835-4.patch1.76 KBginovski

Comments

miro_dietiker created an issue. See original summary.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented

Since striping tags divide the text in multiple times it sometimes hide some lines that haven't changed, thus the line numbers change. I would vote for removing them when striping tags.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

Like i proposed:
We hide it in this issue, because we know about the problem.

Then we open a follow-up to implement a workaround that guarantees the line number stability. We can do this, but it's a larger issue.

ginovski’s picture

ginovski commented
Assigned: Unassigned » ginovski
Status: Active » Needs review
StatusFileSize
new hide_line_numbers_when-2802835-4.patch1.76 KB
new strip_tags.png12.6 KB

Added a flag $strip_tags_active, and checking for it when adding line numbers.
Screenshot provided.

tduong’s picture

tduong commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Looks fine, even though ...

+++ b/src/Plugin/diff/Layout/UnifiedFieldsDiffLayout.php
@@ -156,11 +157,11 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+              'data' => $show && !$strip_tags_active ? $row_count_right : NULL,

... I'm not sure if php explicitly needs "()" for this kind of if-statement

Then although in the proposed solution is written:

I would add test coverage and decide for a simple solution here.
In a follow-up we can discuss...

the simple solution is more or less defined, but we still need a test I guess.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

@tduong See PHP Operator Precedence, yes this works.
http://php.net/manual/en/language.operators.precedence.php
I'm not a big fan of those complex && ! ? : and they are often source of unexpected behavior...

+++ b/src/Plugin/diff/Layout/UnifiedFieldsDiffLayout.php
@@ -156,11 +157,11 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+              'data' => $show && !$strip_tags_active ? $row_count_right : NULL,

I'm asking myself what $show means (better variable name?) and if we should negate the second such as $raw_active.
BTW Why is this $show not applicable to the right side?

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2802835-4-7.txt3.24 KB
new hide_line_numbers_when-2802835-7.patch3.11 KB
new hide_line_numbers.png12.77 KB

$show is from another issue #2793029, it is used to show the same line number on both columns if the line is the same for both revisions.
It compares the line from both revisions, and if they are the same, the left column has its own line number, and the right is increased by 1.
If $show doesn't apply, then only the left column shows the right number of the line, and the right column would be either empty or showing wrong number.
1. I added a test coverage in the DiffRevisionTest, asserting that the line numbers columns are empty.
2. Renamed the variable from strip_tags_active to raw_active and changed usage properly.

I am not sure whether I should remove both columns when 'strip_tags' filter is applied, or keep them empty like this?

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented

I would vote to remove the columns completely. :)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Needs review » Needs work

Yeah. It moves columns around, but definitively, remove them.
Don't forget to adjust the headers.

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new hide_line_numbers_when-2802835-10-if-else.patch7.36 KB
new hide_line_numbers_when-2802835-10-array-slice.patch3.27 KB

Added 2 ways to remove the columns:
1. if/else
2. array_slice with a helper $row_counter.

I think 2. seems more clean.

The last submitted patch, 10: hide_line_numbers_when-2802835-10-if-else.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 10: hide_line_numbers_when-2802835-10-array-slice.patch, failed testing.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented 
+++ b/src/Plugin/diff/Layout/UnifiedFieldsDiffLayout.php
@@ -146,6 +147,7 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+      $row_counter = 0;

@@ -173,6 +175,10 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+            $final_diff[$row_counter] = array_slice($final_diff[$row_counter],2,2);

@@ -196,6 +202,10 @@ class UnifiedFieldsDiffLayout extends DiffLayoutBase {
+              $final_diff[$row_counter] = array_slice($final_diff[$row_counter],2,2);
...
+            $row_counter++;

You implicitly expect that the index $row_counter is identical to what was used before. If you introduce such a key, you will need to use it everywhere (replace []).

array_slice lacks two spaces after comma.

Also as i reminded in #9, you only modify table content rows, not the headings.

So yeah, slicing seems to work well here.
We could also simply switch to named column keys, like i proposed in #2785857: Introduce header / row table keys - then using conditional unset() would be very easy.

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2802835-7-13.txt7.96 KB
new hide_line_numbers-2802835-13.patch7.99 KB

Forgot to configure the tests in the last patch, ignore #10.
Now I added:
1. If/else condition to write/not write first 2 columns with line numbers
2. Configured the test functions properly.

Status: Needs review » Needs work

The last submitted patch, 14: hide_line_numbers-2802835-13.patch, failed testing.

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new hide_line_numbers-2802835-16.patch4.81 KB

Okay, now with array_slice, ignore #14.
1. Configured tests.
2. Added $row_counter as index in the array
3. In the header I only changed the colspan,

Status: Needs review » Needs work

The last submitted patch, 16: hide_line_numbers-2802835-16.patch, failed testing.

ginovski’s picture

ginovski commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2802835-16-18.txt675 bytes
new hide_line_numbers-2802835-18.patch4.55 KB

Reversed small mistake, should be ok now.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Status: Needs review » Reviewed & tested by the community

wow looks nice now. btw #2800155: Flood of warnings after updating to Diff 8.x-1.0-alpha5 should go in first. :)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs tests

Rebased, worked. Committed.
And added some comment about what the code is doing.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.