Conversation
Add a ReturnObject class to return a ‘handled’ value, which the Publisher needs to know. All of the Subscribers only handle a specific className or className and Id combination so it did not matter that two of the classes are derived from DlgListenerBase, which has ColleaguePriority.High.
| } | ||
| // Only handle "LexEntry" class. | ||
| string className = XmlUtils.GetOptionalAttributeValue(command.Parameters[0], "className"); | ||
| if (className == null || className != "LexEntry") |
There was a problem hiding this comment.
While you're in the area, className == null is redundant to className != "LexEntry"
| return; | ||
| } | ||
| // Only handle "FsClosedFeature" for "CmdInsertPhonologicalClosedFeature". | ||
| if (className == "FsClosedFeature" && command.Id != "CmdInsertPhonologicalClosedFeature") |
There was a problem hiding this comment.
we just checked className == "FsClosedFeature".
If you want to clean up, you could combine these two if statements.
| if (!(obj is ReturnObject retObj) || | ||
| !(retObj.Data is Command command)) | ||
| { | ||
| Debug.Assert(false, "Received unexpected object type."); |
There was a problem hiding this comment.
nit: could use Debug.Fail or even a yellow-box crash
There was a problem hiding this comment.
The old code had an Assert to check Command, I think leaving it as an Assert but also checking ReturnObject is sufficient. It doesn't seem like it needs to change.
| } | ||
|
|
||
| public object Data { get; set; } | ||
| public bool ReturnValue { get; set; } |
There was a problem hiding this comment.
Would Handled be a better name?
There was a problem hiding this comment.
In this case I agree, Handled is a better name, but my intent is for this to be generic so the next time it is used it also makes sense, which might be something other than Handled.
There was a problem hiding this comment.
I'm pretty sure Handled is the only case, but ReturnValue is acceptable.
papeh
left a comment
papeh
left a comment
There was a problem hiding this comment.
@papeh reviewed 11 files and all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on mark-sil).
mark-sil
left a comment
mark-sil
left a comment
There was a problem hiding this comment.
@mark-sil made 4 comments.
Reviewable status: 6 of 11 files reviewed, 4 unresolved discussions (waiting on papeh).
| } | ||
|
|
||
| public object Data { get; set; } | ||
| public bool ReturnValue { get; set; } |
There was a problem hiding this comment.
In this case I agree, Handled is a better name, but my intent is for this to be generic so the next time it is used it also makes sense, which might be something other than Handled.
| if (!(obj is ReturnObject retObj) || | ||
| !(retObj.Data is Command command)) | ||
| { | ||
| Debug.Assert(false, "Received unexpected object type."); |
There was a problem hiding this comment.
The old code had an Assert to check Command, I think leaving it as an Assert but also checking ReturnObject is sufficient. It doesn't seem like it needs to change.
| } | ||
| // Only handle "LexEntry" class. | ||
| string className = XmlUtils.GetOptionalAttributeValue(command.Parameters[0], "className"); | ||
| if (className == null || className != "LexEntry") |
| return; | ||
| } | ||
| // Only handle "FsClosedFeature" for "CmdInsertPhonologicalClosedFeature". | ||
| if (className == "FsClosedFeature" && command.Id != "CmdInsertPhonologicalClosedFeature") |
2 participants
Add a ReturnObject class to return a ‘handled’ value, which the Publisher needs to know. All of the Subscribers only handle a specific className or className and Id combination so it did not matter that two of the classes are derived from DlgListenerBase, which has ColleaguePriority.High.
This change is