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

Add keybinding for configurable new window #216

Merged
merged 3 commits into from Feb 28, 2019

Conversation

Projects
None yet
2 participants
@ericcornelissen
Copy link
Contributor

ericcornelissen commented Feb 27, 2019

As per #96 and this comment I added a keybinding to open a new configurable window. The default binding is Ctrl+Shift+N.

preview


Alternative design

1] It may be possible to rely less on a parameter (showProfileSelection) in App.xaml.cs, but I personally don't think that's a good idea.

2] There are now two EventArgs (namely CancelableEventArgs and ProfileSelectEventArgs) that basically only hold a boolean value (cancelled and ShowProfileSelection respectively). On the one hand one the naming helps the reader understand the code, on the other hand it you have (albeit simple) duplicate code. I can refactor the EventArgs into a single BooleanEventArgs class like so:

// Current use of CancelableEventArgs
e.Cancelled = true;

// Example of using BooleanEventArgs
cancelable.Value = true;

However, the current approach forces the programmer to be descriptive, whereas the refactor would allow misuse of the EventArgs, like so:

// Misuse of BooleanEventArgs
e.Value = true;
@felixse

This comment has been minimized.

Copy link
Owner

felixse commented Feb 28, 2019

great job 👍

I think a little duplication here is fine, it makes the code more clear this way. I also renamed the new EventArgs to describe the context in which it is used.

@felixse felixse merged commit c4fcd98 into felixse:master Feb 28, 2019

@ericcornelissen

This comment has been minimized.

Copy link
Contributor Author

ericcornelissen commented Feb 28, 2019

I think a little duplication here is fine, it makes the code more clear this way

Agreed

I also renamed the new EventArgs to describe the context in which it is used.

I wasn't sure about the naming anyway, but this is much better 👌

@ericcornelissen ericcornelissen deleted the ericcornelissen:keybind/configurable-new-window branch Feb 28, 2019

@felixse felixse referenced this pull request Mar 10, 2019

Merged

Dev/renderer neutral #220

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.