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

WORK IN PROGRESS - Install for all users #55

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

GurliGebis
Copy link
Contributor

Changes for installing for all users

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora Would you be able to take a look at this, and see if you can spot what I'm doing wrong? :)

@ddomingos-encora
Copy link

ddomingos-encora commented Nov 20, 2023

It seems it is all good. I'll try to test it once I restore my system (probably tomorrow). I'll let you know

Installer.cpp Outdated Show resolved Hide resolved
@GurliGebis GurliGebis force-pushed the install-for-all-users branch from 72e76c3 to 1bcc21b Compare November 20, 2023 18:59
@ddomingos-encora
Copy link

@GurliGebis In the other discussion, you mentioned this: "The Get-AppxPackage cmdlet shows it as installed, but it isn't registered". What field tells you it is not registered? I did not find such field. Also, do you see any difference in the output of the Get-AppxPackage command when you install the official Notepad++ package and when you install the package generated by the code in this PR?

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora if you run that command, and it shows up in the list, it is installed 🙂
So that is what I meant by it showing it as installed.
What I meant was basically, it is installed, but it doesn't look like it is working (no dllhost.exe process is spawned when a file is right clicked, so the code isn't even being run)

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora could it be an ACL issue? (Just thinking aloud)

@GurliGebis GurliGebis force-pushed the install-for-all-users branch from 1bcc21b to 9183923 Compare November 21, 2023 21:24
@ddomingos-encora
Copy link

ddomingos-encora commented Nov 22, 2023

Hi @GurliGebis I managed to restore my system and test today. It "works" but requires a restart or sign off/sing in. Since the installation of notepad++ does not require a restart, it is probably not a final solution. Still, you might want to keep both this logic and the ensure registration logic, since the ensure registration logic also has a blind spot of not displaying the menu the first time when that code is executed (if was not installed before), so the chance of that happening for other users is reduced. At least the remove for all users flag I think you should keep. If you want to keep the provisioning code as well, I'd use both staging/provisioning and add code, so that it gets installed for the user running the installer.
One idea could be: instead of changing the current code for registering, we would add a new function for provisioning. Then the installation path would call both but the ensure registration logic would call only the one for registration (as today). Let me know what you think. Regards

@GurliGebis
Copy link
Contributor Author

I totally agree @ddomingos-encora 🙂
I'll update the PR with it added back, can you take a look at it afterwards and see if it looks fine?

@GurliGebis GurliGebis force-pushed the install-for-all-users branch 2 times, most recently from 37f43ac to 2763e04 Compare November 22, 2023 11:45
@GurliGebis
Copy link
Contributor Author

@ddomingos-encora There is a problem - it works fine for the first user now.
However, since the package has been provisioned, and is installed on all users - the EnsureRegistrationOnCurrentUser function thinks it is installed (which it technically is), and does nothing.
Any ideas?

@ddomingos-encora
Copy link

Yes, I thought about this later yesterday, after I posted. I was wondering if there could be a difference between finding the package for all users or the current user but I just saw you already implemented this way. The only thing I can think of now would be the Package class itself:
https://learn.microsoft.com/en-us/uwp/api/windows.applicationmodel.package?view=winrt-22621
Maybe some functions like Status can be helpful, or even InstalledDate. Maybe one of them can indicate us that the package is staged but not actually installed. I'll do some tests later today

@GurliGebis
Copy link
Contributor Author

Please do.
From what I could come up with (didn't have long to test, so didn't spend a lot of time on it) - it just looks to be installed correctly, but not loading for some reason.
I even tried setting "Everyone" to "Full control" on the folder, to ensure it isn't ACL related, but that doesn't seem to make any difference.

@ddomingos-encora
Copy link

Look at this explanation here:
microsoft/WindowsAppSDK#2099 (comment)
It seems we are doing the right thing. There is another comment where the same guy says that for the current user we should also Add (besides staging and provisioning).
The only thing that is not clear to me is why the other users are not getting it installed.

@ddomingos-encora
Copy link

I did a few more tests but could not find an attribute that could help us to differentiate that situation, nor even establish when the package gets installed to other users. Sometimes it seems to require a restart, sometimes it doesn't get installed at all. It seems just buggy to me. Unfortunately, I gave up. I think we should change the title of the PR to uninstall to all users and keep only the RemoveForAllUsers flag. Regards

@ddomingos-encora
Copy link

ddomingos-encora commented Nov 28, 2023

@GurliGebis I was doing some tests and for a moment I thought that the RemoveForAllUsers flag was not working but it is. I think this is a good improvement we can take out of all these attempts. Regards

@GurliGebis
Copy link
Contributor Author

@ddomingos-encora I think so as well.
I wonder if it works better on the current insider canary builds of Windows 11, since they might have made changes.
I'll have to test that later.

@GurliGebis GurliGebis force-pushed the install-for-all-users branch from 2763e04 to 5e410dd Compare June 6, 2024 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants