Bugzilla – Full Text Bug Listing
|
Description
Sebastian Ratz
New Gerrit change created: https://git.eclipse.org/r/147067 Gerrit change https://git.eclipse.org/r/147067 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=18dff627915a938700a87dc615ea760207daa180 Sebastian, thanks for the first patch, tests are green in all SDK builds, great! Do you plan to continue on disabled tests in the CommandsTestSuite? Yes, I'll look at the failing ones and see how easily they can be fixed. New Gerrit change created: https://git.eclipse.org/r/147381 Gerrit change https://git.eclipse.org/r/147381 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=399f96bcd7b88224143b485819a41c608f08fe1e Is this done Sebastian? Partly. There are still some failing tests commented out that I haven't had time to look at, yet: https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.ui/+/master/tests/org.eclipse.ui.tests/Eclipse%20UI%20Tests/org/eclipse/ui/tests/commands/CommandsTestSuite.java#40 About Bug66182Test: In bug 66182 / https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.ui/+/13b8b30e687f9d77bc9e03b972d634a782a7078d a fallback mechanism was introduced to LegacyHandlerSubmissionExpression that implicitly matches the shell of the active workbench window if the active shell does not match. At the time that was implemented, WorkbenchCommandSupport did the handler proprity determination itself. It then correctly considered a handler matching the exact shell to be of higher priority than one that only matches that of the the active workbench as fallback. With bug 88656 / https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.ui/+/2aa0965bbe6d7e1c9acd243af620354c7eb93e19 WorkbenchCommandSupport was re-implemented to use the IHandlerService. Since that point, this special handling is not working correctly anymore (the tests in Bug66182Test.java broke and were commented out): With the delegation to the handler activation, the priority of equivalent handlers cannot be compared correctly anymore: HandlerActivation.compareTo() always returns 0 since sourcePriority is always statically determined. That means, that handler conflicts could now theoretically occur. Whats's more interesting: These days, the decision on whether the active workbench window is relevant, lives in https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.ui/+/7f617ce6fdb4be8f151d192ac1dadde271445913/bundles/org.eclipse.ui.workbench/Eclipse%20UI/org/eclipse/ui/internal/services/WorkbenchSourceProvider.java#803 If a TYPE_DIALOG shell is open, ISources.ACTIVE_WORKBENCH_WINDOW_SHELL_NAME will not be put into the currentState anymore. However, and this is the part I don't get, this change to ISources.ACTIVE_WORKBENCH_WINDOW_SHELL_NAME will never reach the IEclipseContext, because in https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.ui/+/7f617ce6fdb4be8f151d192ac1dadde271445913/bundles/org.eclipse.ui.workbench/Eclipse%20UI/org/eclipse/ui/internal/services/EvaluationService.java#129 updates to these variables are filtered out. Removing ISources.ACTIVE_WORKBENCH_WINDOW_SHELL_NAME from that filter gets all test of Bug66182Test.java green again. However, Removing *everything* from that filter completely breaks the IDE completely (e.g. no more keybindings work in the editor). That filter was introduced in bug 338056 but I don't understand why (it also does not contain *all* variables of ISources, why?). There is more filtering going on in ContextAuthority and ExpressionAuthority which I don't understand. Currently, the update of ISources.ACTIVE_WORKBENCH_WINDOW_SHELL_NAME in the context seems to happen only indirectly here: https://git.eclipse.org/r/plugins/gitiles/platform/eclipse.platform.ui/+/7f617ce6fdb4be8f151d192ac1dadde271445913/bundles/org.eclipse.ui.workbench/Eclipse%20UI/org/eclipse/ui/internal/Workbench.java#1905 I.e. only if the selection changes, that variable gets 'repaired' along the way (bug 304920). This smells like there a real problem/bug slumbering here that's somewhat hidden by mitigation in various places. But I'm completely out of clues here. @Rolf Do you have an idea? New Gerrit change created: https://git.eclipse.org/r/150107 Gerrit change https://git.eclipse.org/r/150107 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=941d2be6271d31fe4063caf33a6c31b022da1be4 Sebastian, any more work planned here or can this bug be closed? Still most of the testcases are disabled and broken. For instance, commands with state are not supported in E4. Indeed. I pretty much gave up here. However, in my local dev branch I have added comments to the individual tests explaining the reasons (from my understanding) why the tests do not work anymore. In some cases it even hints to broken functionality. I can upload a gerrit change with these comments but I don't think there's a way to actually get these tests running again. (In reply to Sebastian Ratz from comment #14) > I can upload a gerrit change with these comments +1 New Gerrit change created: https://git.eclipse.org/r/160252 New Gerrit change created: https://git.eclipse.org/r/160253 New Gerrit change created: https://git.eclipse.org/r/160254 New Gerrit change created: https://git.eclipse.org/r/160255 Any objections on the two documentation-only changes? https://git.eclipse.org/r/#/c/160254/ https://git.eclipse.org/r/#/c/160255/ I'll submit them if there are no objections. What about ActionDelegateProxyTest? https://git.eclipse.org/r/#/c/160252/ The change is rather simple but I'd like to get some feedback before changing productive code just for the sake of getting a test back to working. Gerrit change https://git.eclipse.org/r/160255 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=ddad85c8e210e952efdf3bba3ac968277c75a6d1 Gerrit change https://git.eclipse.org/r/160254 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.ui.git/commit/?id=b41fa1db559320de86afbab2b12b1d5766a38f2e |