search

kvakulo / Switcheroo

The humble incremental-search task switcher for Windows
www.switcheroo.io
GNU General Public License v3.0
790 stars 122 forks source link

Improved window closing #25

Closed HellBrick closed 9 years ago

HellBrick commented 9 years ago

Check this out, I've implemented a prototype of #23. I played with it for a while and I don't think extra highlighting of the windows that can't be closed is even needed.

kvakulo commented 9 years ago

Awesome! Thanks! :)

I'll take a closer look one of the next days.

/Regin

On Saturday, January 17, 2015, HellBrick notifications@github.com wrote:

Check this out, I've implemented a prototype of #23 https://github.com/kvakulo/Switcheroo/issues/23. I played with it for a while and I don't think extra highlighting of the windows that can't be

closed is even needed.

You can view, comment on, or merge this pull request online at:

https://github.com/kvakulo/Switcheroo/pull/25 Commit Summary

  • Added SystemWindow.IsClosed()
  • The focus is no longer switched to the closed window
  • Binding to ObservableCollection<> instead of List<> to allow removing items
  • Added a proper AppWindowViewModel
  • The window is marked on closing
  • Migrated WindowFilterer to AppWindowViewModel to preserve IsBeingClosed state
  • Fixed animation glitch
  • Implemented primitive WindowCloser
  • WindowCloser waits until the end of time (not really, just until the main windows is hidden)
  • Fixed the switch functionality (oops)
  • Minor clean-up

File Changes

Patch Links:

— Reply to this email directly or view it on GitHub https://github.com/kvakulo/Switcheroo/pull/25.

HellBrick commented 9 years ago

Apparently there is some weird bug in this implementation that typically manifests after a long period of idle: Switcheroo doesn't recognize the windows as closed and they stay grayed out. After the main window is re-opened everything works fine again. I'm not sure why it happens yet, probably need to add some logs to track this down.

kvakulo commented 9 years ago

This feature is simply amazing! It gives a much better experience when closing windows -- and I really like the animation when the window is removed.

I experienced the weird bug that you describe above a few times. I'm also not sure in which cases it happens, but hopefully it won't be too difficult to track down :)

I have just a few comments on the code:

Keep up the good work @HellBrick! :+1:

HellBrick commented 9 years ago

Glad you liked it ;)

AppWindowViewModel was originally planned to be in Switcheroo project, but I moved it to Core in c4d0dac1e5734bf5e8c3fc05d4f16552c9b17f54. The IsBeingClosed property is stored in the view model, so re-creating a view model after applying the filter would lead to this flag being lost. There were two possible solutions to this problem. First was to add this flag to AppWindow, which didn't feel right, because the fact that the user tried to close the window once is not really a part of the state of the window itself. So I went with another one: preserving the instance of the view model through the filtering routine. Theoretically, it's possible to do this without migrating WindowFilterer to using AppWindowViewModel directly by storing a dictionary of view models in the view, but it seemed like an unnecessary over-complication. After all, the Core project has some view-related code anyway: it references System.Xaml; XamlHighlighter looks totally view-specific; AppWindow.FormattedTitle and AppWindow.FormattedProcessTitle might be better off being view model exclusive properties (actually, now that I look at the code of this branch, these properties aren't really needed in AppWindow).

However, I just got an alternative idea on how to move view model back to Switcheroo project. FilterResult and WindowFilterer.Filter() can be made generic with the type argument constrained to an interface that exposes WindowTitle and ProcessTitle properties. This may be a better solution than keeping the AppWindow -> AppWindowViewModel map in the main window. How do you feel about this solution?

On your second question: yeah, sure it can. I brought Caliburn just for its NotifyOfPropertyChange() method that takes an expression instead of a string. C# 6 has the awesome nameof() feature that should make this method obsolete, but for the time being I think NotifyOfPropertyChange() implementation can simply be copied from Caliburn's source code.

kvakulo commented 9 years ago

Haha - you are totally right about XamlHighlighter, AppWindow.FormattedTitle and AppWindow.FormattedProcessTitle. They certainly should not be Core either :)

I like the solution that you describe above - it seems to be the right way to go about it :+1:

(If AppWindow.FormattedTitle and AppWindow.FormattedProcessTitle can be removed from AppWindow it's great - nice to have a view model now!)

/Regin

HellBrick commented 9 years ago

I've cleaned up the stuff we discussed. There's still the bug to hunt down, but I probably won't have the time to deal with it until the weekend.

HellBrick commented 9 years ago

Good news! I've finally tracked down and fixed the bug. It turned out to be a very silly issue that I completely overlooked when searching for something more intricate.

kvakulo commented 9 years ago

It's looking great! And nice work on tracking down that bug!

Thanks @HellBrick!

kvakulo commented 9 years ago

I'll include this change in the next release of Switcheroo, but if anybody wants to try it out before then, there's a build with the feature available here: http://teamcity.codebetter.com/viewLog.html?buildId=180068&tab=artifacts&buildTypeId=Switcheroo

jaredbidlow commented 9 years ago

I'm using Switcheroo 0.9.1.98. I followed this issue thread to here. I thought the "leave the Switcheroo overlay open after closing a window" issue would be fixed, but it isn't. The issue is that when I search for a program in Switcheroo intending to close all of the windows for that program, any prompt by the program when closing will reset Switcheroo to the unfiltered state forcing me to search again.

HellBrick commented 9 years ago

I just tried it with Visual Studio on 0.9.1.98 and it works as intended: http://i.imgur.com/aJPemOm.png

I think more info on how to reproduce the issue may be helpful, probably in a separate issue (I guess https://github.com/kvakulo/Switcheroo/issues/23 should be closed by now, since the feature that was requested in it was implemented by this PR).

kvakulo commented 9 years ago

@jaredbidlow Does it happen when you try to close a window of a specific program?

@HellBrick #23 has been closed now :)

jaredbidlow commented 9 years ago

@kvakulo Excel2010. I don't know if what I want is what you have shown, @HellBrick. The Swicheroo window does stay open if window requires user attention to close until I go and attend to the prompt and close the window.

kvakulo commented 9 years ago

Ok, that's the way it's working currently: whenever the Switcheroo overlay is reopened the filtering is cleared.

There's work in progress on closing several windows at the same time (#48) - that might help in above scenario. Otherwise feel free to raise a new issue and describe the functionality you would like to have added to Switcheroo.

/ Regin