-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Changes to fix issues in DemoConsole
#9419
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
Changes from all commits
9e87828
54577c1
974cc44
aa4750f
795876d
901eb7b
2999908
88b2dbe
8c949a9
a972eee
92c3e73
007f35f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -117,6 +117,10 @@ public void Insert(int index, Glyph value) | |
| /// </summary> | ||
| public void Remove(Glyph value) | ||
| { | ||
| List.Remove(value); | ||
| // Its possible that the value doesn't exist in the list in case of an `Undo` operation | ||
| if (List.Contains(value)) | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was because remove was throwing exceptions. But the exceptions were caught further up. Up to the team if they are happy with the exception.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elachlan, Did you find a scenario where we are trying to call remove for a glyph that wasn't added to collection? This change should be good.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, it would have been when deleting and then performing an "undo" (I think). Do you want a debug.assert for when the value isn't in the list?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If it is a valid scenario, i would put a comment about it. Debug assert will be annoying for a valid scenario.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added comment. |
||
| { | ||
| List.Remove(value); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,11 @@ public Entry(object item) | |
| } | ||
|
|
||
| public object Item { get; set; } | ||
|
|
||
| public override string? ToString() | ||
| { | ||
| return Item.ToString(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ This can NRE
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should it be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. wait,
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This has nullable types enabled and object is marked as non-nullable. So it shouldn't NRE?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we know for sure |
||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you enable nullability on this file or is it a big change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a big change I think. I'd rather have it done in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@halgab / @gpetrou did either of you want to tackle null annotating
ResourceCodeDomSerializer.SerializationResourceManager.cs?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was waiting for #9099 to be merged to tackle
CodeDomSerializerand then derived classes, but I can take care of this one file after this PR merges for sureThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is: #9457