-
-
Notifications
You must be signed in to change notification settings - Fork 233
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
Opening and closing another copy stops the ongoing process #136
Comments
Yeah; it would do that. The author's idea of a graceful shutdown on close is to use As an added bonus; the program doesn't even bother to use the unique process ID. It filters based on image name, which means any and all open instances of This piece of software is - quite evidently - not ready for general use. |
well this can be changed easy i guess |
Not really. The program currently doesn't call If you're looking to narrow the target for the taskkill operation; there's no way to know what the process ID of the corresponding Really; the entire guts of this thing should be axed and it should be rewritten to use |
@rjgotten us design skills seem to be on pair to mine, but i just see that it have to be rewritten a bit. Telling that it should be axed dont motivate the developer, he might miss knowledge and use the cmd path to get the verbose messegnes. he could rewrite and kill the gui insted the the process and close the cmd everytime when he have done analyze/compress individual and prevent the user to launch multiple insances of CompactGui. That seems lesser work to me from his POV. |
@rjgotten While inflammatory, you do have valid concerns. There's really no need to be using cmd.exe anymore, but it's been left behind because I haven't got around to code cleanup in a while (I've been really busy, so changes and fixes at the moment have been in spare time)
Now as you mentioned (and I am well aware of), calling a process through cmd causes you to lose sight of the processID, so you can't target just that one.
Now if you have a look at the code that calls that function, you'll see if checks to make sure the current UI isn't already running an action. If it is running, then it throws a warning that you have to commit to before it kills the compression. I chose this route at the time I was working on localisation because it would limit the amount of core changes that were required, so it wouldn't introduce new bugs. If isActive = 1 Then
If MessageBox.Show _
("Are you sure you want to exit?" & vbCrLf & vbCrLf & "Quitting while the Compact function is running is potentially dangerous." _
& "Continuing to close could lead to one of your files becoming stuck in a semi-compressed state." _
& vbCrLf & vbCrLf &
"If you do decide to force quit now, you can potentially fix any unreadable files by running Compact again," _
& "selecting the 'Force Compression' Checkbox and then running uncompress on the folder." & vbCrLf & "Click Yes to continue exiting the program.",
"Warning!", MessageBoxButtons.YesNo, MessageBoxIcon.Exclamation, MessageBoxDefaultButton.Button2) <> DialogResult.Yes Then
e.Cancel = True
Else
Try
LetsKillStuff()
Catch ex As Exception
End Try
End If So if something is already running, it's not just going to go ahead and kill it, which I thought was a reasonable way around the issue - someone wants to cancel it, warn them that cancelling isn't going to be clean, and then give them the option. I even called the function "LetsKillStuff()" to highlight that it's not a great idea. Works in most situations? Yes. Good code? No. Which brings us back to the matter at hand - multiple windows open. Honestly, it was supposed to be single-instanced, and a while back I must have toggled it back by accident. So that would be easy to 'fix' - just stop users from opening more than one instance, problem solved. Or I could add another capture to the kill function to check if the number of compactgui processes is >1. For now, I'll just switch it back to single instancing, a bandaid fix until I have more free time to sort this out properly. Never fear, I don't plan to use cmd in the long run, as it's a bloody stupid way of going about it. Just have a look at this entire section It will make you cry how inefficient it is. |
like i told. im glad that i get no rebackup. |
@TjReiny @rjgotten do you want to try this version? It should be much safer, and multi-instancing should work fine since it's calling and quitting its own process now - it also won't force kill on exit, but rather wait for the current file (not folder) to finish processing and then close. (The source is under the development branch) @Noneneeded2 if you want to give this a go too, that would be great :)
|
thanks! |
@Noneneeded2 it might be better if you use the one from the downloads page instead :) |
Hey, great app
The issue is when I'm compressing a huge folder, then open another copy of the app to compress some other folder, when the second compressing is done and I close the app, the first huge folder process stops, doesn't use any hard drive time/CPU.
Thanks for your efforts
The text was updated successfully, but these errors were encountered: