NppFTP sends NPPM_RELOADFILE message to N++ when "No" is clicked in file reload dialog

dcorsello commented 3 years ago

Description of the Issue

When editing a file that was downloaded from the server via NppFTP in Notepad++, if the same file is opened again via NppFTP, the reload dialog appears, asking if I want to reload the file and lose changes. When I choose "No", the file reloads anyway, and I lose all changes and edit history. This has resulted in major delays due to loss of critical work. I'm using Notepad++ v8.1.9.2 and NppFTP v0.29.8.

Steps to Reproduce the Issue

Open the NppFTP window and double-click on a file to open it from the server for editing.
Edit the file and do not save.
Double-click on the same file in the NppFTP window.
When the Reload dialog appears, click on "No".

Expected Behavior

The file should not reload from the server, and the current editing history should be preserved.

Actual Behavior

The file reloads from the server, all unsaved changes are lost, and the current editing history is lost.

Debug Information

Notepad++ v8.1.9.2 (32-bit) Build time : Nov 21 2021 - 04:27:12 Path : C:\Program Files (x86)\Notepad++\notepad++.exe Command Line : Admin mode : OFF Local Conf mode : OFF Cloud Config : OFF OS Name : Windows 10 Enterprise (64-bit) OS Version : 2009 OS Build : 19043.1348 Current ANSI codepage : 1252 Plugins : ComparePlugin.dll DSpellCheck.dll mimeTools.dll NppConverter.dll NppExport.dll NppFTP.dll

I originally posted this as a Notepad++ issue. xomx replied, "I debugged it and it seems to me as a NppFTP issue indeed, as the N++ respect correctly the 'No' answer and it handles the opened file buffer accordingly, but then the NppFTP plugin sends NPPM_RELOADFILE message to N++ anyway and so the N++ has to obey..."

dcorsello commented 3 years ago

I should add that I would never intentionally reopen a file from the server. But sometimes I do it accidentally, usually when I have a large number of files open. The N++ issue number is #10816.

xomx commented 3 years ago

Quick fix proposal.

In the NppFTP source .\src\Windows\FTPWindow.cpp -> class method FTPWindow::OnEvent -> switch(queueOp->GetType()) -> case QueueOperation::QueueTypeDownload is the following relevant code:

...

if (code == 0) {
    //Download to cache: Open file
    OutMsg("Download of %s succeeded, opening file.", opdld->GetExternalPath());
    ::SendMessage(m_hNpp, NPPM_DOOPEN, (WPARAM)0, (LPARAM)opdld->GetLocalPath());
    ::SendMessage(m_hNpp, NPPM_RELOADFILE, (WPARAM)0, (LPARAM)opdld->GetLocalPath());
} else {
    //Download to other location: Ask
    int ret = ::MessageBox(m_hNpp, TEXT("The download is complete. Do you wish to open the file?"), TEXT("Download complete"), MB_YESNO);
    if (ret == IDYES) {
    ::SendMessage(m_hNpp, NPPM_DOOPEN, (WPARAM)0, (LPARAM)opdld->GetLocalPath());
    ::SendMessage(m_hNpp, NPPM_RELOADFILE, (WPARAM)0, (LPARAM)opdld->GetLocalPath());
    }

...

1) Remove the redundant(?) NPPM_RELOADFILE messages.

I am not the author of the NppFTP code, so I do not know, why is needed the reloading of the same file after its successful opening in N++?!

2) The issue can be also fixed when we change the (WPARAM)0 for the NPPM_RELOADFILE to (WPARAM)true.

this change later in the N++ code causes calling the Notepad_plus::doReload(BufferID id, bool alert) with the 'alert' param set to true
but it is suboptimal, as with this change user will be annoyed with two similar reload-confirm messageboxes instead of one

dcorsello commented 2 years ago

I tried downgrading NppFTP from its current version, 0.29.8 to versions as far back as 0.29.4 from 2019. The problem persists.

I install N++ updates whenever the program prompts that a new version is available. Do N++ updates include plugin updates? If not, then it would seem to me that N++ is definitely at fault, because the same plugin would have been in place for years on my main computer. If N++ updates do include plugin updates, then from my perspective, the problem could be with either N++ or NppFTP.

