search

activescott / lessmsi

A tool to view and extract the contents of an Windows Installer (.msi) file.
https://lessmsi.activescott.com
MIT License
1.29k stars 150 forks source link

feat: Store settings in the application folder #160

Closed learn-more closed 2 years ago

learn-more commented 3 years ago

137

activescott commented 3 years ago

Why this? It adds file I/O logic for one of the things that should be automatic. Also writing such settings into app folder won't work if it is in program files or similar and seems kinda less than ideal to write data into a program directory and instead of a per-user location.

learn-more commented 3 years ago

Why this? It adds file I/O logic for one of the things that should be automatic. Also writing such settings into app folder won't work if it is in program files or similar and seems kinda less than ideal to write data into a program directory and instead of a per-user location.

The issue requested to store it in the application folder, I assumed that is what you meant with 'locally'

As far as I know, the only way to use the application settings is to basically re-write the entire provider anyway, so I found it easier to just write it entirely from scratch. (And probably less code as well.)

To use the existing application settings infra, one would have to implement a 'SettingsProvider', which also means writing and reading somewhere manually.

activescott commented 3 years ago

Sorry I overlooked the "#137" in the PR description. I like the idea of portable, but now that I saw the PR it did make me think that in a "traditional install" for windows where apps are installed into "Program Files" apps won't be able to write into that directory. We don't actually provide an msi that installs there so it may not be common among lessmsi users. I also checked Chocolatey (which we do provide a popular package for) and apparently Chocolatey does allow writing to the app directory (I did not test though).

Since it is somewhat unorthodox to write data into the same directory as the app though, I wonder if maybe we shouldn't have a fallback to a user-specific, safe location to write settings if we don't have write/create permission or should we just fail and not save settings? What do you think?

learn-more commented 3 years ago

Sorry I overlooked the "#137" in the PR description. I like the idea of portable, but now that I saw the PR it did make me think that in a "traditional install" for windows where apps are installed into "Program Files" apps won't be able to write into that directory. We don't actually provide an msi that installs there so it may not be common among lessmsi users. I also checked Chocolatey (which we do provide a popular package for) and apparently Chocolatey does allow writing to the app directory (I did not test though).

Since it is somewhat unorthodox to write data into the same directory as the app though, I wonder if maybe we shouldn't have a fallback to a user-specific, safe location to write settings if we don't have write/create permission or should we just fail and not save settings? What do you think?

One approach that comes to mind is this: The most clean solution would probably be, first try to load it from the application folder, Then fall back to %appdata%, and possibly even fall back to %localappdata%.

Since your requested a minimal change, I did not write code like that, but if you prefer that (and with a bit more robust error handling maybe) then I can rework the code into something like this. Please note, that I might add another class depending on how much code there is going to be.

Do you think the location I chose for the new code 'Model' is correct? If not, please suggest a better place.

learn-more commented 3 years ago

Left a couple minor notes on the code. If you could look at those then I'll do a deeper dive and test this one out and as long as I don't run into any obvious errors I'll merge.

Please do make sure that you have tested it. If you could please test in c:\Program Data... like chocolatey uses, and in c:\Program Files...\ (where it should fail gracefully) and confirm I will really appreciate it!

Thanks again for contributing!

I test all code submitted ;) But I did not test it in multiple places, nor how it behaves in a readonly dir. When we reach a consensus how to implement this exactly, I'll make sure to test the code in multiple places, and include in a new PR comment what I tested.

activescott commented 3 years ago

Sorry I overlooked the "#137" in the PR description. I like the idea of portable, but now that I saw the PR it did make me think that in a "traditional install" for windows where apps are installed into "Program Files" apps won't be able to write into that directory. We don't actually provide an msi that installs there so it may not be common among lessmsi users. I also checked Chocolatey (which we do provide a popular package for) and apparently Chocolatey does allow writing to the app directory (I did not test though). Since it is somewhat unorthodox to write data into the same directory as the app though, I wonder if maybe we shouldn't have a fallback to a user-specific, safe location to write settings if we don't have write/create permission or should we just fail and not save settings? What do you think?

One approach that comes to mind is this: The most clean solution would probably be, first try to load it from the application folder, Then fall back to %appdata%, and possibly even fall back to %localappdata%.

Since your requested a minimal change, I did not write code like that, but if you prefer that (and with a bit more robust error handling maybe) then I can rework the code into something like this. Please note, that I might add another class depending on how much code there is going to be.

Yeah, I don't think we need such robust fallback. I'm fine to just drop/loose the saved settings if it can't save for now. If people complain we can always add the feature to fallback to a user-specific directory later. Again I apologize for being a stickler, but I am guessing there are several thousand people using this utility regularly based on how many people download a new version in chocolatey in just a few days so I want to make sure it at least isn't crashing or particularly problematic for users.

Do you think the location I chose for the new code 'Model' is correct? If not, please suggest a better place.

No concerns on the code location.

Thanks again and please to tag me when you want me to take another look.

activescott commented 2 years ago

@learn-more I'm also going to go ahead and close this since it's quite stale, but feel free to re-open it if you're interested. I'd be happy to pitch in and help you get it over the line!

learn-more commented 2 years ago

@activescott the net win for me is not big enough on this feature (I would not use it myself) to justify the risk of breaking something / the extra work required to properly validate it.

If someone wants to continue on this where I left off, they are free to take my code. I am not sure how github handles PR's for deleted branches, but I will delete my branch for this somewhere in the future.