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

notepad++ 8.5.1 no icon when launching from the context menu (edit file) #13410

Closed
AvenCores opened this issue Mar 25, 2023 · 45 comments
Closed
Assignees
Labels

Comments

@AvenCores
Copy link

windows 11 last bild
npp.8.5.1.Installer.x64.exe

no icon when launching from the context menu (edit file)
image

but if you just run notepad++ as a normal application, the icon is displayed
image

@donho
Copy link
Member

donho commented Mar 25, 2023

Indeed, I can reproduce it.

@GurliGebis
Not sure if it's link to NppModernShell.dll - have you any idea about it?

@donho donho added the accepted label Mar 25, 2023
@donho donho self-assigned this Mar 25, 2023
@donho
Copy link
Member

donho commented Mar 25, 2023

BTW, @AvenCores Could you provide the debug info via menu ?->Debug Info... please ?

@AvenCores
Copy link
Author

BTW, @AvenCores Could you provide the debug info via menu ?->Debug Info... please ?

Notepad++ v8.5.1 (64-bit)
Build time : Mar 21 2023 - 19:16:10
Path : C:\Program Files\Notepad++\notepad++.exe
Command Line :
Admin mode : OFF
Local Conf mode : OFF
Cloud Config : OFF
OS Name : Windows 11 Pro (64-bit)
OS Version : 22H2
OS Build : 22621.1413
Current ANSI codepage : 1251
Plugins :
mimeTools (2.9)
NppConverter (4.5)
NppExport (0.4)

@GurliGebis
Copy link
Contributor

GurliGebis commented Mar 25, 2023

@donho the code reads the current path of the dll, combine that with notepad++.exe
The problem is that the RC2 it is based on is only half finished - so instead we should get a new test build with my latest PR, which people with issues can be pointed to.
No point in using a lot of energy debugging something that has already been changed alot in the next release.

@donho
Copy link
Member

donho commented Mar 27, 2023

@GurliGebis
So this bug has not been fixed in the binaries we have tested - I have reproduced with both Notepad++ 32 & 64 installer under Windows 11.

@GurliGebis
Copy link
Contributor

@donho okay, I'll try and see if I can reproduce it.
It is a bit weird, since all that it does is call CreateProcess to launch it.

@GurliGebis
Copy link
Contributor

@donho is the issue still present in TEST5?

@donho
Copy link
Member

donho commented Mar 27, 2023

@GurliGebis

It is a bit weird, since all that it does is call CreateProcess to launch it.

It works - Notepad++ has been launched and the file is opened in launched Notepad++.
It's just that the icon of application is not shown on the taskbar.

is the issue still present in TEST5?

Yes. As I said, I can reproduce in TEST5 (both x32 & x64), though they are not "clean" installation.

@GurliGebis
Copy link
Contributor

@donho I can reproduce it myself.
It seems to be that when the process is created using CreateProcess, the icon is not used for some reason.
I have tried changing it to run notepad.exe instead, and there the icon is working fine.

So there is something with the notepad++.exe that explorer doesn't like, when it is run like this.

I have tried running it from a simple console program, and it does the same, so it is not related to this DLL (or to be more specific, it is not related to being run from within the DLL), but something about it being starting using CreateProcess.

I'll see if I can figure out why CreateProcess doesn't want to let the icon show.

@GurliGebis
Copy link
Contributor

@donho if I hardcode it like this:

const wstring EditWithNppExplorerCommandHandler::GetNppExecutableFullPath()
{
    return L"C:\\Windows\\System32\\notepad.exe";
}

It starts the normal notepad.exe, and it keeps its icon just fine.
I don't know what the cause of this is, but since we can start programs with the CreateProcess, I'm at a bit of a loss.

Any ideas?

@GurliGebis
Copy link
Contributor

okay, it seems like it is related to it being spawned inside dllhost for some reason

@GurliGebis
Copy link
Contributor

@donho do you have any ideas?

@donho
Copy link
Member

donho commented Mar 27, 2023

