Skip to content

Conversation

hasan-ozbey
Copy link
Contributor

@hasan-ozbey hasan-ozbey commented Feb 14, 2020

for #20
This is the updated version for Combined renderer (name could be changed).

Basically it will find </del> closures in old lines and change them with new parts and their delimiters in new lines.

Old:
Hello world.

New:
Hello test.

Outputs:
Hello <del>world</del><ins>test</ins>.

Needs discussion & further testing.

@jfcherng jfcherng added the enhancement New feature or request label Feb 14, 2020
@jfcherng jfcherng linked an issue Feb 14, 2020 that may be closed by this pull request
Signed-off-by: Jack Cherng <[email protected]>
Signed-off-by: Jack Cherng <[email protected]>
@jfcherng
Copy link
Owner

I just do some coding style tweaks in the feat-renderer-combined branch. Please merge it into your v6 branch.

@jfcherng
Copy link
Owner

I just add more cases to example/.

Click to expand screenshots (current Combined renderer)

line-level (the region from the first different char to the last different char):
image

word-level:
image

Click to expand screenshots (GitHub's comment history)

image

image

@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey commented Feb 16, 2020

awesome! currently i'm working on a new JSON renderer for less storage volume in DB. Maybe you can allow users to use their own renderers from their namespaces and add all of the current renderers as an example in v7? I think the repo will be more robust & customizable that way. What do you think? It will require lots of work though but i'm sure that this lib will be the one when it comes to diff implementation in PHP.

@jfcherng
Copy link
Owner

jfcherng commented Feb 16, 2020

Eventually, I think it's much easier to not take care about line numbers just like what GitHub does... I kinda feel that diff on "article" is totally different from diff on "codes" right now. Feel like doing a monkey patch.

The following is how the feat-renderer-combined-no-num branch looks like currently. Each detail level seems to have its own disadvantage though.

line-level:
Snipaste_2020-02-17_01-07-24

word-level:
Snipaste_2020-02-17_01-07-57

The thought about dealing with a "replace" block is to treat them as "a line with \n in it" and then (hard-codedly) use a (line-level) LineRenderer to mark differences. And then the mergeDiffs() you wrote may be able to handle it later.

@jfcherng
Copy link
Owner

jfcherng commented Feb 16, 2020

awesome! currently i'm working on a new JSON renderer for less storage volume in DB. Maybe you can allow users to use their own renderers from their namespace and add all of the current renderers as an example in v7? I think the repo will be more robust & customizable that way. What do you think?

Users should already can do it. I didn't actually try it but basically you just do not use the RendererFactory but just initiate your own renderer by new MyRenderer($rendererOptions).

// custom usage
$differ = new Differ(explode("\n", $old), explode("\n", $new), $differOptions);
$renderer = new MyRenderer($rendererOptions);
$result = $renderer->render($differ);

Your renderer must implements the RendererInterface.

@jfcherng
Copy link
Owner

I notice that GitHub's diff uses <table> layout. This discourages me to move to flex-box layout since the current tabular layout works fine. 🤔

@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey commented Feb 16, 2020

I noticed diff2html library also uses the table layout. Maybe you just can implement some scrollbar like that lib does.

@jfcherng jfcherng force-pushed the v6 branch 4 times, most recently from d079960 to 0394a74 Compare February 17, 2020 19:05
@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey commented Feb 20, 2020

I finished new JSON format but it can't be used in this repo because of ignoreWhitespace and ignoreCase options. Here's the comparison:

https://www.diffchecker.com/hWRMcRDB

It's not duplicating context lines and can be useful for custom projects. I did this for my Flarum forum extension because Flarum itself is not ignoring whitespaces & cases either.

I coudn't find any other way to do this new Combined renderer other than replacing </del> tags though. Because the original JSON format fits well to this repository.

I also implemented diff2html library's layout without any problems, it fits well and looks beautiful by the way.

@jfcherng
Copy link
Owner

jfcherng commented Feb 21, 2020

Thanks. I have written some inspirations down in the project page. So if v7 ever happens, there are some thoughts.


I finished new JSON format but it can't be used in this repo because of ignoreWhitespace and ignoreCase options. Here's the comparison:

We can do some checking in getChanges() to see if old and new are exactly the same (which should be most of the cases since people won't use those options mostly) and do some simplification. Or do it in the JSON renderer. Either way, it's a breaking change so it won't happen in v6.

It's not duplicating context lines and can be useful for custom projects. I did this for my Flarum forum extension because Flarum itself is not ignoring whitespaces & cases either.

Yeah. I think people don't use those options in 99% cases. Those options are more like because they were there or seen in other diff softwares.

I coudn't find any other way to do this new Combined renderer other than replacing tags though. Because the original JSON format fits well to this repository.

I cannot figure out a good way to make it work with line numbers. #22 (comment) is the best result I can get.

I also implemented diff2html library's layout without any problems, it fits well and looks beautiful by the way.

I try to make this lib unrelated (or less related) to frontend thing (I am lame in writing maintainable CSS) but for HTML renderer output, it has spoke itself (it's for frontend). If the user uses Vue.js, React.js or other templating lib, he/she may use the JSON output to write the layout in the frontend, which I think is more flexible.

@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey commented Feb 22, 2020

I will use this in production without line numbers but i think it shouldn't print the new line here, because the old line already indicates the deleted part:

Screenshot

@jfcherng
Copy link
Owner

jfcherng commented Feb 22, 2020

I will use this in production without line numbers but i think it shouldn't print the new line here, because the old line already indicates the deleted part:

Screenshot

I am not able to reproduce on my feat-renderer-combined-no-num branch.

image

But it may not directly usable for you since your Json renderer output is currently different from this repo's.

@jfcherng
Copy link
Owner

jfcherng commented Feb 23, 2020

I have changed the algorithm so replace-type blocks are always combine-able. Having different counts of del/ins actually doesn't matter.

char-level:
image

I have also fixed the behavior when detailLevel is none. So codes in the feat-renderer-combined-no-num branch should be okay now unless there are unspotted bugs or problems.

@jfcherng jfcherng force-pushed the v6 branch 4 times, most recently from 4c74aa9 to 8b89bbc Compare February 23, 2020 20:40
@jfcherng jfcherng force-pushed the v6 branch 7 times, most recently from fe01bb0 to 2277a98 Compare February 28, 2020 14:33
@jfcherng jfcherng merged commit 2cf8e84 into jfcherng:v6 Feb 28, 2020
@jfcherng
Copy link
Owner

jfcherng commented Feb 28, 2020

Thanks. That's see how it will actually works. 🎆

6.5.0 has been released.

@hasan-ozbey
Copy link
Contributor Author

looks beautiful 🤩 i'll implement it in Flarum extension later today. Thanks!

@hasan-ozbey
Copy link
Contributor Author

hasan-ozbey commented Mar 4, 2020

i think i found a bug.

Old:

Testing one
Testing two

New:

Testing one

What's with?

Output in Word-level:

Could you confirm it?

@jfcherng
Copy link
Owner

jfcherng commented Mar 5, 2020

@the-turk Yes, confirmed. \n should be replaced with <br> for HTML output.

8adc4f0 should fix it.

jfcherng added a commit that referenced this pull request Mar 5, 2020
jfcherng added a commit that referenced this pull request Mar 5, 2020
jfcherng added a commit that referenced this pull request Mar 5, 2020
jfcherng added a commit that referenced this pull request Mar 5, 2020
jfcherng added a commit that referenced this pull request Mar 5, 2020
jfcherng added a commit that referenced this pull request Mar 5, 2020
jfcherng added a commit that referenced this pull request Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New Renderer Suggestion
2 participants