#436 Fix for bugs #2441, #2585, #2889 and #3396

[Ede_123]

I tracked down the cause of the mentioned bugs ([#2441], [#2585], [#2889] and [#3396]) to the call of "ShowWindow()" in "winmain.cpp". This tiny patch however seems to completely fix the mentioned bugs for me (using Windows 7).

However you're marking "ShowWindow()" with "IMPORTANT !!!" in source code. Why is it important when the windows is already shown (that is neither minimized nor maximized)? Is it really necessary to call "ShowWindow()" in this case?

I wasn't able to see any unwanted effects on my side with this patch applied.

[Don HO]

I tested the patch but it doesn't work on my Windows 8.
Am I missing somethings?

Don

[Ede_123]

Can you tell me what didn't work on Win 8? What were you trying?

What is "ShowWindow()" supposed to do in this part of the code when the window is already opened?

[Ede_123]

Just retested on Windows 7 x64 Prof. thoroughly and still couldn't find any regressions.

The bug is fixed in all cases I tried (that is all cases I could imagine):
- Opening a file via double click
- Opening a file via context menu
- Opening a file via "Send to"

while Notepad++ being in:
- maximized state
- minimized state
- snapped to edge of display
- maximized vertically by dragging the cursor to the top of the display while resizing the window

Can you please update me on what didn't work as expected on your side?

[Don HO]

After applying your patch, the current code in winmain.cpp looks like:
int sw = 0;

    if (::IsZoomed(hNotepad_plus))
        sw = SW_MAXIMIZE;
    else if (::IsIconic(hNotepad_plus))
        sw = SW_RESTORE;
    //else
        //sw = SW_SHOW;

    // IMPORTANT !!!
    if (sw != 0)
        ::ShowWindow(hNotepad_plus, sw);

    ::SetForegroundWindow(hNotepad_plus);

After compiling the code and launching the binary, I "snap" Notepad++ on the left side (so Notepad++ takes a half of screen. I then use context menu to open a file in Notepad++ - It change back (restore) the old position from the left half side position.

The same behaviour on both Windows 8 32 bit and Windows 7 64 bit.

Don

[Ede_123]

Did you consider the cited code change has to be in the executable that is called by the context menu entry, not the executable of the already started instance of Notepad++?

If you compiled a debug version but didn't replace the Notepad++ executable that is called when you open a file via context menu, the changed code isn't executed therefore nothing changes at all.

[Don HO]

I did clean then build to make sure the correct binary will be launched.
I quit Notepad++ after each test.
Each rebuild I relaunch Notepad++.

Don

[Ede_123]

Sorry, I have no doubt you're launching the correct executable.

But when you choose "Edit with Notepad++" in the context menu I suppose a different Notepad++ executable is launched (the one copied by the installer by default to the "Program Files" directory)? The new instance then sends the command to open the file to the already started instance.

The above code has to be in the executable invoked by the context menu entry (not the already started one).

[Pekka Pöyry]

I can confirm that this patch works on my Windows 7 machine.

I used following simple test so that my Notepad++ installation didn't disrupt it:
1. Start notepad++.exe directly from explorer
2. Snap it to left side of the screen.
3. Repeat step 1

Patched build worked correctly, non patched version removed window aero snap.
Everything also worked when I replaced my default installed Notepad++.exe this patched version.

[Don HO]

Hi Pekka,

This patch indeed fix what you described.
However it fixes none of bug described in [#2441], [#2585], [#2889] and [#3396] on both win7 & win8.

Don

[Ede_123]

I just installed version 6.3. Then I applied my patch to winmain.cpp from 6.3 source and compiled it. Finally I replaced the previously installed 6.3 executable with my custom build.

Result: This patch still works and I resolves ALL of the mentioned bugs.

But I've got an idea what could cause the confusion: Is it possible Windows does some strange caching for the executables used in file extension handlers? Because directly after installing unpatched version 6.3, the bugs appeared to be fixed, too. So I assume my previously installed patched build of 6.2.3 was still in memory and used when double clicking on files. Shortly after it stopped working, though.

[Ede_123]

And again for clarity and to be completely sure we're not doing different things:

• This patch does not have to be in the currently running executable of Notepad++.
• This patch has to be in the executable called when opening a file from explorer or using the context menu (this should be the executable in the install directory in "Program Files").

[Pekka Pöyry]

Thanks Don for looking into this matter.

In my configuration I have the latest release of Np++ installed with executable built from svn + this patch. In my configuration aero snap keeps working when I open files from explorer context menu or when open files that are registered as to be opened with Np++.

This patch needs to applied to the executable that Windows context menu is registered to. Thats why it is harder to test this patch. If Windows context menu opens unpatched Np++ executable, this patch won't work.

I mentioned in my last post testing routine that I used to test this patch. In my opinion if that test succeeds, then that executable should fix this problem.
One possible reason that explains that you are not seeing the bug being fixed is that you are not patching the executable, that is registered to handle context menu commands.

Have you tried replacing your installed Np++ executable with patched one?

[Don HO]

winmain_patch.diff (the only patch in this tracker) contains the following lines:

--- winmain_6.2.2.cpp 2012-11-16 00:28:00 +0000
+++ winmain.cpp 2012-11-22 21:57:02 +0000
@@ -303,11 +303,12 @@
sw = SW_MAXIMIZE;
else if (::IsIconic(hNotepad_plus))
sw = SW_RESTORE;
- else
- sw = SW_SHOW;
+ //else
+ //sw = SW_SHOW;

    // IMPORTANT !!!

• ::ShowWindow(hNotepad_plus, sw);
• if (sw != 0)

• ::ShowWindow(hNotepad_plus, sw);

which is to patch winmain.cpp in Notepad++.

I don't see any other patch for NppShell.dll - do I miss something?

Don

[Pekka Pöyry]

No, you are not missing anything.

If I understand this situation correctly, context menu will start Notepad++ executable with opened file as an parameter.
Then this new Np++ instance notices that another Np++ is already open and it sends message to it to open this file. Before sending this open file message, the new Np++ instance will call ShowWindow and SetForegroundWindow to the old Np+ instance to bring it to front. Calling ShowWindow when old Np++ window is not minimized or maximized will cause aero snap to fail, and old Np++ window will restore to old location.

Preventing ShowWindow call when old Np++ instance is not minimized or maximized will fix this bug.

[Ede_123]

Changes to NppShell.dll are not needed. The shell extension calls a new instance of notepad++.exe. This new instance of Notepad++ forwards the command to open the file to the already running instance.

Additionally the new instance calls ShowWindow() on the already running instance to restore its window if it's currently hidden (and that's exactly what the above patch changes: ShowWindow() is only called if the window needs to be restored, not if it is already shown because that causes the error).

[Martinko]

hi i was able to fix it in my computer. Fix is only temorary with very ugly global variables. I would need to rewrite it a little bit so I could paste the code here.
Also could someone tell me what is the process of committing the code to npp?

solution is described here https://sourceforge.net/p/notepad-plus/bugs/3572/

[Pekka Pöyry]

Hi Martinko, thanks for trying to help us with this problem.

I don't wish to sound rude but personally I get a feeling that the solution you suggest is just a hack instead of real fix. Global variables and saving window coordinates when certain events happen sounds very complicated and it can cause other new issues. Trying to keep source code as clean and simple as possible is important in a projects as large as this one.

My opinion is that this patch is the best way to fix this problem and we should rather get it accepted or find other good way to fix this problem. I suggest that you try to fix some other bug, as this bug already has good fix candidate.

Good luck!

[Martinko]

Hello, as I mentioned I only found the problem. I made actual fix next day.
Of course I didn't use global variable :).
I've attached patch to the bug. You can see my solution here.

https://sourceforge.net/p/notepad-plus/patches/460/

I'm using npp with my fix and I didn't notice any incorrect behavior

[Ede_123]

Hmm... does Don still read this? I don't want to nag him about this patch but I'm afraid it gets lost after he decreased it's priority and closed it early being of the opinion it wouldn't fix the mentioned bugs.

[Ede_123]

I resubmitted this patch as [patches:#482] since I'm afraid Don doesn't receive any notifications on this one anymore.

Since this bug fix is that easy (yet extremely useful) it would be a shame to not get it accepted, so any helpful input on the new ticket is highly appreciated!