Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Diff mode Merged shows result only partially #90

Closed
vendeeglobe opened this issue Feb 11, 2021 · 12 comments · Fixed by #93
Closed

Diff mode Merged shows result only partially #90

vendeeglobe opened this issue Feb 11, 2021 · 12 comments · Fixed by #93
Assignees

Comments

@vendeeglobe
Copy link

After implementing the Merged mode a few days ago using php-diff 2.3.1, it seems that the HTML Merged mode shows the diff only partially in comparison to Unified or SideBySide.

Example:
Unified
diff_unified

Merged
diff_merged

Example text:
version_a.txt
version_b.txt

@DigiLive
Copy link
Collaborator

Can you mention the lines you are referring to?
HTML merged only marks inline differences and added blocks text.
Blocks of texts which are removed at version 2, are marked in line column of the table:
afbeelding

@vendeeglobe
Copy link
Author

Ignores changes in line
19 to 21
70 to 86
99 - not shown at all, not like in your example above

diff_sidebyside

Example: (enabled diff for guests temporarily)
https://wackowiki.org/doc/Doc/English/Themes/diff?a=4227&b=0&diffmode=5

@DigiLive
Copy link
Collaborator

Which options did you provide to the Diff class?

@vendeeglobe
Copy link
Author

No options, only default. Should I set particular options for Merged?

https://github.com/WackoWiki/wackowiki/blob/master/wacko/handler/page/diff.php#L352

@vendeeglobe
Copy link
Author

vendeeglobe commented Feb 12, 2021

I forgot to mention that I use PHP 8.0.

Found this in the error log with varying array keys:
PHP Warning: Undefined array key 5 in /lib/php-diff/Diff/Renderer/Html/Merged.php on line 217

preg_match_all('/\x0.*?\x1/', $changes['changed']['lines'][$lineNo], $addedParts, PREG_PATTERN_ORDER);

@DigiLive
Copy link
Collaborator

I didn't test with php 8, bit I'm still suprised by it.
I'll take a look into it soon.

Thank you for testing and feedback.

@DigiLive DigiLive self-assigned this Feb 12, 2021
@DigiLive
Copy link
Collaborator

I forgot to mention that I use PHP 8.0.

Found this in the error log with varying array keys:
PHP Warning: Undefined array key 5 in /lib/php-diff/Diff/Renderer/Html/Merged.php on line 217

preg_match_all('/\x0.*?\x1/', $changes['changed']['lines'][$lineNo], $addedParts, PREG_PATTERN_ORDER);

Something is incorrect with your error description or file. See below lines.

if (!$lineNo && $this->lastDeleted !== null) {
$headerClass = 'ChangeDelete';
}

Also the script doesn't throw a warning or error at my local server, using php v8.0.2.

@vendeeglobe
Copy link
Author

vendeeglobe commented Feb 13, 2021

I patched away the table header therfore the different line number.
https://github.com/WackoWiki/wackowiki/blob/master/wacko/lib/php-diff/Diff/Renderer/Html/Merged.php#L217

The error messages are from the server log and do probably not belong to the posted example. I turned error reporting on again after posting the issue.

@DigiLive
Copy link
Collaborator

DigiLive commented Feb 13, 2021

I've created a new issue for it since it seems unrelated to the original report.
Please continue the discussion about the thrown warning over there.

Also you don't have to patch the renderer. You can create your own or extend an included one.
See https://github.com/JBlond/php-diff/wiki/3.-Custom-Renderer

@DigiLive
Copy link
Collaborator

Ignores changes in line
19 to 21
70 to 86
99 - not shown at all, not like in your example above


19 to 21: Bug confirmed. Will create a PR for it soon.
70-86: Marked as inserted (see below).
99: Table header marked as deleted at version 1 (see below).

afbeelding

DigiLive added a commit that referenced this issue Mar 27, 2021
* Renderer doesn't show lines which where added to version 2 of a
  replaced block. Code assumed the same amount of lines in replacement
  blocks at both versions. Now it handles replacement block differently
  when the amount of lines are equal or differs.
* Code cleanup.
* ClassName for lines of replacement blocks are incorrect. Insertion
  is insinuated, but should be replacement.
JBlond added a commit that referenced this issue Mar 27, 2021
Fix #90 - Diff mode Merged shows result only partially
@DigiLive
Copy link
Collaborator

Don't forget to clear you browsers cache to pull in the newest css files.

@vendeeglobe
Copy link
Author

Already merged and Browser cache has been purged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants