Skip to content

* Added CloseProcesses, a multiple window closer.#48

Closed
cutecycle wants to merge 8 commits into
kvakulo:masterfrom
cutecycle:master
Closed

* Added CloseProcesses, a multiple window closer.#48
cutecycle wants to merge 8 commits into
kvakulo:masterfrom
cutecycle:master

Conversation

@cutecycle

Copy link
Copy Markdown

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
Copy Markdown
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

kvakulo commented Jul 1, 2015

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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

kvakulo commented Jul 1, 2015

Copy link
Copy Markdown
Owner

+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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

kvakulo commented Jul 13, 2015

Copy link
Copy Markdown
Owner

@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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

@kvakulo

kvakulo commented Mar 13, 2018

Copy link
Copy Markdown
Owner

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