pnedev / comparePlus

Compare plugin for Notepad++
GNU General Public License v3.0
961 stars 139 forks source link

Unexpected behavior when Ignore whitespace selected #345

Closed jeesusonherraturku closed 1 year ago

jeesusonherraturku commented 1 year ago

Thank you for this very useful plugin.

I tested whether I can get same results using Winmerge and comparePlus plugin.

When Ignore whitespace is ON: Below is an image of Winmerge (one line). The functionality is what I expect. The red part shows that one word is different. comparePlus plugin however ignores the whole line and does not show any difference. (The language is in Finnish).

winmerge_ignore_whitespaces

In Winmerge there are two options for Ignoring whitespace, "Ignore All" and "Ignore changes". I have "Ignore changes" selected, maybe this is why it works differently.

I would suppose that many people would want to see the important difference as illustrated in the picture. In my case I cannot set Ignore whitespace completely OFF , since it will fill my screen with too many lines that I dont want to see ( I have also "show only differences" selected).

I may have other questions but I will open a separate issue if necessary.

Thank you.

jeesusonherraturku commented 1 year ago

I tested that if I select "Ignore All" in Winmerge, then the behavior seems to be similar than in comparePlus plugin.

Perhaps "Ignore changes" functionality comes to comparePlus plugin also, so that we can see important differences as in my example. If I understood correctly it very similar to "Ignore all". It is described here

https://manual.winmerge.org/en/Compare_files.html#CompareFiles_linediff-highlight

pnedev commented 1 year ago

Hello @jeesusonherraturku ,

You are right, when 'Ignore Spaces' is enabled in ComparePlus it behaves as if they are not there so j otka matches jotka in your example. This might seem awkward to the user, I agree. I'll examine this a bit and see if I should change the behavior or add additional 'Ignore changed spaces' option (the current is 'Ignore all spaces').

Snabel42 commented 1 year ago

Hello @jeesusonherraturku ,

You are right, when 'Ignore Spaces' is enabled in ComparePlus it behaves as if they are not there so j otka matches jotka in your example. This might seem awkward to the user, I agree. I'll examine this a bit and see if I should change the behavior or add additional 'Ignore changed spaces' option (the current is 'Ignore all spaces').

Just adding another voice to say that this feature would be much appreciated.

pnedev commented 1 year ago

Hello @jeesusonherraturku , All,

Please try this CP development build (win32 or win64) and tell me what you think. It has implementation of "Ignore All Spaces" (the current behavior that removes all spaces) and the new option "Ignore Changed Spaces" that only ignores the spaces in certain place that have different type/size.

You could try comparing these lines (use Copy/Paste): file 1: this is a test line file 2: this is atest line Try the two options but have in mind that "Ignore All Spaces" has higher priority and if both are checked (enabled) the behavior will be like "Ignore All Spaces".

I would appreciate your opinion on this particular case: Shall "Ignore Changed Spaces" totally ignore the spaces in the beginning and the end of the lines so only for the beginning and the end of the lines to behave like "Ignore All Spaces"? For all other places the space is kept as a words delimiter and is not ignored totally - it should be present in order to keep the words separate but its format and size doesn't matter.

BR

Yaron10 commented 1 year ago

Hello Pavel,

Thank you very much for another excellent improvement. Much appreciated. 👍

Try the two options but have in mind that "Ignore All Spaces" has higher priority and if both are checked (enabled) the behavior will be like "Ignore All Spaces".

Wouldn't it be better if only one option could be checked at any given time? - I think this is the expected behavior, and it would also be much clearer. (When the user checks one option, programmatically uncheck/disable the other one).

Shall "Ignore Changed Spaces" totally ignore the spaces in the beginning and the end of the lines...

If I understand correctly the request here and its usefulness in real-life, a space between non-space characters might fundamentally change the meaning of the text and create two completely different words. This, apparently, is irrelevant to leading/trailing spaces.

Let's see what the others think.

BR


Hello @jeesusonherraturku,

Thank you for opening this issue.

jeesusonherraturku commented 1 year ago

Hello,

Thank you for the update!

I downloaded the update and was able to do testing. It seems to work quite well most of the time. Occasionally Notepad++ hangs if I put spaces or tabs at the of line (closes automatically after about 5 seconds). It seems to occur if only other line has spaces/tabs at the end of line, but the compared line does not. I was able to take screenshot of one comparison that crashed. I dont know if its only my system.

New Project

For the other thing:

Personally, I would prefer ignoring all spaces/tabs at the beginning and end of lines when "Ignore changed spaces" .