If the problem is in NppFTP, I’m not sure that the edit you suggested will fix the problem. The code you identified hasn’t changed in years. I checked as far back as NppFTP v0.27.3 from December, 2017.

xomx commented 2 years ago

@dcorsello

As I said, I am not the author of the NppFTP code, so your questions about the NppFTP code should be answered by someone else. I am just another user of the NppFTP and I tried my best to find a solution for this potential dangerous situation, as the next time it can be me bitten.

As for the N++ plugins automatic update - I do not think so. At least for me it works in a way, that I have to check for an update in N++ every time via Plugins -> Plugins Admin... ->Updates tab.

Did you also tried the opposite testing (I mean let the latest vanilla NppFTP plugin as is, but downgrade N++ versions to the point that this issue will not be present)? That way you can confirm your theory about this issue (that in fact this is a N++ problem and not the NppFTP one and moreover it then could also help to locate the problem in the N++ source code, because of we will know what change in N++ source code causes this issue).

And maybe this problem has been there for years, but you just discovered it nowadays.

I saw that you have a building problem, so here is my x64 Release Unicode patched version (method No.1, it works ok for me, but use it at your own risk):

http://redwoodal.sweb.cz/NotepadPlusPlus/NppFTP_x64_Unicode_0.29.8.1(patched-deleted_NPPM_RELOADFILE).7z

dcorsello commented 2 years ago

You've gone the extra mile. Thanks a lot for your help.

So far, so good with your modified NppFTP.

xomx commented 2 years ago

My fix still needs to be revised or at least better understood. My feeling is that this project is currently in the maintenance mode (original author no longer works on it, so small fixes or updating libraries only). I am not a networking programmer specialist, but from what I see, the code in the NppFTP project is very well written, so I doubt that the NPPM_RELOADFILE message is there for nothing (and twice!). What I think is that this hack(?) was needed in the past, but then the N++ project development continued (and the NppFTP not) and this became obsolete. So if somebody can check it (eg by the opposite testing I described above), I will be glad. Otherwise, it will have to wait until I have time (ie perhaps never).

xomx commented 2 years ago

So I did the homework myself.

The NppFTP has been removed from the N++ standard distribution in the version 6.8.4 (around Oct 2015), but I have not found the reason why. I suppose that there were problems with the NppFTP.

Version 6.8.3 was the last one with it, its change.log says that it had NppFTP 0.26.3 within.

So I tried some earlier N++ binary distribution (6.7.4 x86, where the NppFTP was still in the installer, I have only to update it for my testing purposes to v0.29.6, because of my network server refused to connect otherwise - too much old hashes). But the result is much worse - if you try to reproduce the issue there, after doubleclicking the same file in the NppFTP subwindow, the file is 1st reloaded (without a message or a chance for an user to stop that), only then the reload-confirm messagebox asking Yes/No appeared (now useless). So the problem described in this issue has been there all the time, I think.

Now it is safer to proceed to a PR.

@chcg Do I understand well that you are now the NppFTP project maintainer? If so, what do you think about the simple change proposed? (commenting out the two lines, where the redundant NPPM_RELOADFILE message is)

chcg commented 2 years ago

Yes, currently I try to maintain this project, also I'm not really using the plugin on my own. Currently trying to get OpenSSL 1.1.1/3.0 running without issue #265, but failed so far.

The behaviour is this way since the initial commit at github in 2010 with Revision: 87a3b12fb49c5466f80fb45f17f7f048d9dfc26b. To me that seems to be somehow duplicated code indeed. Maybe at some point in time it made sense, but currently I see no actual reason to have both calls.

xomx commented 2 years ago

@chcg Ok, I will submit the PR. But to what branch - master or stable_v0_29?

chcg commented 2 years ago

@xomx Is your PR just the removal of the both NPPM_RELOADFILE calls? In this case I could remove them also by my own. Otherwise you could create a PR for master and I will take care for the takeover on the stable branch.

xomx commented 2 years ago

Is your PR just the removal of the both NPPM_RELOADFILE calls?

Yes. So far so good, no problem with the patched NppFTP.

In this case I could remove them also by my own.

Yes please.

chcg commented 2 years ago

Fixed with https://github.com/ashkulz/NppFTP/releases/tag/v0.29.9 and https://github.com/ashkulz/NppFTP/releases/tag/v0.30.12

ashkulz / NppFTP