Community
Participate
Working Groups
Created attachment 95811 [details] Removes activities from the enabled list which are controlled by disabled expressions Build ID: I20080330-1350 Steps To Reproduce: 1. Add expression controlled disabled activities with the ActivityManager.setEnabledActivities() method to the enabled activities list. 2. Enable their expressions and fire their evaluation source. 3. Notice that they aren't updated properly since the system don't recognizes the difference since it has them already in the enabled list. More information: With this patch I just remove every activity which is disabled and controlled by an expression from the setEnabledActivities list. Due to a logic short-cut in the code this may be a risk, because it says if every activity is in the enabled list it doesn't question the TriggerPointAdvisor which tests against the expressions.
(In reply to comment #0) > With this patch I just remove every activity which is disabled and controlled > by an expression from the setEnabledActivities list. Due to a logic short-cut > in the code this may be a risk, ... This was bad described: the patch offers no risk, but if it's not patched the described risk comes up.
Created attachment 96016 [details] Extended Patch against wrong enablement This patch includes the last patch (see comment #0), and fixes a second loophole. An activity which was already removed from the enabledList, and then disabled by an expression, wouldn't update it's state without this patch. It would stay filtered but would not be restricted. This patch should be applied ASAP. A corresponding unit test is also included.
I really dont like seeing the expression being tested in the manager - that logic all belongs in the advisor. At least, it has until now... Why is it not sufficient to just strip any activities bound to expressions from the list passed into setEnabledIds?
(In reply to comment #3) > I really dont like seeing the expression being tested in the manager - that > logic all belongs in the advisor. At least, it has until now... Maybe. But forever the only "activity.setEnabled(..)" command has been in the MutableActivityManager! It's in the updateActivity(..) method which is private and called by the public method "setEnabledActivities(..)". And the calling of this method in the activity enabled listener (enabledWhenListener) depends on the enabled state of the activity. In the old version it tested against the state of the enabledList which may be incorrect in that way that it doesn't mirrors the actual state of expression enabled activities. > Why is it not sufficient to just strip any activities bound to expressions > from the list passed into setEnabledIds? Well, you can of course go this way and remove all of the expression controlled activities. But remember that you then have to write a second way where the listener of expression controlled activities don't uses setEnabledActivities(..) and handles the transformation of the enabledList itself (in the "enabledWhenListener") and the updates of other listeners too. So that expression disabled activities are also removed from the enabledList, not only the non-expression-controlled. And they both should be synchronized, don't they? I think no one wishes to make two parallel lists. Additionally I want you point to the logic short-cut point of the code (which is actually really reasonable); it's in MutableActivityManager in "updateIdentifier(Identifier identifier, Set changedActivityIds)", in the following lines: if (enabledActivityIds.size() == definedActivityIds.size()) { enabled = true; enabledChanged = identifier.setEnabled(enabled); ... } else { ... call advisor ... } (That's not by me, this has been already there for a very long time.)
Created attachment 96129 [details] Activity States, very preliminary. For a discussion base.
Created attachment 96367 [details] New patch way proposal nr. 0 This is a patch proposal which divides the control of expression enabled activities and "normal" activities. Expression controlled activities can't be filtered anymore (see Bug 224673). This solution takes a more general approach to divide the two types of activities. It relies on the new MutableActivityManager#setExpressionEnabledActivityIds() method.
Created attachment 96370 [details] New patch way proposal nr. 1 This is a patch proposal which divides the control of expression enabled activities and "normal" activities. Expression controlled activities can't be filtered anymore (see Bug 224673). This solution takes a more specific approach to divide the two types of activities. It relies on the two new MutableActivityManager#addExpressionEnabledActivity and #removExpressionEnabledActivity methods. Both patches are valid working solutions, which use the same unit tests and stuff. The outside API behavior is the same. Paul, Kim: just tell me what your favorite way is - general, specific, 3rd way - and I will work it out (actually these both maybe just need a little more java-doc (although they are all private methods)). Just chose: [ ]: Way 0 [ ]: Way 1 [ ]: Total other way
On first blush I like option 1. I have some minor issues with the names of some methods (remove*) but not enough to complain. I want to look at it a bit further but I think it's a keeper.
(In reply to comment #8) > On first blush I like option 1. I have some minor issues with the names of > some methods (remove*) but not enough to complain. I want to look at it a bit > further but I think it's a keeper. > After discussing with Paul there are still some issues. Basically the problem is this: the code in the patch takes into account a user trying to set enabled an activity bound to an expression but doesn't guard against a user trying to remove enablement from an activity bound to an expression. The following test code should show the problem: TestSourceProvider testSourceProvider = new TestSourceProvider(); IEvaluationService evalService = (IEvaluationService) PlatformUI .getWorkbench().getService(IEvaluationService.class); evalService.addSourceProvider(testSourceProvider); testSourceProvider.fireSourceChanged(); IWorkbenchActivitySupport support = PlatformUI.getWorkbench() .getActivitySupport(); support.setEnabledActivityIds(Collections.emptySet()); Set set = new HashSet(support.getActivityManager().getEnabledActivityIds()); Set previousSet = new HashSet(support.getActivityManager().getEnabledActivityIds()); set.add(EXPRESSION_ACTIVITY_ID_2); support.setEnabledActivityIds(set); assertEquals(previousSet, support.getActivityManager().getEnabledActivityIds()); testSourceProvider.setVariable(); testSourceProvider.fireSourceChanged(); set = new HashSet(support.getActivityManager().getEnabledActivityIds()); assertFalse(set.equals(previousSet)); set.remove(EXPRESSION_ACTIVITY_ID_2); support.setEnabledActivityIds(set); assertFalse(support.getActivityManager().getEnabledActivityIds().equals(previousSet)); evalService.removeSourceProvider(testSourceProvider);
Created attachment 96468 [details] Suggested replacement patch Takes the Option 1 patch and changes it. Instead of processing the deltas as a whole we process additions and removals separately. From each we strip any expression activities. We then use these deltas to augment the previously enabled list, instead of blowing it away. This patch contains the above test case I posted.
(In reply to comment #10) > Created an attachment (id=96468) [details] > Suggested replacement patch > > Takes the Option 1 patch and changes it. Instead of processing the deltas as a > whole we process additions and removals separately. From each we strip any > expression activities. We then use these deltas to augment the previously > enabled list, instead of blowing it away. This patch contains the above test > case I posted. > I haven't run the full set of suites over this patch - only UtilsTest
(In reply to comment #11) > > I haven't run the full set of suites over this patch - only UtilsTest > All tests are passing for me.
(In reply to comment #9) > After discussing with Paul there are still some issues. Basically the problem > is this: the code in the patch takes into account a user trying to set enabled > an activity bound to an expression but doesn't guard against a user trying to > remove enablement from an activity bound to an expression. How? Only the deltaActivityIds are taken into account for changing anything. And every id gets removed from the set if it has an expression. So how can you touch it? It must be total immutable! > The following test code should show the problem: > ... > set = new > HashSet(support.getActivityManager() > .getEnabledActivityIds()); > assertFalse(set.equals(previousSet)); This shouldn't change anything, right? The activity has an expression on it, so it should be totally ignored! > set.remove(EXPRESSION_ACTIVITY_ID_2); > support.setEnabledActivityIds(set); Then, why should this be false? It must be true: they both are still equal. > assertFalse(support.getActivityManager() > .getEnabledActivityIds().equals(previousSet)); > > evalService.removeSourceProvider(testSourceProvider); Instead: assertTrue(support.getActivityManager() .getEnabledActivityIds().equals(previousSet)); I can't see any failure in my code. The call in setEnabledActivityIds(): > removeExpressionControlledActivities(this.enabledActivityIds, > deltaActivityIds); > if (deltaActivityIds.size() > 0) { > activityEventsByActivityId = updateActivities(deltaActivityIds); > } else { return; } And in removeExpressionControlledActivities(): > for (Iterator iterator = deltaActivityIds.iterator(); iterator > .hasNext();) { > String id = (String) iterator.next(); > IActivity activity = (IActivity) activitiesById.get(id); > Expression expression = activity.getExpression(); > if (expression != null) { Look: both are removed! enabledActivityIds and deltaActivityIds, so enabledActivityIds are corrected _and_ deltaActivityIds. > enabledActivityIds.remove(id); > iterator.remove(); ... > } > } So I don't see a chance of abuse.
(In reply to comment #13) > How? Only the deltaActivityIds are taken into account for changing anything. > And every id gets removed from the set if it has an expression. So how can you > touch it? It must be total immutable! This is the point ... the set returned from getEnabledActivityIds() must be the union of any user-set activities plus any expression-enabled-true activities. That means when the user calls setEnabledActivityIds(set) we must remove any expression-enabled-false activities and add back any missing expression-enabled-true activities. > > set.remove(EXPRESSION_ACTIVITY_ID_2); > > support.setEnabledActivityIds(set); > Then, why should this be false? It must be true: they both are still equal. > > assertFalse(support.getActivityManager() > > .getEnabledActivityIds().equals(previousSet)); This is the point ... EXPRESSION_ACTIVITY_ID_2 is enabled (via its expression). It doesn't matter that the user tries to remove it from the list of enabled IDs, it is still enabled and must still be in the getEnabledActivityIds() set PW
(In reply to comment #14) > (In reply to comment #13) > > How? Only the deltaActivityIds are taken into account for changing anything. > > And every id gets removed from the set if it has an expression. So how can you > > touch it? It must be total immutable! > > This is the point ... the set returned from getEnabledActivityIds() must be the > union of any user-set activities plus any expression-enabled-true activities. > > That means when the user calls setEnabledActivityIds(set) we must remove any > expression-enabled-false activities and add back any missing > expression-enabled-true activities. > > > > set.remove(EXPRESSION_ACTIVITY_ID_2); > > > support.setEnabledActivityIds(set); > > Then, why should this be false? It must be true: they both are still equal. > > > assertFalse(support.getActivityManager() > > > .getEnabledActivityIds().equals(previousSet)); > > This is the point ... EXPRESSION_ACTIVITY_ID_2 is enabled (via its expression). > It doesn't matter that the user tries to remove it from the list of enabled > IDs, it is still enabled and must still be in the getEnabledActivityIds() set > > PW > ^--- what he said.
Based on an IRC conversation I think we're now all in agreement. If you dont have any objections Paul I'll put this in for tomorrows build.
Silence is golden. Fix in HEAD.
Created attachment 96906 [details] Little fix to the fix. Sorry. I have to add a fix I already added localy. Sorry. Two lines of code. It just didn't cause any bug, because it fired a notifier with false data, which no one was listening to.
See comment #18 for the reopening cause.
Reducing bug to minor, because no really harmful errors are expected, which can be caused by the bug left.
Patch applied.
Verified by code inspection in I20080429-0100. Jan, if you have a moment, I'd appreciate it if you could verify as well.
Verified in I20080429-0100 with extended junit UI test.
