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

Add innosetup postuninstall cleanup of extra files #1058

Closed
wants to merge 11 commits into from

Conversation

mpheath
Copy link
Collaborator

@mpheath mpheath commented Jul 30, 2021

Adds the option for a user to remove extra files at uninstall of Sandboxie-Plus.

cleanup.png

The messagebox will not display if the install is silent.

Default button is No, as removing {win}\Sandboxie.ini should be an explicit Yes as the file may have been updated.

Current files to remove are:

  • {localappdata}\{#MyAppName}\{#MyAppName}.ini
  • {win}\Sandboxie.ini

A TStringList type is used so adding more files later can be done by

Paths.Append('another_path_to_remove');

The UninstallDelete section will remove empty directories of:

  • {app}
  • {localappdata}\{#MyAppName}

Adds a custom message of english.UninstallRemoveFiles=Remove these files?. Adds ChineseSimplified and Italian lines which are commented, ready for translation.

@isaak654
Copy link
Collaborator

I'm going to test this locally as soon as possible!

@isaak654 isaak654 added the Work in progress Still in progress label Jul 30, 2021
@DavidXanatos
Copy link
Member

Well if we want a thorough clean up we should also remove the C:\Sandbox folder

@mpheath
Copy link
Collaborator Author

mpheath commented Jul 30, 2021

There is not just "C:\Sandbox" folder, but other Sandbox paths that can set in Sandboxie.ini. This could be a rabbit hole with many paths. "C:\Sandbox" is easy to locate and delete by the user. Any other Sandbox locations is something that the user should know.

Would it be remove to empty folders, remove non-empty folders or remove folders with just desktop.ini in them and possibly including the username subfolder or a custom named username subfolder? Perhaps some other scenerios might exist. This seems like something that the user may know better how to handle.

Programs like VirtualBox do not ask to remove "%userprofile%\VirtualBox VMs" if I remember correctly. I just consider this as acceptable.

Files hidden away in %AppData% is a benefit to remove by the installer that is controlled the user, as they can be differcult for the average user to know about, yet to locate and to remove.

@DavidXanatos
Copy link
Member

hmm yea but we could make it work by providing a sandman.exe command line option to delete all sandboxes and the root location, it would do that before unloading driver etc this way it could query all the paths.

@mpheath
Copy link
Collaborator Author

mpheath commented Jul 30, 2021

If sandman.exe can do it, then perhaps it could be run from the UninstallRun section. Show a Gui with checkboxes to select the options, as a user may want to remove the files, though keep the Sandbox folders. That may take some testing to see if can work OK. Otherwise could show 2 msgboxes, 1 msgbox for the removal of files that this PR is already does and 1 msgbox for sandbox removal that runs sandman.exe to remove sandboxes.

Thinking about it now, deleting the Sandboxie.ini does lead to Sandbox removal as the settings, paths information... get deleted with the removed ini file. It is really like a big red button to nuke Sandboxie.

@isaak654
Copy link
Collaborator

isaak654 commented Jul 30, 2021

@DavidXanatos

hmm yea but we could make it work by providing a sandman.exe command line option to delete all sandboxes and the root location, it would do that before unloading driver etc this way it could query all the paths.

Not without introducing it for Sandboxie Classic too... it has the same problem of C:\Sandbox leftovers.

@mpheath

Thinking about it now, deleting the Sandboxie.ini does lead to Sandbox removal as the settings, paths information... get deleted with the removed ini file. It is really like a big red button to nuke Sandboxie.

Not physically, I still have the C:\Sandbox with the DONT-USE.TXT file and the Username subfolder after selecting NO in Sandboxie Classic and rebooting:

uninstall_settings_Classic

It would also be a good idea to copy partially the text from the Classic dialog, as it looks more informative:

8054;ins;01
Would you like to keep your Sandboxie.ini configuration file?
Select YES to keep configuration settings in the Sandboxie.ini file.
Select NO to delete the Sandboxie.ini file and lose your configuration settings.

@mpheath
Copy link
Collaborator Author

mpheath commented Jul 31, 2021

I am not keen on the idea of removing sandboxes and configuration files with one click of a button. A second msgbox could be shown to confirm if yes was selected. This is because too much could be lost if the user is distracted at that time.

Another condition that might be easier to implement, is for the user to shutdown Sandman, remove "C:\Sandbox" and then do the uninstall. It can then be detected that "C:\Sandbox" does not exist, so the uninstall will offer to remove the configuration files as they may no longer be needed. The bonus of this condition is that the average Sandboxie user may not often see the msgbox to remove the configuration files as they would keep "C:\Sandbox". This can allow a uninstall and a reinstall to happen without the concern of answering a msgbox. This also allows a possibly unhappy user (or other) to remove "C:\Sandbox" and then do the uninstall, and the msgbox will show to help with removing the configuration files.

The later may take just a few extra lines in the code section to implement. A similar condition could possibly be added to the Sandboxie Classic uninstaller to behave similar. The msgbox strings as isaak654 mentions, could then be more generally descriptive, instead of one line and the listing of paths as this current PR does. The strings would be different as Sandboxie Plus also has the Sandboxie-Plus.ini for Sandman.

A concern might be if "{sd}\Sandbox" in InnoSetup constant syntax, is somewhere else. Example, user has changed Sandboxie.ini FileRootPath to use another path or drive as the main default Sandbox location. This user may then see the msgbox always on uninstall. This may be unfortunate side effect of using a non standard default Sandbox location. A workaround could be to use GetIniString and check if FileRootPath is the default \??\%SystemDrive%\Sandbox\%USER%\%SANDBOX% or '' and do not show the msgbox if it is different.

Tested change of FileRootPath from Sandbox to Sandbox1. A Sandbox1 can be created on use though Sandbox remains after a system restart. I guess Sandbox can be removed manually. For this change, the msgbox did not show.

So, code added into PostUninstall_Cleanup() decides if to continue with showing the msgbox:

  // check if valid to continue
  FileRootPath := '\??\%SystemDrive%\Sandbox\%USER%\%SANDBOX%';

  if DirExists(ExpandConstant('{sd}\Sandbox')) then
    exit;

  if GetIniString('GlobalSettings', 'FileRootPath', FileRootPath,
                  ExpandConstant('{win}\Sandboxie.ini')) <> FileRootPath then
    exit;

Thoughts?

@isaak654
Copy link
Collaborator

isaak654 commented Jul 31, 2021

I am not keen on the idea of removing sandboxes and configuration files with one click of a button. A second msgbox could be shown to confirm if yes was selected. This is because too much could be lost if the user is distracted at that time.

I agree, I just wanted to show up also the Classic behavior because it's important to align these types of changes in both editions.

Another condition that might be easier to implement, is for the user to shutdown Sandman, remove "C:\Sandbox" and then do the uninstall. It can then be detected that "C:\Sandbox" does not exist, so the uninstall will offer to remove the configuration files as they may no longer be needed. The bonus of this condition is that the average Sandboxie user may not often see the msgbox to remove the configuration files as they would keep "C:\Sandbox". This can allow a uninstall and a reinstall to happen without the concern of answering a msgbox. This also allows a possibly unhappy user (or other) to remove "C:\Sandbox" and then do the uninstall, and the msgbox will show to help with removing the configuration files.

A concern might be if "{sd}\Sandbox" in InnoSetup constant syntax, is somewhere else. Example, user has changed Sandboxie.ini FileRootPath to use another path or drive as the main default Sandbox location. This user may then see the msgbox always on uninstall. This may be unfortunate side effect of using a non standard default Sandbox location. A workaround could be to use GetIniString and check if FileRootPath is the default \??\%SystemDrive%\Sandbox\%USER%\%SANDBOX% or '' and do not show the msgbox if it is different.

It's good for the average Sandboxie user, but it doesn't show to the advanced user that for cleaning the configuration files, you need to remove the Sandbox folder. Your logic is fine, I just think that it should be presented (in some way) for better transparency.

@mpheath
Copy link
Collaborator Author

mpheath commented Aug 3, 2021

Updated for the Msgbox to be descriptive of operation, similar to the Classic. Execute Sandman.exe to remove sandboxies and the root location (waiting for /RemoveSandboxes argument to be added).

@isaak654
Copy link
Collaborator

isaak654 commented Aug 3, 2021

Updated for the Msgbox to be descriptive of operation, similar to the Classic. Execute Sandman.exe to remove sandboxies and the root location (waiting for /RemoveSandboxes argument to be added).

immagine

Legacy users could complain about the lack of a third option to clean only configuration files, please consider the checkboxes approach instead of Yes or No:

Show a Gui with checkboxes to select the options, as a user may want to remove the files, though keep the Sandbox folders.

@mpheath
Copy link
Collaborator Author

mpheath commented Aug 3, 2021

Perhaps better with a TaskDialogMsgBox?

Example was quickly done. Final text could be different.

brief.png

That would need 4 custom messages for the labels.

@isaak654
Copy link
Collaborator

isaak654 commented Aug 3, 2021

Perhaps better with a TaskDialogMsgBox?

Example was quickly done. Final text could be different.

It's interesting on the aesthetic side, but at the same time more straight without mentioning paths, configuration files names and a generic explanation for each option (about what is recommended for a specific use case).

Somehow this type of dialog attracts me more, because it leaves space for detailed labels:

github

I'm not sure if the additional description labels could be achieved with a TaskDialogMsgBox, but it seems very unlikely.

@mpheath
Copy link
Collaborator Author

mpheath commented Aug 4, 2021

The Git install is using a custom page for install. Just like the Sandboxie-Plus installer uses a custom page to select normal install or portable install. Uninstall is not custom page friendly.

TaskDialogMsgBox mock of Git selection with a few extra lines to show newlines in the detailed notes. It totally covers the uninstall progress window.

taskgit.png

Current PR code of calling the cleanup function is too late. Need to call before the Sbie driver is disabled so this will show immediately after the initial msgbox asking to uninstall. Once done with the TaskDialogMsgBox, the uninstall progress window will do the uninstall of the program.

Now, need titles and details so that a user could make a clear informed decision about keeping or removing configuration files, sandboxes and the root sandbox location. Who is keen enough to make the titles and details? else, I may have to do it.

@isaak654
Copy link
Collaborator

isaak654 commented Aug 4, 2021

Now, need titles and details so that a user could make a clear informed decision about keeping or removing configuration files, sandboxes and the root sandbox location. Who is keen enough to make the titles and details? else, I may have to do it.

Ok, if you take as reference your last picture:

Replace "Uninstall" with "Sandboxie-Plus Uninstall"
Replace "Adjusting your Path environment" with "Select Uninstall Type"
Replace "How would you like to use.." with "How would you like to uninstall Sandboxie-Plus?"
-> Keep configuration files and sandboxes
This is the most recommended option if you plan to reinstall Sandboxie while keeping your configuration files and sandboxes.
-> Remove configuration files
Select this option to remove the configuration settings in Sandboxie.ini and Sandboxie-Plus.ini while keeping the sandboxes unchanged.
-> Remove configuration files and sandboxes
Select this option to remove all configuration files and sandboxes, including the Sandbox folder located in custom paths with FileRootPath.

I think that the last label could be improved a bit, but any other improvement in other areas would be welcome.

EDIT: Sandboxie.ini could be easily moved from the Windows folder in C:\Program Files\Sandboxie-Plus, for example I prefer to keep it there and it still works. I don't know if your script includes this aspect, in case I could test it later.

@mpheath
Copy link
Collaborator Author

mpheath commented Aug 4, 2021

I have updated the code with changes.

This is the TaskDialogMsgBox:

tasksb.png

The main dialog title is hard coded by InnoSetup so remains as Uninstall.

Other labels were applied as suggested (Thanks isaak654).

Uses 5 Custom Messages.

EDIT: Sandboxie.ini could be easily moved from the Windows folder in C:\Program Files\Sandboxie-Plus, for example I prefer to keep it there and it still works. I don't know if your script includes this aspect, in case I could test it later.

I am not aware that Sandboxie.ini may be moved out of the Windows Directory. If so, the script can be updated.

@DavidXanatos
Copy link
Member

the driver first looks in the folder teh driver is in and than in teh windows folder for the sbie ini

@isaak654
Copy link
Collaborator

isaak654 commented Aug 4, 2021

I have updated the code with changes.

This is the TaskDialogMsgBox:

Other labels were applied as suggested (Thanks isaak654).

Just one note, it seems you missed the last revision of the updated strings (my previous post was updated a lot), so here it is:

last_strings_revision

I think these strings are more user-friendly to translate than the committed strings.

@mpheath
Copy link
Collaborator Author

mpheath commented Aug 4, 2021

@DavidXanatos
If that is for the portable install where the config files extract to {app}, then the uninstaller does not currently handle portable installs. I 'll add the {app} paths in for both inifiles.

@isaak654
Ah, later revisions. I copied them, tested...and did not notice the update.
OK, will look into it.

@isaak654
Copy link
Collaborator

isaak654 commented Aug 4, 2021

Ah, later revisions. I copied them, tested...and did not notice the update.
OK, will look into it.

In the meantime I have found a misleading sentence (my bad), in the second label I wrote Select this option to remove the configuration settings instead of configuration files, someone could understand that only the settings will be removed... so I'm going to release a commit once you've updated the strings to the last revision.

And I might have another suggestion... what do you think about the presence of a Cancel button in order to exit from the dialog and stopping the uninstall to continue?

@mpheath
Copy link
Collaborator Author

mpheath commented Aug 4, 2021

Cancel button, doubt if possble. TaskDialogMsgBox uses Msgbox constants and already using MB_ABORTRETRYIGNORE which is 3 buttons. No forth option to allow for a Cancel button.

files instead of settings done.

Inifile files in {app} done.

@isaak654

This comment has been minimized.

@mpheath mpheath marked this pull request as draft September 1, 2021 03:16
@mpheath
Copy link
Collaborator Author

mpheath commented Sep 1, 2021

Set this PR as draft. Not just /RemoveSandboxes is needed in Sandman. Translations are needed. Nothing less than full translation before commit. Nobody wants a non-english speaking person to lose data by failing to understand which button to select. This may take quite some time to do which is why I have now set it to draft.

@DavidXanatos
Copy link
Member

About people accidently deleting thair sandbox folder, I would go, if that is possible, for a solution like github has when doing irreversable changes to soemthing.

You show a question window to the user and the user must confirm the operation, not by clicking something but by typing in what he wants to be done. (in githubs case its only the repo name)

We would have a window that says: "If you want all your sandboxes to be removed, then enter 'delete all sandboxes' into the field below",
only once the user entered this correctly he can click on ok, else only no is available.

Now idk. if we can do something like this with innosetup, but I could add it in the sandman UI so that the uninstaller calls sandman to do the clean up and sandman shows this confirmation prompt and handles the deletion, if yes is confirmed.

what do you think?

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 6, 2021

@DavidXanatos It is more about language than confirmation. If you only know for example Turkish (which appears to be unmaintained) and get a message in English, that will not help. The Github handling is not the same concern as it is a confirmation in a language that you know.

If a language is supported for the install, and is not supported for the TaskMessageDialog during uninstall, then there exists a problem. The user would hopefully select the correct button.

There are a few languages not done for the Plus installer yet. The install is not high risk, but the uninstall could be a high risk with the TaskMessageDialog in a foreign language.

This is why translation is so important for the TaskMessageDialog and why I have stated the concern.

Nobody wants a non-english speaking person to lose data by failing to understand which button to select.

It is the understanding of the language being displayed to the user.

@DavidXanatos
Copy link
Member

but would someone who does nto understand the languate type the sentence 'delete all sandboxes' to the text field bellow in order to unlock the ok button?

@isaak654
Copy link
Collaborator

isaak654 commented Sep 6, 2021

but would someone who does nto understand the languate type the sentence 'delete all sandboxes' to the text field bellow in order to unlock the ok button?

Surely it seems a good way to prevent a possible accidental button click on the third uninstall option, but at that point it could be convenient to do the same for the second uninstall option. Losing all settings can also be a great loss as much as losing the settings + the sandboxes.

In this case, I would suggest short sentences with recurring words easier to find with online translators or just looking at the existing Classic translations:
Please type 'delete all configuration files' to remove all configuration files.
Please type 'delete all sandboxes' to remove the configuration files and the sandboxes.

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 7, 2021

I usually do a copy and paste for the Github confirmations. I just do not see the point of typing. It is probably a method to block the scripting of the interface like a captcha does.

4 tasks to complete:

  • TaskMessageBoxDialog
  • CustomMessages in English
  • Sandman with sandboxes removal code
  • Translation of CustomMessages

First 2 tasks done a month ago. Doing this in sequence is slow. Doing this in parallel would be quicker.

If the CustomMessages could be added to Languages.iss and be committed into master branch, then the translators can do their task of translating and submitting the PRs. These CustomMessages will not affect the current operation of the installer while being translated.

This could help with progress. At the moment, a month has passed without a sign of progress. Any languages that are not translated for the TaskMessageBoxDialog when it is committed, may need to be disabled in Languages.iss. Disabling languages may seem harsh, though it may be a necessity. This may prompt translators for the missing translations to do their task.

Perhaps the idea of doing tasks in sequence is not always a good idea and that doing tasks in parallel might be better for this feature to progress.

@isaak654
Copy link
Collaborator

isaak654 commented Sep 8, 2021

I usually do a copy and paste for the Github confirmations. I just do not see the point of typing. It is probably a method to block the scripting of the interface like a captcha does.

Probably the typing is best suited in this specific uninstall context than GitHub confirmations.

The italian translation for the UninstallTaskLabel(s) is ready, I just need to know the purpose of these underscores notations (&).. it's just a way to switch quickly from an option to another?

Also, do I need to update the localization here or just on the master branch?

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 8, 2021

There is no underscore _ in the english custom messages. You probably mean amperasand &.

The ampersand character & is for setting accelerator keys in GUIs. Accelerator keys are for keyboard interaction while a mouse like device will click the button displayed. Ampersand before a character defines that character as the accelerator key.

  • &Test = Alt + T
  • T&est = Alt + E
  • Te&st = Alt + S
  • Tes&t = Alt + T
  • &&Test = Literal string &Test

Doubled && is literal &.

The ampersand behavior is quite standard amongst GUI creation in many languages, even Sandman which uses QT.

The percentage sign % is a special escape character:

  • %n = line break

% followed by a integer is a parameter so %1 is replaced by the first argument. There are no arguments for this TaskMessageBoxDialog.

The percentage sign can be considered as special to Inno Setup.

Reference: CustomMessages

Also, do I need to update the localization here or just on the master branch?

I see that you have committed the custom messages to master. Good. Translating can start. :)

@isaak654
Copy link
Collaborator

isaak654 commented Sep 8, 2021

Yes, I meant the ampersands that on the GUI are displayed as underscores. They are quiet annoying because it's inconvenient to assign the same letter for two buttons, and I need to differentiate some term in the translation.

However, if you can confirm that the button click is triggered only with the ALT key, I could even tollerate them.

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 8, 2021

If you set accelerator keys, each button (or other control) should set a different character.

The english accelerator keys:

  • &Keep configuration files and sandboxes
    • Alt + K
  • Remove &configuration files
    • Alt + C
  • Remove configuration files and &sandboxes
    • Alt + S

These can be useful for some users who prefer to use keyboard instead of a mouse like device. A touchpad on a laptop can be harder to use than a ALT + key combination to select the button. Accelerator keys can help make it easier for a user.

The ALT + key is for & based accelerator keys. You could do like others have done, which is to use like (&S) to specify or perhaps even omit the & altogether if you consider it as not wanted your language.

Using the ALT key alone will display the accelerator keys. Once ALT is pressed, you may see the characters set as accelerator keys with an underline. Pressing ALT on it's own does no selection. Selection needs to be a ALT + key combination.

If no accelerators set, a keyboard user could possibly use TAB and SPACE for movement and selection. So other options exist for keyboard users.

Set them or don't set them. That is a choice.

@isaak654
Copy link
Collaborator

This could help with progress. At the moment, a month has passed without a sign of progress. Any languages that are not translated for the TaskMessageBoxDialog when it is committed, may need to be disabled in Languages.iss. Disabling languages may seem harsh, though it may be a necessity. This may prompt translators for the missing translations to do their task.

I would be inclined to provide an automatic translation for the current Plus languages with untranslated entries, while new Plus translations in other languages that won't include the Plus installer entries should not be merged at all.

I think that the Turkish translation needs to be excluded from the installer in Languages.iss and from the Plus UI too, because there is no significant activity since almost 7 months... this can be done for the Plus UI by removing/disabling this line:

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 19, 2021

I would be inclined to provide an automatic translation for the current Plus languages with untranslated entries, while new Plus translations in other languages that won't include the Plus installer entries should not be merged at all.

That is an option that could be attempted if translation by someone is not done. If translation can be done both ways and look reasonably the same, then that might be good enough to pass.

I think that the Turkish translation needs to be excluded from the installer in Languages.iss and from the Plus UI too, because there is no significant activity since almost 7 months... this can be done for the Plus UI by removing/disabling this line:

In my view, this decision should be risk based. If you remove the language, then you may lose any interest in updating by a another Turkish translator. If the risk of losing data is high, then I would advise to disable the language.


I will call out:

to update Languages.iss for the Sandboxie Plus installer. Your translations would help to progress this feature.

xorcan did not autocomplete so any Turkish translators who can do the task are welcome to translate.

@isaak654
Copy link
Collaborator

isaak654 commented Sep 19, 2021

In my view, this decision should be risk based. If you remove the language, then you may lose any interest in updating by a another Turkish translator. If the risk of losing data is high, then I would advise to disable the language.

There is a concrete risk of a translation mismatch even after 3 releases of continuos Plus UI changes, the same checkbox could be easily rearranged to do the opposite thing, but with the outdated description. Since the last Turkish update, more than 20 versions were released, so disabling the .ts file for Turkish language is a necessary thing to do at least for the Plus interface. The file sandman_tr.ts will continue to stay on the repository.

I'm planning to provide installer translations for both Spanish and Turkish, in this way the loss of interest from new potential translators would be greatly reduced.

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 19, 2021

I do not see a reason for the Plus installer to support Turkish if Sandman will not support Turkish. If Sandman will disable Turkish due to being high risk, also disable in Plus installer. Example: I would not like to install with an installer in english and find that the software is in a foreign language (not english).

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 27, 2021

@yfdyh000 translate to chinese simplified, to update Languages.iss for the Sandboxie Plus installer. Your translations would help to progress this feature.

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 29, 2021

Closed due to #1235 which is a revised Pull Request of this feature.

@mpheath mpheath deleted the innosetup_cleanup branch November 10, 2021 10:19
@isaak654 isaak654 removed the Work in progress Still in progress label Feb 24, 2024
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.

None yet

3 participants