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

ComboBox cursonDownKey nullref fix #1472

Merged
merged 4 commits into from
Sep 29, 2021
Merged

Conversation

En3Tho
Copy link
Contributor

@En3Tho En3Tho commented Sep 28, 2021

A small fix for #1471
Added null guard to fix null ref when pressing keyDown inside combobox
Improved an error message when view cannot be found

Improved an error message when view cannot be found
@En3Tho
Copy link
Contributor Author

En3Tho commented Sep 28, 2021

@tznind @BDisp Is this suitable fix for the problem or should it be done another way? Is ComboBox supposed to show other values when cursorDown key is pressed?

@tznind
Copy link
Collaborator

tznind commented Sep 29, 2021

Thanks! @tig handles PR review but this looks fine to me. Are you able to add a unit test?

@En3Tho
Copy link
Contributor Author

En3Tho commented Sep 29, 2021

Okay, So I've found a few bugs with a unit test. Please look at them. Hontesly can we just make search set not null all the time? Is one allocation worth it?

One of the bugs was: another nullref with searchset
Another: wheh ComboBox has both initial value and source, when added to the Top, method ShowList() was called with a searchset value which was ... null.

I fixed this with another null check but I feel like all these null checks are completely not worth it and only litter the code. Considering LINQ usage in this repo I think that allocations are not really a concern... Considering the amount of subtle bugs I believe .Net applications should be as null-free as possible with a few "fast-speed-low-level" exceptions.

@BDisp
Copy link
Collaborator

BDisp commented Sep 29, 2021

I think is better avoiding null list. It must be an empty list on these cases.

@BDisp
Copy link
Collaborator

BDisp commented Sep 29, 2021

Another solution to make possible to insert ComboBox on a smaller superview is to add the ListView to the superview's Superview, instead added to the ComboBox.

@tznind
Copy link
Collaborator

tznind commented Sep 29, 2021

I agree with both of you, empty list is good. Lets solve the simple null reference issue first then look at a different PR for bigger changes?

@BDisp
Copy link
Collaborator

BDisp commented Sep 29, 2021

@En3Tho don't forget that the searchset is a new source based on filtering what is tipping. If TextField is empty then all the source elements are shown.

@En3Tho
Copy link
Contributor Author

En3Tho commented Sep 29, 2021

Yeah, but if you set ComboBox "Text" (or value) initially, then it is not actually empty and this behaviour skips searchset initialization resulting in a null ref

Copy link
Collaborator

@tig tig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@tig tig merged commit f602d3b into gui-cs:main Sep 29, 2021
BDisp pushed a commit to BDisp/Terminal.Gui that referenced this pull request Oct 2, 2021
* added null guard to fix null ref when pressing keyDown inside combobox
Improved an error message when view cannot be found

* Added a unit test to ensure combobox can process all key events
Found and fixed a new nullref

* Found a new bug when source is already present and combobox is added to a top view

* searchSet is auto initialized to new List() now to make the code a little bit safer
BDisp pushed a commit to BDisp/Terminal.Gui that referenced this pull request Oct 2, 2021
* added null guard to fix null ref when pressing keyDown inside combobox
Improved an error message when view cannot be found

* Added a unit test to ensure combobox can process all key events
Found and fixed a new nullref

* Found a new bug when source is already present and combobox is added to a top view

* searchSet is auto initialized to new List() now to make the code a little bit safer
tig pushed a commit that referenced this pull request Oct 5, 2021
…1447)

* Fixes #1446. Added more features to the Border and Toplevel focus.

* Prevents throwing exception on negative effect3DOffset values

* Ensures that a view can be focused.

* Only sets focus if it isn't disposing.

* Fixes ViewToScreen and DrawChildBorder Effect3D.

* Unit test for negative coordinates with the ViewToScreen method.

* Added Tab navigation feature to the Editor scenario.