@GurliGebis
In old NppShell.dll, which function was used for calling Notepad++ ?
IMO, if we use the same function for launching Notepad++, it will work.

@GurliGebis
Copy link
Contributor

@donho is there a version where it works?

@GurliGebis
Copy link
Contributor

All of them have been using CreateProcess for it - I have tried with ShellExecute, but that gives the same result.

From what I can see, it only happens with the notepad++.exe binary, every other program I have tried with has it working just fine.
So I think some changes are needed in there, but I have no idea of what those changes are.

@donho
Copy link
Member

donho commented Mar 27, 2023

@GurliGebis
You can take v8.5 in which NppShell.dll source code was untouched:
https://github.com/notepad-plus-plus/notepad-plus-plus/archive/refs/tags/v8.5.zip

@donho
Copy link
Member

donho commented Mar 27, 2023

@GurliGebis
I can check it right now with my laptop.
However, I'm is in urgency with my wife for my mother in law, so I will be unavailable in any moment.

@GurliGebis
Copy link
Contributor

@donho yes, that used the old IContextMenu stuff, which works completely different.
So that is not an option.

Sure, no worries.
If I run notepad.exe, it seems like it does something, so the dllhost.exe isn't the parent process.
I'll investigate further

@donho
Copy link
Member

donho commented Mar 27, 2023

yes, that used the old IContextMenu stuff, which works completely different.
So that is not an option.

How about ShellExecute()?
It works very well in Notepad++ code base - search in Notepad++ and they are used everywhere.

@GurliGebis
Copy link
Contributor

@donho I have already tried that one, the result is the same (as mentioned above).

@GurliGebis
Copy link
Contributor

We might need help from someone who knows a bit more about programming against the Windows UI than I do.

@donho
Copy link
Member

donho commented Mar 27, 2023

I have already tried that one, the result is the same (as mentioned above).

How do you use it? Could you provide the code please?

if I hardcode it like this:

const wstring EditWithNppExplorerCommandHandler::GetNppExecutableFullPath()
{
return L"C:\Windows\System32\notepad.exe";
}

It starts the normal notepad.exe, and it keeps its icon just fine.

Can you put MessageBox into

const wstring EditWithNppExplorerCommandHandler::GetNppExecutableFullPath()
{
    const wstring path = GetInstallationPath();
    const wstring fileName = L"\\notepad++.exe";
const wstring fullFilePath = L"\"" + path + fileName + L"\""
MessageBoxW(NULL,fullFilePath.c__str()  ...)
    return fullFilePath  ;
}

@GurliGebis
Copy link
Contributor

This is how I tried with ShellExecuteEx:

wstring applicationName = GetNppExecutableFullPath();

wchar_t* application = (LPWSTR)applicationName.c_str();

SHELLEXECUTEINFO sei = { 0 };

sei.cbSize = sizeof(SHELLEXECUTEINFO);
sei.lpVerb = L"open";
sei.lpFile = applicationName.c_str();
sei.nShow = SW_SHOWNORMAL;
ShellExecuteExW(&sei)

That ignores which file is selected (just to test), but it still shows nothing.

The full file path is: "C:\\Program Files\\Notepad++\\notepad++.exe"

@GurliGebis
Copy link
Contributor

From what I can see from examples around the internet, we are executing it correctly.
Looking at it from Process Monitor, I can see the commandline it is being run with, and that looks normal.

@GurliGebis
Copy link
Contributor

Just compared the info on the notepad++ process when open normally and using the extension - everything is the same, except the parent process is dllhost instead of explorer.
But if the problem was with dllhost being the parent process, then it should also fail with notepad.exe and other programs, which it doesn't.

I have to go for tonight.

@GurliGebis
Copy link
Contributor

Just tried one more thing, and that makes it even more weird:

const std::wstring contextMenuParameter = L"--contextmenu";

