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

Registry patch #1781

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

nohopestage
Copy link

@nohopestage nohopestage commented Oct 30, 2021

Fixed Min not showing in the Default Programs list, added support for .pdf file extension.

@PalmerAL
Copy link
Collaborator

PalmerAL commented Nov 2, 2021

This seems fine; I'll try to test it in a few days. Since I don't know much about the registry - what's the difference between HKCU / HKCR / HKLM?

@Syndamia
Copy link
Member

Syndamia commented Nov 3, 2021

Correct me if I'm wrong, but I believe:

  • HKEY_CURRENT_USER (HKCU) is for user-specific configurations
  • HKEY_LOCAL_MACHINE (HKLM) is for global configurations
  • HKEY_CLASSES_ROOT (HKCR) is for filename extension information (and some other stuff)

@nohopestage
Copy link
Author

nohopestage commented Nov 3, 2021

Correct me if I'm wrong, but I believe:

  • HKEY_CURRENT_USER (HKCU) is for user-specific configurations
  • HKEY_LOCAL_MACHINE (HKLM) is for global configurations
  • HKEY_CLASSES_ROOT (HKCR) is for filename extension information (and some other stuff)

You are right. I don't really know why it didn't work with HKCU though, I just noticed all the other browsers having their keys in HKLM.

Fixed the keys not being deleted on uninstall. Corrected a typo. Added a function to delete the values that are not being deleted with the keys.
@nohopestage
Copy link
Author

nohopestage commented Nov 4, 2021

Apparently, the original problem has nothing to do with HKCU/HKLM. For some reason the installer creates empty values under HKCU/SOFTWARE/Classes/Min in its subkeys. Specifically, an empty value in ../Min/shell/open/command causes the problem. At least setting it manually fixed the issue on Windows 10. Not sure if that's the case for Windows 7 though, as I remember setting the value manually with no result. I'll test it once again, please don't merge for now.

Hopefully fixed some keys not being created on install, minor changes.
@nohopestage
Copy link
Author

nohopestage commented Nov 4, 2021

Seems like Windows 7 requires the key to be under HKLM:

The default installation of Windows does not register a per-user default Internet or email program, only a system-wide default. This provides a smooth upgrade path from previous versions of the operating system, in which only the HKEY_LOCAL_MACHINE subtree is supported for client registrations.

Regarding the empty values issue: some values in the array were named without single quotes, which could've caused the problem. Everything should be fine now.

@PalmerAL
Copy link
Collaborator

This isn't working for me so far (Windows 10 20H2). It looks like the HKCR\Min keys are being set, but none of the HKLM\SOFTWARE\Clients\StartMenuInternet keys are showing up in the registry editor. I can still set Min as the default browser (although that may be from registry keys left over from an earlier installation), but I can't set it as the default for PDF.

Will try debugging in a bit, but let me know if you have any ideas.

@nohopestage
Copy link
Author

nohopestage commented Nov 12, 2021

That is weird, I've tried running the script from node command line and it was working fine. Is the value under RegisteredApplications being created? Does the Min key under HKCR have all its subkeys and values? Is the Min value under HKCR/.pdf/OpenWithProgIds there? If so, you may need to manually set Min as the default for .pdf in Default apps > Set defaults by app. You can send me the build so I could test it on Windows 7 too.

@PalmerAL
Copy link
Collaborator

I tried running the script directly from the node CLI (good idea!), and if I modify it to print out errors from the regedit functions, I get this:
Screen Shot 2021-11-14 at 7 26 19 PM
If I rerun as an administrator, I don't get any errors, and the keys show up in the registry editor. So I think the issue is that writing to HKCR / HKLM requires administrator access, but when the installer launches an instance of Min to write to the registry, it's not running as administrator. I'm not exactly sure how to fix this, but this issue suggests using https://www.npmjs.com/package/sudo-prompt, which looks like a decent solution.

Although when you were running it with Node, were you able to make it run successfully without administrator access?

@nohopestage
Copy link
Author

Ah, that makes sense. Sorry, writing to HKCR / HKLM does require administrator privileges, which I forgot to mention. There seems to be an easier solution though.

@PalmerAL
Copy link
Collaborator

I haven't tried it, but that looks like it would make Min run as administrator all the time, rather than just during the installation (which is what we want).

@nohopestage
Copy link
Author

nohopestage commented Nov 16, 2021

Oh, actually yes, but isn't it also the case for the solution you mentioned? Apparently, there's no option to require a Squirrel installer to run with admin rights.

@PalmerAL
Copy link
Collaborator

sudo-prompt executes a terminal command as root; I was thinking that we could run registryConfig.js in its own subprocess (using the electron binary and electron_run_as_node), and then we would only need to prompt when we want to update the registry during install.

The installer isn't running the script - it's launching an instance of Min, which then checks for a flag and runs the registry config if it's present: https://github.com/minbrowser/min/blob/master/main/main.js#L40-L42. So even if the installer ran as admin it wouldn't help.

@nohopestage
Copy link
Author

That makes sense. sudo-prompt is the way to go.

@PalmerAL
Copy link
Collaborator

I think 588a0cf is close to the right way to do it. If I run it directly with node_modules\.bin\electron . --squirrel-install it works, but if I build the installer and run that, the browser hangs on startup. I'm not sure why yet; maybe you'll see the issue.

@nohopestage
Copy link
Author

nohopestage commented Dec 3, 2021

Oh, I wish I could. I'm not familiar with JS/Electron, so I see nothing wrong there. At least you can send me the build to see if I can reproduce.

@PalmerAL
Copy link
Collaborator

I finally came back to this PR; I think the hang I was seeing before was actually a graphics issue with my VM. If I change the shortcut to Min to run with --disable-gpu it seems to be working now. Or maybe something else changed in the meantime 🤷

I can set Min as the default handler for PDF files now, but if I try to open one, I get stuck in this loop:

Screen.Recording.2022-01-15.at.5.53.33.PM.mov

@nohopestage
Copy link
Author

I was not able to reproduce this bug, it might have something to do with your VM. Could you please send me the installer, so I could try to reproduce it that way? I've only tried to run registryConfig.js in node CLI.

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.

3 participants