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

Do not move diff marks to next line if the user deletes a line #30

Closed
Yaron10 opened this issue Mar 24, 2016 · 30 comments
Closed

Do not move diff marks to next line if the user deletes a line #30

Yaron10 opened this issue Mar 24, 2016 · 30 comments
Labels

Comments

@Yaron10
Copy link

Yaron10 commented Mar 24, 2016

STR:

Open the files in Test.zip.
Activate compatibility.ini and Compare.
Delete line 3.

Result:
Line 3 has both "Deleted" and "Different" images.

** Even if line 4 was not "Different", the diff marks from line 3 shouldn't be moved but removed.
** I don't expect Compare Plugin to dynamically re-compare, but it would be good to fix this issue if possible.

@pnedev
Copy link
Owner

pnedev commented Mar 25, 2016

I'll test that ASAP. Thanks for reporting.

BR

@Yaron10
Copy link
Author

Yaron10 commented Mar 25, 2016

Thank you Pavel. I appreciate that.

BR

@pnedev
Copy link
Owner

pnedev commented Apr 6, 2016

Confirmed. I'll look into this shortly.

BR

@pnedev pnedev added the bug label Apr 6, 2016
@pnedev
Copy link
Owner

pnedev commented Apr 7, 2016

Fixed in master branch.

BR

@pnedev pnedev closed this as completed Apr 7, 2016
@Yaron10
Copy link
Author

Yaron10 commented Apr 8, 2016

Hello Pavel,

That's a progress. I really appreciate it.
It still needs some tuning up. :)

Could you please try the modified files?

Delete the first not-equal line.
Result: the marker from the next line is removed too.

Similar issues in other cases.

Thank you very much.

BR

Test.zip

@pnedev
Copy link
Owner

pnedev commented Apr 12, 2016

Hello Yaron,

This is fixed, thanks.

BR

@Yaron10
Copy link
Author

Yaron10 commented Apr 13, 2016

Hello Pavel,

Thank you so much. I really appreciate it.

** I don't expect Compare Plugin to dynamically re-compare, but it would be good to fix this issue if possible.

When I opened this issue I wasn't sure whether Compare or NPP moved the mark to the next line.
Testing Bookmarks, I understand it's NPP.
And if so, this request may well be in the realm of dynamically re-comparing (where do we draw the line?).

Anyway, the current implementation has some major improvements but also some regressions.
Could you please compare the prefs files (prefBackup as current; lines 61-62)?

I haven't checked the relevant code. How about undoing the line-delete, remove its mark and redo the deletion?
That is if if you think it should be implemented at all.

Also, our discussion in #29 might affect this as well.
I would suggest to revert the changes and return to this issue later.
What do you think?

Best regards.

@pnedev
Copy link
Owner

pnedev commented Apr 14, 2016

Hello Yaron,

I'm a bit confused now.
Let's leave aside the dynamic re-compare idea (which is great by the way but way too complex). Just let's leave it for "dessert" ;)

What possible connection might exist between the user deleting some lines from the compared file and the compare process itself? This has nothing to do with re-compare. The line markings were simply left behind while the line is gone (deleted by the user) and the next line markings were moved up together with its corresponding line which resulted in several different markings for the moved lines.
So I just make sure the deleted line/s markings go away with the line/s itself/themselves.

How about undoing the line-delete, remove its mark and redo the deletion?

The current implementation is functionally the same but more optimal. It first removes the markings and then moves to deleting the files. No need for undo/redo.

Anyway, the current implementation has some major improvements but also some regressions.
Could you please compare the prefs files (prefBackup as current; lines 61-62)?

I tried that and I didn't see any problem. Files compare just fine both ways.
Am I missing something?
I admit I haven't tried comparing them with old version of the plugin - please don't make me do so, it's rather painful :)

Please clarify further the problem as I don't see one, thanks!

BR

@Yaron10
Copy link
Author

Yaron10 commented Apr 14, 2016

Hello Pavel,

Thank you for a delicious meal.
Judging by the appetizers and the main course, the dessert would be great too.
You'll end up with three Michelin stars. :)

What possible connection might exist between the user deleting some lines...

I had in mind the "Two Equal Files" concept. Implementing it, there shouldn't be any empty lines with no marks.
So it was just a vague idea that this might affect the issue in question.

The current implementation is functionally the same but more optimal. It first removes the markings and then moves to deleting the files. No need for undo/redo.

Great!

I tried that and I didn't see any problem.

Please see the screenshots.
http://s24.postimg.org/4sxyqejn9/Old.png
http://s24.postimg.org/wgf9i8u7p/New.png

Just compare. No deletions.

Could some other recent modifications be the cause?

Best regards.

@pnedev
Copy link
Owner

pnedev commented Apr 15, 2016

Hello Yaron,

Could some other recent modifications be the cause?

Sure, it should have something to do with my modifications on the blank lines handling but the weird thing is that I cannot reproduce this issue on my side.
Are you using the latest master branch commit - e0b9c81 ?
Please try again, thanks.

BR

@Yaron10
Copy link
Author

Yaron10 commented Apr 15, 2016

Hello Pavel,

Thank you very much.

Yes, I'm using the latest master branch commit.
@xylographe, can you please try it?

BR

@xylographe
Copy link
Contributor

