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

* Added CloseProcesses, a multiple window closer. #48

Closed
wants to merge 8 commits into from

Conversation

cutecycle
Copy link

Tried my hand at adding a "Close all processes appearing in the filter" function [Ctrl+Q]. New to the code, so:

  • I'm currently not sure how to make this empty the list after the windows have been closed
  • Haven't added the shortcut name to the help text

@HellBrick
Copy link
Contributor

I like the feature, seems useful for closing those infinitely multiplying Explorer windows ;)

I have one concern about the implementation though: it seems if you're trying to close a bunch of windows and one of them prevents itself from being closed (imagine the classic 'wanna save changes?' dialog), the rest of them won't even attempt to be closed. I would try something like this:

HellBrick@7c4b757
HellBrick@17cb452

(I'm not sure it actually works, just bouncing an idea around =))

@kvakulo
Copy link
Owner

kvakulo commented Jul 1, 2015

Very cool! Thanks for your contribution @cutecycle!

I'll take a closer look at it shortly - just some quick thoughts:

  1. I'm thinking that the shortcut could be CTRL + SHIFT + W to make it similar to the current close a single window shortcut
  2. The feature can be potential destructive, if one accidentally presses the combination when for instance the filter is empty. Maybe a confirmation box should be shown when using this shortcut? Something like: "Are you sure you want to close all 30 windows? [ ] Don't show this message again"

/ Regin

@cutecycle
Copy link
Author

@HellBrick i was definitely worried about those! I'll merge to check it out later today.
Thanks @kvakulo! Maybe a warning if the filter contains more than one process, or more than ten windows of the same process? And an underlined-W for a "Quit Anyway"?

@HellBrick
Copy link
Contributor

I definitely like the idea of a warning if the filtered windows belong to more than 1 process. And this also automatically handles the empty filter case. Not sure whether there's a need for a warning if too many windows of the same process are closed, but if the warning comes with a 'Don't ask again' checkbox, then I see no harm in having it.

@kvakulo
Copy link
Owner

kvakulo commented Jul 1, 2015

+1 to showing the warning only if there are windows from more than one process.

Let's skip the warning if there are only windows from one process no matter how many.

And let's skip the 'Don't ask again' checkbox too.

/ Regin

@cutecycle
Copy link
Author

Admittedly, I'm out of my element in VS/C#! I'm not quite sure how to do a proper stack trace but with the RemoveWindow I get an InvalidOperationException with CloseProcesses even with the new changes; I'm guessing it's related to modifying the container of window references? Like, RemoveWindow is trying to remove from an empty _unfilteredWindowList?

…s to avoid modifying a collection that's being iterated on
@HellBrick
Copy link
Contributor

There's a problem with modifying _filteredWindowList when iterating through it, it should be copied before closing stuff: HellBrick@19c3b54

@kvakulo kvakulo mentioned this pull request Jul 9, 2015
@cutecycle
Copy link
Author

I tried to add help text for the close all windows command; however it expands the width of the window but leaves the width of the window list the same.

@kvakulo
Copy link
Owner

kvakulo commented Jul 13, 2015

@cutecycle Just skip the help text for now, I'll try to figure out how to add more space, or maybe an additional window with documentation for the advanced stuff :)

Do you want to work further on this feature, or should I merge it? Then I'll prepare to include it with the next release.

/ Regin

@cutecycle
Copy link
Author

I wanted to make one more commit with a warning MessageBox for closing over ten windows, though I'm not at my machine at the moment. The process name check would be a bit more complicated so I'll leave it for now.

@HellBrick
Copy link
Contributor

It's not that complicated =) HellBrick@682422e. You may also want to revert d6c4ca2, since it intentionally disabled things.

P.S. The code formatting became really horrible now that there were 3 different instances of Visual Studio used to edit the file ;) Do any of you guys know a way to distribute formatting settings with the repository to avoid this problem with external contributions?

@cutecycle
Copy link
Author

Awesome! Yeah, at one point I was using 2015 and gvim as well as VsVim which uses a different indentation format within VS, and eventually turned on invisibles and was just like... whoops, this is a mess.

The convention project wide is 8 spaces for one tab, then?

@HellBrick
Copy link
Contributor

As far as I can see, @kvakulo's original code uses 4 spaces as indent.

@kvakulo
Copy link
Owner

kvakulo commented Mar 13, 2018

I'll try to finish this in #90

@kvakulo kvakulo closed this Mar 13, 2018
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.

4 participants