if (isInList(contextMenuParameter.c_str(), params))
{
wchar_t path[FILENAME_MAX] = { 0 };
GetModuleFileName(NULL, path, FILENAME_MAX);

STARTUPINFO si;
PROCESS_INFORMATION pi;

ZeroMemory(&si, sizeof(si));
si.cb = sizeof(si);
ZeroMemory(&pi, sizeof(pi));

std::wstring newCommandLine(pCmdLine);
size_t pos = newCommandLine.find(contextMenuParameter);
if (pos != std::wstring::npos)
{
	newCommandLine.replace(pos, contextMenuParameter.size(), L"");
}

newCommandLine.erase(newCommandLine.begin(), std::find_if_not(newCommandLine.begin(), newCommandLine.end(), [](wchar_t c)
{
	return std::isspace(c);
}));

newCommandLine.erase(std::find_if_not(newCommandLine.rbegin(), newCommandLine.rend(), [](wchar_t c)
{
	return std::isspace(c);
}).base(), newCommandLine.end());

std::wstring appName = std::wstring(path);
std::wstring cmdLine = L"\"" + appName + L"\" " + newCommandLine;

wchar_t* application = (LPWSTR)appName.c_str();
wchar_t* command = (LPWSTR)cmdLine.c_str();

if (!CreateProcessW(application, command, nullptr, nullptr, false, 0, nullptr, nullptr, &si, &pi))
{
	return 1;
}

return 0;
}

Added this to wWinMain and tested from a command prompt, if I run it with --contextmenu filename.txt it spawns a new version with the correct icon.
If I do that same thing from the context menu handler, it spawns it without the icon.
So it isn't even the parent process that is the cause of it, since that is the same for both of them (since notepad++.exe starts notepad++.exe, it is the parent of the final process).

I'm out of ideas for now

@GurliGebis
Copy link
Contributor

Could it be some kind of permission token that is different, since it is initiated from dllhost, compare to when it is initiated from explorer?
I will try with notepad.exe copied into the notepad++ folder tomorrow

@ozone10
Copy link
Contributor

ozone10 commented Mar 27, 2023

IIRC CreateProcess() might require https://learn.microsoft.com/en-us/windows/win32/sbscs/application-manifests#msix to have identity to work properly.

@donho
Copy link
Member

donho commented Mar 28, 2023

@GurliGebis

if I hardcode it like this:

const wstring EditWithNppExplorerCommandHandler::GetNppExecutableFullPath()
{
return L"C:\Windows\System32\notepad.exe";
}

It starts the normal notepad.exe, and it keeps its icon just fine.

I think maybe we should dig here.
If you hard code as following:

const wstring EditWithNppExplorerCommandHandler::GetNppExecutableFullPath()
{
    return L"C:\\Program Files\\Notepad++\\notepad++.exe";
}

Does it work for x64 binary?
If it does, then try return L"C:\\Program Files (x86)\\Notepad++\\notepad++.exe"; on 32 bits Notepad++.

If both work, then I guess (I could be wrong very probably) that the function which returns a const string is optimized by the compiler, and for the reason of that I am not aware (race condition?) that dllhost (I just use your term) system retrieves app icon if the dll has provided on time (statically on compiling time, but not dynamically on runtime).

@GurliGebis
Copy link
Contributor

@donho okay, I have made some progress.

If I move notepad.exe into the Program Files folder, it doesn't work at all (I guess it tries to load some dll files directly from it's path, since it normally live in system32, which is rich with dll files).