* ComboBox cursonDownKey nullref fix (#1472)

* added null guard to fix null ref when pressing keyDown inside combobox
Improved an error message when view cannot be found

* Added a unit test to ensure combobox can process all key events
Found and fixed a new nullref

* Found a new bug when source is already present and combobox is added to a top view

* searchSet is auto initialized to new List() now to make the code a little bit safer

* Fixes WindowsDriver HeightAsBuffer set to false. (#1466)

* Bump ReportGenerator from 4.8.12 to 4.8.13 (#1473)

Bumps [ReportGenerator](https://github.com/danielpalme/ReportGenerator) from 4.8.12 to 4.8.13.
- [Release notes](https://github.com/danielpalme/ReportGenerator/releases)
- [Commits](danielpalme/ReportGenerator@v4.8.12...v4.8.13)

---
updated-dependencies:
- dependency-name: ReportGenerator
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fixes #1445. Fixing more the Curses and WSL clipboard. (#1448)

* Fixes #1445. Fixing more the Curses and WSL clipboard.

* Fixing unit tests.

* Changing namespace.

* Fixes WSL2 clipboard unit test.

* Upgrades devcontainer with the MainLoop fix.

* Fixes pasting with no selection and with lines break.

* Prevents the event button click being fired after a button pressed with mouse move.

* Fixes the char [ not being processed.

* Added Application.QuitKey property to allow change the quitting application key. (#1450)

* Added Application.QuitKey property to allow change the quitting application key.

* Fixes QuitKey unit test by reseting his value.

* Locks timeouts until is added.

* Fixes #1467. AlternateForward/BackwardKey bypasses dialog modality (#1468)

* Changed namespace.

* Fixing merge conflicts.

* Fixes mouse click issue.

* Removing windows resizing because buffer resizing is enough.

* Fixes #1477. Mouse click and console bottom on Windows Terminal.

Co-authored-by: Igor Bagdamyan <37334640+En3Tho@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
BDisp added a commit to BDisp/Terminal.Gui that referenced this pull request Oct 5, 2021
…us. (gui-cs#1447)

* Fixes gui-cs#1446. Added more features to the Border and Toplevel focus.

* Prevents throwing exception on negative effect3DOffset values

* Ensures that a view can be focused.

* Only sets focus if it isn't disposing.

* Fixes ViewToScreen and DrawChildBorder Effect3D.

* Unit test for negative coordinates with the ViewToScreen method.

* Added Tab navigation feature to the Editor scenario.

* ComboBox cursonDownKey nullref fix (gui-cs#1472)

* added null guard to fix null ref when pressing keyDown inside combobox
Improved an error message when view cannot be found

* Added a unit test to ensure combobox can process all key events
Found and fixed a new nullref

* Found a new bug when source is already present and combobox is added to a top view

* searchSet is auto initialized to new List() now to make the code a little bit safer

* Fixes WindowsDriver HeightAsBuffer set to false. (gui-cs#1466)

* Bump ReportGenerator from 4.8.12 to 4.8.13 (gui-cs#1473)

Bumps [ReportGenerator](https://github.com/danielpalme/ReportGenerator) from 4.8.12 to 4.8.13.
- [Release notes](https://github.com/danielpalme/ReportGenerator/releases)
- [Commits](danielpalme/ReportGenerator@v4.8.12...v4.8.13)

---
updated-dependencies:
- dependency-name: ReportGenerator
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Fixes gui-cs#1445. Fixing more the Curses and WSL clipboard. (gui-cs#1448)

* Fixes gui-cs#1445. Fixing more the Curses and WSL clipboard.

* Fixing unit tests.

* Changing namespace.

* Fixes WSL2 clipboard unit test.

* Upgrades devcontainer with the MainLoop fix.

* Fixes pasting with no selection and with lines break.

* Prevents the event button click being fired after a button pressed with mouse move.

* Fixes the char [ not being processed.

* Added Application.QuitKey property to allow change the quitting application key. (gui-cs#1450)

* Added Application.QuitKey property to allow change the quitting application key.

* Fixes QuitKey unit test by reseting his value.

* Locks timeouts until is added.

* Fixes gui-cs#1467. AlternateForward/BackwardKey bypasses dialog modality (gui-cs#1468)

* Changed namespace.

* Fixing merge conflicts.

* Fixes mouse click issue.

* Removing windows resizing because buffer resizing is enough.

* Fixes gui-cs#1477. Mouse click and console bottom on Windows Terminal.

Co-authored-by: Igor Bagdamyan <37334640+En3Tho@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
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.

None yet

4 participants