pnedev / comparePlus

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

Comparing an untitled empty document to clipboard #324

Closed Yaron10 closed 2 years ago

Yaron10 commented 2 years ago

Hello Pavel,

If you compare an untitled empty document to clipboard, you only get the clipboard temp document. It also starts with a random number: Clipboard_4 or Clipboard_5.

Thanks again for your work. I appreciate it.

BR

pnedev commented 2 years ago

Hello Yaron,

I suspect you start with Clipboard_4 because you already have Clipboard_1 to Clipboard_3 files in your user's Temp folder. Those may be left from previous CP tests you have conducted which shouldn't happen but might have happened when you had been testing the compare to clipboard functionality (or maybe if some crashes had occurred). You might as well have active compares to clipboard that have Clipboard_N file depending on which compare in a row it is.

What do you mean by "you only get the clipboard temp document" ? I couldn't understand, sorry. Thanks.

BR

Yaron10 commented 2 years ago

Hello Pavel,

What do you mean by "you only get the clipboard temp document" ?

STR: Open NPP with no session (i.e. you have "New 1"). Compare to Clipboard.

Result: You only get the clipboard temp document in a single-view mode.

Expected: "New 1" in one view and Clipboard_1 in the other view.


I suspect you start with Clipboard_4 because you already have Clipboard_1 to Clipboard_3 files in your user's Temp folder. ... (or maybe if some crashes had occurred).

👍 That is correct.

The crash does NOT occur with the current NPP. For my personal use I have a slightly modified NPP build (I haven't included some of the recent PluginManger commits). With that build NPP crashes on comparing an untitled empty document to the clipboard. (On Windows 7 I get a message: "NPP crashed due to ComparePlus.dll").

Obviously this is not your problem, and I'd have to figure it out myself. But I'm wondering if the two issues here might be related (that is: the single-view and the crash).

I'd like to emphasize that I test the original NPP build before opening an issue. :)

Thanks again.

pnedev commented 2 years ago

Hello Yaron,

I get it now, thanks for the clarification :+1:

Well, I'll leave it as a known 'limitation' because it is not a real use case on one hand and on second - it behaves that way because the empty doc ('New 1') that is shown on Notepad++ start without session is a special case "file". What I mean is that if you try to open an existing file when you start Notepad++ that way the initial 'New 1' will be gone (which is good of course and is how Notepad should behave). But that "disappearance" of the 'New 1' leads to only showing the clipboard contents file on CP clipboard compare. That is because CP creates clipboard file and opens it via Notepad++'s message NPPM_DOOPEN which leads to the 'New 1' disappearance. I hope that brings some light into what's going on under the hood but I have no idea why in your build that leads to a crash.

P.S. I admit that this STR looks strange (even if it's not a valid use case and it is very unlikely that somebody will do that as it is pointless) so I'll think of a way to give a reasonable feedback in such case. Thanks again.

BR

pnedev commented 2 years ago

Hello again Yaron,

The strange behavior should be fixed in this build. Thanks.

BR

Yaron10 commented 2 years ago

Hello Pavel,

Thank you for the detailed explanation. 👍 And thank you also for the latest commit.

The current implementation might be better, but it's slightly inconsistent/confusing. If you only have an empty "New 1" - operation ignored. (BTW, it would be nice if the old "- operation ignored." should appear in all the relevant messages. :) ). But if you have empty "New 1" and "New 2" - "Compare to Clipboard" is executed.

From a user point of view: what's the difference?

If you prefer to allow comparing empty files, how about opening a "New 2" before "Compare to Clipboard" and then close it? Too much?


Apropos empty "New 1" and "New 2": Compare. Close compared files? - No.

Result: Only one file remains open.

We might have discussed it already. :)

pnedev commented 2 years ago

Hello Yaron,

I will be perfectly happy with ignoring compares of empty unsaved files, what do you think? (This means that compares of unsaved but not empty files will be OK as well as comparing empty but saved files - saved means 'existing on disk', not necessarily saved after the last changes).

About the case with 'New 1' vs. 'New 2' compare - there is nothing I can do about it (or it is really not worth the extra effort).

BR

Yaron10 commented 2 years ago

Hello Pavel,

I will be perfectly happy with ignoring compares of empty unsaved files

I assume you mean any type of Compare, correct? I think that would be great.

This means that compares of unsaved but not empty files will be OK

Sure.

as well as comparing empty but saved files

I don't quite understand it. Why should an empty "saved" file be different and get compared? If you argue that the user might not know the file is empty - well, they would get the "Empty file(s)" message. Also, you'd only need to check "docLength" instead of "docLength" + "saved".

About the case with 'New 1' vs. 'New 2' compare - there is nothing I can do about it (or it is really not worth the extra effort).

This should also be solved if you don't allow comparing empty file(s).

Thanks again for your great work. I appreciate your patience.

pnedev commented 2 years ago

Hello Yaron,

I assume you mean any type of Compare, correct? I think that would be great.

Exactly. Great, I will implement it (it is already implemented for compare to clipboard, need to fix only compare between files).

Why should an empty "saved" file be different and get compared?

My logic is this: If a doc in Notepad++ is empty and it is not an existing file then there is no reason to compare it because it is fresh Notepad++ document - newly created and unmodified - insignificant. If a doc in Notepad++ is opened existing empty file (empty doc again) then it is nevertheless saved to disk so is 'significant' in that matter. It might be log that is emptied or something. In that line of thought consider what will happen if you delete the whole content of the opened doc (it is existing file) but don't save it and trigger 'Last Saved Diff'. Then you won't be able to see what modifications were made. Same is true for all self-comparing commands - SVN and Git.

It is easier for me to check only if doc is empty but don't you think that in the above cases it is better to trigger comparison even though the doc is empty?

This should also be solved if you don't allow comparing empty file(s).

In sake of consistency you are right.

BR

Yaron10 commented 2 years ago

Hello Pavel,

and trigger 'Last Saved Diff'.

Good point. 👍 Those commands should be excluded.

It might be log that is emptied or something.

Do you mean it's currently logging and the user wants to compare when the file is empty?

My approach was that trying to compare an empty file is probably a "PEBKAC", and displaying the "Empty file(s)" message would be much better.

That's not the case if a non-existing document is emptied in compare-mode. We surely shouldn't prompt "Empty file(s)" but let Compare go on.

So at this stage:

  1. You allow comparing existing empty files.
  2. You allow re-comparing non-existing empty files.

And I'm wondering now if it's worth the extra code/work and also if it would not be rather confusing. Perhaps leaving it as it is (with https://github.com/pnedev/comparePlus/commit/57eb486d1a9d83d197c258bfe94bee9ec67fab4b) should be better?

What do you think?

Thank you.

pnedev commented 2 years ago

Hello Yaron,

Perhaps leaving it as it is (with https://github.com/pnedev/comparePlus/commit/57eb486d1a9d83d197c258bfe94bee9ec67fab4b) should be better?

I think it is better that way. No point making it more sophisticated than needed IMO and these are minor 'issues'. Thank you 👍

BR

Yaron10 commented 2 years ago

Hello Pavel,

It's brilliant as it is. 👍 Thanks again for your hard work.

BR

Yaron10 commented 2 years ago

Hello Pavel,

Just another interesting case:

Apropos empty "New 1" and "New 2": Compare. Close compared files? - No.

Result: Only one file remains open.

If you open a new doc now, you get two "New X". https://github.com/notepad-plus-plus/notepad-plus-plus/issues/11704.

Thank you.