@Yaron10
Could you please elaborate?
Which file in the left (main) view?
Delete line 3 in the file on the left or the file on the right?

@Yaron10
Copy link
Author

Yaron10 commented Apr 15, 2016

Hello xylographe,

With the latest commit I get wrong results regardless of any deletions.
http://s24.postimg.org/4sxyqejn9/Old.png
http://s24.postimg.org/wgf9i8u7p/New.png

prefsBackup in the top/left view. Just compare.

Thank you.
BR

@xylographe
Copy link
Contributor

@pnedev
Something does in fact go wrong. Using the two files from Yaron, prefs Backup.js and prefs.js, comparing in the same way as in the images (prefs.js in the bottom (sub) view) : when I position the caret on blank line 62 (in the top view), then press Del followed by Ctrl-Z (undo) and Alt-D (re-compare) an empty line (not a blank/gap line) has been inserted, marked with a red minus.

After "Clear Compare" the empty line (line 61 in the top view) is still there, although NPP has not marked the file as modified in the tab bar. A subsequent "Reload" of prefs Backup.js makes empty line 61 disappear.

Can you reproduce that, Pavel?

@Yaron10
Copy link
Author

Yaron10 commented Apr 16, 2016

@xylographe,

But what do you see when you just compare the files and scroll to line 61?

@pnedev
Copy link
Owner

pnedev commented Apr 16, 2016

Hello guys,

I'll leave that for another time, I need some rest.
Can you meanwhile re-test with latest master? Thanks.

BR

@pnedev pnedev reopened this Apr 16, 2016
@pnedev
Copy link
Owner

pnedev commented Apr 16, 2016

@xylographe ,
After a quick test I was able to reproduce the blank line appearing. I'll investigate later.

BR

@Yaron10
Copy link
Author

Yaron10 commented Apr 16, 2016

Hello Pavel,

The Compare Plugin seems to require a team of developers.
I've been wondering for quite a while how you can juggle with all these issues at the same time.
Allow me to express again my sincere gratitude and appreciation.
Please slow down. And please have a good rest this weekend. :)

I'd rather continue discussing this issue next week.

Have a pleasant and refreshing weekend.

Yaron

@xylographe
Copy link
Contributor

@Yaron10

But what do you see when you just compare the files and scroll to line 61?

Nothing unusual as you can see in NPP-CP-prefs-001
Using CP f228177

@pnedev

Can you meanwhile re-test with latest master?

Same steps as above lead to same result in CP f228177

@Yaron10
Copy link
Author

Yaron10 commented Apr 16, 2016

@xylographe,

I've downloaded a fresh portable (7z) NPP 6.9.1 and got the same wrong results.
(I just put ComparePlugin.dll in the plugins folder; no other changes).
Could you please try that?
Could you also upload your built DLL?

I hope Pavel is lying somewhere in the sun and won't be reading any of these messages until next week. :)

Thank you.
Have a nice weekend.

@xylographe
Copy link
Contributor

ComparePlugin-f228177 (built with VS 2015)
I'm using normal, downloaded 32-bit NPP 6.9.1 on Win7SP1[64]

@Yaron10
Copy link
Author

Yaron10 commented Apr 16, 2016

Thank you. I appreciate that.

It works properly with your build.
I'm using VS 2013 Express. Deleted everything and rebuilt it. Never had a problem before.
Any idea? If not, I'll wait until Pavel returns from his vacation. :)

BR

@xylographe
Copy link
Contributor

Oops, I typed "(built with VS 2015)" but I meant "(built with VS 2013)"

My idea: Upgrade to VS Community 2013 perhaps?

@Yaron10
Copy link
Author

Yaron10 commented Apr 16, 2016

Thanks again.
I'll wait for Pavel. He might have another idea.

BR

@pnedev
Copy link
Owner

pnedev commented Apr 17, 2016

Hello guys,

@xylographe ,
The issue with the appearing line is a tough one.
This is because now I'm manually deleting the mark of the line and when you undo, the line is automatically back but I do not restore its mark. At that moment N++ knows the file is not changed but when you clear the compare results the extra blank is not deleted because the marking is missing and you finally get a file that appears saved but with an extra line. @Yaron10 was right to some extent that this issue is on the border line with a dynamic re-compare.
I'll think about fixing that, I have an idea already.

@Yaron10 ,
It's very strange that VS 2013 Express is giving different results. I have no idea now what might be wrong but it is a compiler issue for sure. I'll try to analyze it further.

BR

@Yaron10
Copy link
Author

Yaron10 commented Apr 18, 2016

Hello Pavel,

Thank you. I appreciate it.

I think the compiler issue is a question for Claudia.

BR

@pnedev
Copy link
Owner

pnedev commented Apr 18, 2016

Hello Yaron,

OK, that's good. Claudia may have some idea about that or think of something we miss.

BR

@pnedev
Copy link
Owner

pnedev commented Apr 20, 2016

Hello @xylographe ,

The particular issue you reported is fixed with the latest commit, thanks.

BR

@pnedev pnedev closed this as completed Apr 20, 2016
@Yaron10
Copy link
Author

Yaron10 commented Apr 20, 2016

Hello Pavel,

That's a major improvement.
Thank you.

BR

@xylographe
Copy link
Contributor

Thank you, Pavel.
Fix confirmed. (CP 81dbed4)

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

No branches or pull requests

3 participants