search

AdamsLair / duality

a 2D Game Development Framework
https://adamslair.github.io/duality
MIT License
1.4k stars 289 forks source link

Prevents null exception when selecting component type #751

Closed foreverWIP closed 4 years ago

foreverWIP commented 4 years ago

Addresses issue #749

Disables the OK button if no text is present in the filter. Additionally, disables the OK button if no node is currently selected from the list.

foreverWIP commented 4 years ago

Taking a look at my changes, it seems the property I set up (ShouldEnableOkButton) doesn't just rely on text length, it also returns true when a node is selected (i.e. itemListing.SelectedNode != null). However, not only is the text length check unnecessary (since the filter finding a match will select that node in the list and enable the Ok button anyway) but it can also lead to a scenario where the Ok button can be enabled from a single character doesn't match the first letter of any node in the list. The current commit fixes this up.

foreverWIP commented 4 years ago

The changes in the designer code came from me testing which events best kept the Ok button properly updated. The Designer.cs should be properly rolled back now. As for the initial disabling of the Ok button, I've moved it out of the type checking so it now works on all list dialogs. I'm still uncertain about whether I could format my changes better, but functionally it's ready to go.

ilexp commented 4 years ago

I'm still uncertain about whether I could format my changes better, but functionally it's ready to go.

It's easier to show and tell here, so I've pushed directly to your PR branch, check out the "Style Tweaks" commit. Looked good overall, just two changes:

Granted, the class you were working on wasn't sticking to our codebase rules 100%, so it was harder in this case to just "blend in". You can find a full overview on the guidelines here.

Another thing I changed as I tested was where the button state is updated: Since users can navigate with keyboard input only, there may not be a click event at all, but cursor key presses. Instead, I've moved the state update to the SelectionChanged event to cover all selection changes, regardless of their source.

What I also notices just now is that you were adding your changes directly to master. It's not an issue for the Pull Request, but I strongly recommend to always do PRs with a separate feature/xy branch. Otherwise, rejecting or squash-merging PRs may cause problems in your own fork of the project, and it doesn't allow you to update your fork to the latest master version without also having your PR changes around. It also makes it impossible to have more than one PR at a time!

Back to topic though: Merging this 👍

ilexp commented 4 years ago

Merged, and triggered a binary release. Your fix will be in editor version 3.1.7, should be out there in about 20 minutes. Thanks!