This repository was archived by the owner on Feb 6, 2025. It is now read-only.
operationsClient's removeCompletedOperations doesn't work#14
Open
rcjsuen wants to merge 1 commit intoeclipse-archived:masterfrom
Open
operationsClient's removeCompletedOperations doesn't work#14rcjsuen wants to merge 1 commit intoeclipse-archived:masterfrom
rcjsuen wants to merge 1 commit intoeclipse-archived:masterfrom
Conversation
…tionsClient When the operations client tries to remove completed operations, it retrieves the current collection of operations, removes the completed ones, and sets the modified collection back. However, this operation doesn't actually work because the preferences services assumes that a modification is being made to the passed in collection and that the entries that have been removed from the collection should not be acted upon. To fix this, the clear flag will be defined and set to true so that the preferences service will know that the preferences should be overwritten with the passed in collection. Signed-off-by: Remy Suen <remy.suen@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
operationsClient'sremoveCompletedOperationsdoesn't actually remove the completed operations when the call is made to the preferences service. This is because it checks for acleardefined on theoptionsto determine whether the original preference's value should be merged with the passed in value or not.In our case, the operations list gets parsed and completed entries gets removed. Then it gets to the preferences service and the removed entries gets added back in because it believes that only the passed in values should be modified and any values that aren't passed in should just be preserved. This makes sense when thinking of preferences in the regular way but fails in the
removeCompletedOperationscase so the fix is to send in{ clear: true }as an option when trying to remove the completed operations.Note that this pull request is technically incomplete as the test doesn't seem to fail. When I comment out the fix, the test will just timeout instead of failing. The error that the assertion throws seems to get swallowed somewhere. Can someone with some knowledge about our
Deferredimplementation take a look at this?