For better illustration I took picture of Winmerge to show how it works currently when "Ignore changes". It ignores all spaces/tabs at the end in all cases. But for the beginning it works differently. If one of the compared lines does not have any spaces/tabs at the beginning it will not ignore them, but if both lines have at least one space then it will ignore. My use case(scanned documents) often leaves excessive spaces/tabs at the end, so Im happy that they are ignored, while showing if a whole word has changed meaning. I dont know why for the beginning of lines it works differently in Winmerge. I would prefer at least for the end of lines ignoring all spaces/tabs (otherwise it is not so useful)

(In the picture the spaces and tabs are marked show you can see them).

New Project (1)

Hopefully I didnt make errors in my explanation. Thank you for your hard work.

Yaron10 commented 1 year ago

I can reproduce the crash with previous ComparePlus builds as well.

STR: Uncheck Ignore Spaces. Compare these files.

Result: NPP crashes.

pnedev commented 1 year ago

Hello guys,

Thank you both for the feedback.

I see that both of you prefer to totally ignore spaces in the beginning and at the end of the line when "Ignore Changed Spaces" is enabled. I also think that would be better.

Unfortunately I couldn't reproduce the crash you have observed under Wine on Linux. I'll try on native Win11 some time later, thanks for letting me know.

Yaron,

Regarding

Wouldn't it be better if only one option could be checked at any given time? - I think this is the expected behavior, and it would also be much clearer. (When the user checks one option, programmatically uncheck/disable the other one).

I see your point but such implementation is not very optimal and straightforward. It doesn't seem very handy for the user if the two options are mutually exclusive and disabling each other because for example one might want to use mainly "Ignore changed" and from time to time to enable "Ignore all" on top of it. Now if we have "Ignore changed" enabled then "All" will be disabled and the user would have to go to NPP plugin twice to first disable "Changed" and then to enable "All". The same scenario is valid also if enabling temporarily "All" leads to automatic disabling of "Changed". So maybe "All" (as it is the more strict limitation and thus has the higher effect) could make "Changed" inactive but not the other way around. My personal preference would be to leave them as they are because that way they are more easily changeable and in my opinion are not ambiguous. If you can think of a good way to make your suggestion more clear and easy for the user I would certainly consider it.

Thanks again for the discussion, much appreciated.

Yaron10 commented 1 year ago

Hello Pavel,

I see that both of you prefer to totally ignore spaces... ... I'll try on native Win11 some time later...

👍


Concept: "Ignore Changed Spaces" is actually "Ignore Changed Spaces Only". Aren't the two commands "All" and "Only" contradicting?

A side note: See how NPP implements "View -> Show Symbol -> Show White Space and Tab" and "View -> Show Symbol -> Show End of Line". Checking one command, automatically unchecks the other. IMO, that behavior is wrong in that case: the two commands are not contradicting whatsoever. (The third command "Show All Characters" probably made the coder implement it that way).

Practically: I see your point. But... Yesterday, testing this new feature and trying either All or Only - I found the necessity to check one and uncheck the other (each time I wanted to switch between the modes) slightly inconvenient and unexpected.

My personal preference would be to leave them as they are because that way they are more easily changeable and in my opinion are not ambiguous.

You are the maestro! :)

Thanks again for your time and work. I do appreciate it.

BR

pnedev commented 1 year ago

Hello again guys,

I managed to reproduce the NPP crash on Win11. It doesn't appear to be related to the spaces. I'll find the reason and fix it.

Yaron,

Checking one command, automatically unchecks the other.

I like that solution and implement it that way. Thank you 👍

BR

Yaron10 commented 1 year ago

Hello Pavel,

👍 Great. I appreciate it.

BR

pnedev commented 1 year ago

Hello guys,

Mutually exclusive 'Ignore Spaces....' options behavior is implemented and also the line start/end spaces are now always ignored when one of them is active. Please try this development build and tell me what you think: win32, win64.

I'll let you know when I fix the NPP crash issue. Meanwhile just don't compare files that have a single line. Thanks!

BR

Yaron10 commented 1 year ago

Hello Pavel,

👍 Brilliant! Thank you for the major refactoring. Much appreciated.

BR

jeesusonherraturku commented 1 year ago

Hi,

Thank you.

Based on my testing it seems to work properly now.

Only thing I observed was that when you change between 'compare all' and 'compare changes' you need to press Compare button, it does not work automatically. Maybe its intentional. (this may not be important).

pnedev commented 1 year ago

Maybe its intentional.

Yes, that is intentional. ComparePlus reads its compare options just before you start the compare and keeps them unchanged during Auto-updating process when you change the contents of the compared files. It is done so you don't compromise your current compare if you want to change your options and compare other files. Changing the compare options will require manually re-comparing the files.

Thanks. BR

pnedev commented 1 year ago

Hello guys,

The NPP crash when single-line files are compared is fixed, this is the development build: win32, win64.

Thank you both for testing and reporting 👍 BR

Yaron10 commented 1 year ago

Hello Pavel,

:+1: Great! Thanks again for your time and work.

BR