HOWEVER - and now it gets interresting - If I copy Notepad++.exe into C:\Windows\System32 and hardcode that path into the dll (and ignore it's errors about missing xml files), the icon WORKS!!!!

So now the question is - why does it work in System32 and not in Program Files.

My theory is that it is related to file system permissions - I'll keep digging - but now we are getting somewhere 😀

@GurliGebis
Copy link
Contributor

Okay, I can confirm it is a permission issue.

If I run this:

$perm = Get-Acl C:\Windows\System32\notepad.exe
Get-Item -Path 'C:\Program Files\Notepad++' | Set-Acl -AclObject $perm

And then change the permissions to be inherited on child items (since it only sets it on the folder), it works!

So, now to figure out what the difference is between the default permissions and the ones we have on notepad.exe, so we can figure out what is missing.
Once I have figured that out, it should be somewhat trivial (like I haven't said that before.....) to change the NppShell.dll to modify the exe permissions.

@GurliGebis
Copy link
Contributor

Okay, I got it figured out - now to fix it.

@GurliGebis
Copy link
Contributor

@donho are you ready?

Here we go.....

When registering a sparse package, the directory it is located in gets the following ACL set:
screenshot

This causes the application to be unable to read the icon, when it is being loaded in the context of the context menu.
So, the solution I have come up with (since I don't want to manipulate ACLs (since that is fragile, and can break at any moment, depending on user permissions), is to install the NppShell.dll and NppShell.msix files inside a subfolder 😀
This causes it to only change the permissions on the subfolder, leaving the mainfolder alone, causing it to work.

I will create two pull requests - one for the NppShell project, that requires it to be inside a subfolder, and another one for this repo that updates the installer to install those files inside a subfolder called contextmenu

Now it all starts to makes sense.

@GurliGebis
Copy link
Contributor

@donho first step is to merge this PR and then let me know - once that is done, I'll update the #13411 PR to point to that and have the files in the correct folder.
Then we should be good to go.

PR in NppShell: notepad-plus-plus/nppShell#6

@GurliGebis
Copy link
Contributor

GurliGebis commented Mar 28, 2023

@donho I have updated the #13411 with the installer changes - it still points to the HEAD of NppShell, so I'll update that part after you have merged the PR mentioned above.

To see the changes needed for the installer (I hope, since I cannot build that and test): https://github.com/notepad-plus-plus/notepad-plus-plus/compare/9d7b7b45089b1a3febc6bca5b24b69263696216f..91518508445a228d7bae3b570ae153dc8109fb85

@donho
Copy link
Member

donho commented Mar 28, 2023

@GurliGebis
That's a good news!
Sorry, due to my mother in law, I'm always in hospital.
I can do a build when I get home (around 22:00 Paris time)

@GurliGebis
Copy link
Contributor

@donho I am sorry to hear that.
No hurry, I have to fix something first, since it needs to reset the folder permissions if upgrading, since they will be wrong if going from 8.5.1, since it has had the package registered there.
I will ping you from the other PR when it is ready.

@donho
Copy link
Member

donho commented Mar 28, 2023

BTW, have you tried what @ozone10 has suggested ?

@GurliGebis
Copy link
Contributor

@ozone10 and @donho yes, we already have the msix tag in the notepad++.exe manifest 🙂
I have also tried changing the manifest to have the complete common name from the certificate, which doesn't make a difference.
The issue is file system permissions, so I'm going with moving the dll and msix into a subdirectory, which solves it nicely.

@GurliGebis
Copy link
Contributor

@donho I just tested notepad-plus-plus/nppShell#6, and it is working.
So if you can merge that when you get home, and then, let me know, I'll update the #13411 PR, and you can then build a new installer, and we should be good to go (at least if my NSIS changes don't blow up) 😀🚀

@donho
Copy link
Member

donho commented Mar 28, 2023

@GurliGebis
Merged. You go now.

@GurliGebis
Copy link
Contributor

@donho thanks, I have pushed the #13411 branch again, please pull it, rebuild the nppshell dll with the head of nppshell, and build a TEST6 installer from it 🙂

@GurliGebis
Copy link
Contributor

I'm away for the next hour 🙂

@donho
Copy link
Member

donho commented Mar 30, 2023

Fixed by notepad-plus-plus/nppShell@e4f9037

@lmstearn
Copy link

Hey there, just a quickie about the (not so monstrous) icon in the right click context menu for "Edit with Notepad++", posted an inf file for the Admin version. The line in question has:
CONTEXTPATH="C:\Program Files\Notepad++\contextMenu\NppShell.dll"
HKCR,*\Shell\runas,"Icon",,"%CONTEXTPATH% ""%1"""
Assuming here the icon is in NppShell.dll, is that right, and would it have a different number to the one given?
Thanks very much for N++ ! 😄

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

5 participants