Skip to content

Side by side diff shows only partially all deleted lines #32

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

Closed
vendeeglobe opened this issue Jun 6, 2020 · 10 comments · Fixed by #33
Closed

Side by side diff shows only partially all deleted lines #32

vendeeglobe opened this issue Jun 6, 2020 · 10 comments · Fixed by #33
Assignees
Labels

Comments

@vendeeglobe
Copy link

This happens only occasionally within the side by side diff mode.

Side_by_side-diff
Inline-diff

php-diff: v1.16
WackoWiki 6.0.7

@JBlond
Copy link
Owner

JBlond commented Jun 6, 2020

That is a css problem. @vendeeglobe Please inspect which WackoWiki css rule overrides the css rule from php-diff.

@JBlond JBlond added the bug label Jun 6, 2020
@vendeeglobe
Copy link
Author

vendeeglobe commented Jun 6, 2020

But then the missing lines should show up in the HTML and they are not.

I can provide other examples.

@DigiLive
Copy link
Collaborator

DigiLive commented Jun 6, 2020

Can you provide sample texts, so we can use them at our systems?

@vendeeglobe
Copy link
Author

vendeeglobe commented Jun 6, 2020

The text files with the source text for each revision.

Example 1:
example-1_r136.txt
example-1_r138.txt

Example 2:
example-2_r45.txt
example-2_r50.txt

Examples can be use freely.
Here you can see that the same source is passed for both modes:
https://github.com/WackoWiki/wackowiki/blob/master/wacko/handler/page/diff.php#L336

@DigiLive DigiLive self-assigned this Jun 6, 2020
@DigiLive
Copy link
Collaborator

DigiLive commented Jun 6, 2020

Since commit c017af5, the string for the table rows of deleted lines are defined with the heredoc syntax at method SideBySide::generateTableRowsDelete().

The concatenation is accidentally dropped with this change, which results in only returning the last row instead of all deleted rows.

I'll fix this bug, update the tests, examples and readme file and make PR.

@DigiLive DigiLive closed this as completed Jun 6, 2020
DigiLive added a commit that referenced this issue Jun 6, 2020
SideBySide::generateTableRowsDelete() doesn't concatenate the tablerows
of deleted lines.
@DigiLive DigiLive linked a pull request Jun 6, 2020 that will close this issue
@DigiLive
Copy link
Collaborator

DigiLive commented Jun 6, 2020

I've made a PR which is waiting on JBlond to be reviewed and merged.

@DigiLive DigiLive reopened this Jun 6, 2020
@JBlond JBlond closed this as completed in #33 Jun 8, 2020
JBlond added a commit that referenced this issue Jun 8, 2020
@JBlond
Copy link
Owner

JBlond commented Jun 8, 2020

@vendeeglobe There is now a release for it.

@JBlond
Copy link
Owner

JBlond commented Jun 8, 2020

@DigiLive I checked out 3e243f1 the last commit before the merge fix. I wonder why the phpunit test didn't fail.

@vendeeglobe
Copy link
Author

Thanks for patching.

@DigiLive
Copy link
Collaborator

DigiLive commented Jun 9, 2020

@DigiLive I checked out 3e243f1 the last commit before the merge fix. I wonder why the phpunit test didn't fail.

Theoretically...

  • In that commit, the sample texts and the resulting texts are generated by us. The test is comparing them to match to each other. So we must make sure all have the correct content.
  • In the sample texts of that commit there's only a block with 1 deleted line.
    Having only 1 deleted line, there's no need to concatenate.
$html1 = 'OneDeletedLine'
$html2 .= 'OneDeletedLine'
// $html2 == $html1

Therefor, I've updated the sample text to a block of 2 deleted lines.

$html1 = 'fisrtDeletedLine';
$html1 = 'SecondDeletedLine';
$html2 .= 'fisrtDeletedLine';
$html2 .= 'secondDeletedLine';
// $html2 != $html1
// 'fisrtDeletedLineSecondDeletedLine' != 'SecondDeletedLine'

You might want to add more lines for the other type of changes as well (equal, inserted, replaced lines). Another (and maybe easier) method is to mock some of the classes.

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

Successfully merging a pull request may close this issue.

3 participants