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

Improve progress and status messages #1168

Merged
merged 1 commit into from Sep 12, 2021
Merged

Conversation

mpheath
Copy link
Collaborator

@mpheath mpheath commented Sep 5, 2021

An improvement with progress messages for the Plus installer. The current uninstall is not very informative. The KmdUtil scandll and KmdUtil stop SbieDrv can each take several seconds. Looking at Uninstalling Sandboxie-Plus... the whole time is not progress, as it is unknown what it is doing with KmdUtil in the code section.

The uninstall progress messages are now informative and if something freezes for some reason, like it is paused on KmdUtil scandll for an hour, then you can know what is causing it by the progress message. Not that I expect that scenerio, though unexpected things can happen.

The upgrade shows a custom progress page that looks like the Prepare To Install page as I cannot do status messages on the Ready page about stopping processes, the service and the driver. Command line messages are used so no translation should be needed.

Summary:

  • Better progress messages for install, upgrade and uninstall.
  • Two empty lines between sections to improve readability.
  • Removed old commented sections and entries.
  • Code cleanup:
    • Moved some file deletes, registry entries and KmdUtil calls into the UninstallDelete, Registry and Run sections.
    • All keywords lowercase except for True and False.
    • Variable, function and procedure names as CamelCase.
    • Comments as Sentence case.
    • Moved any comments into definitions that were above the definitions.
    • Functions and procedures ordered with events nearest to bottom and others above.
    • Two empty lines between functions and procedures to improve readability.
    • Removed unneeded NextButtonClick2() and moved code into NextButtonClick().
    • Removed unneeded CurStepChanged() and InitializeUninstall().
    • Removed old commented code.
    • Removed unused variable names.

This is the alternative script tested for the past month ... without the UninstallCleanup() procedure.

Commit may put PR #1058 into conflict. That can be resolved ... or if needed, closed and post a new PR for the UninstallCleanup().

Would like to discuss any concerns before a commit.

@mpheath mpheath marked this pull request as ready for review September 5, 2021 06:23
@isaak654
Copy link
Collaborator

isaak654 commented Sep 5, 2021

Personally I would suggest to merge this PR soon after 0.9.6, resolve the conflicts in PR #1058 and split both UninstallTaskLabel5 and /RemoveSandboxes parts in a new PR with David as assignee.

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 6, 2021

Personally I would suggest to merge this PR soon after 0.9.6,

Seems reasonable to commit at start of a new version development cycle. I do not want to shock anyone which is why I posted some links earlier to notify of the incoming change. Much of the proven to work code is still intact. It is the changing of case and the reorder of functions which may make the diff look like is something totally different. Any new code changes or other changes has been tested many times.

resolve the conflicts in PR 1058

Just noticed it has already gone into conflict over Installer/Languages.iss.

PR 1058 can be updated later based on the commit of this PR. This will resolve conflicts.

split both UninstallTaskLabel5 and /RemoveSandboxes parts in a new PR

Doubt if a split will help with progress, unless you imply that PR 1058 should try being like Classic with just remove of configuration files. And perhaps later, add the remove of sandboxes if still considered as worthwhile. Is this what your suggestion is about? Otherwise, I thought it was settled.

@DavidXanatos
Copy link
Member

I pan to repease 0.9.6 this weekend, do you think i can merge this pull request and use the new script already?

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 6, 2021

I consider it ready. I can let you know if the merge looks good with testing the script to undoubtly ensure it is good.

I just cloned this repo and pulled the PR, done the merge and Sandboxie-Plus.iss compares the same to my local copy that has been tested. So, it should be good.

@isaak654
Copy link
Collaborator

isaak654 commented Sep 6, 2021

Doubt if a split will help with progress, unless you imply that PR 1058 should try being like Classic with just remove of configuration files. And perhaps later, add the remove of sandboxes if still considered as worthwhile. Is this what your suggestion is about?

There is no ETA about the new /RemoveSandboxes switch, so it could be reasonable to delay it by splitting that part into a new smaller PR and merging the rest first (with or without translations).

@mpheath
Copy link
Collaborator Author

mpheath commented Sep 6, 2021

I will change the uninstall in PR 1058 to omit /RemoveSandboxes later.

@isaak654
Copy link
Collaborator

Build 0.9.6 is out, are you ready to merge this?

I've just rerun the CIs that belong to your pull requests and it appears a WDK version mismatch.
I think it's the same for all pull requests in this section created before this commit, so I wouldn't be worry about it.

@mpheath mpheath merged commit 7998570 into sandboxie-plus:master Sep 12, 2021
@mpheath mpheath deleted the status branch September 20, 2021 23:14
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