eclipse-platform / eclipse.platform.ui

Eclipse Platform
https://projects.eclipse.org/projects/eclipse.platform
Eclipse Public License 2.0
78 stars 163 forks source link

Second IElementUpdater is not called #1261

Open basilevs opened 10 months ago

basilevs commented 10 months ago

While investigating failure of org.eclipse.ui.tests.commands.CommandEnablementTest.testRestoreContributedUI() I've found that org.eclipse.ui.internal.WorkbenchHandlerServiceHandler.updateElement(UIElement, Map) queries local context, but does not track it. When handlers are changed due to a change of activation conditions, neither ICommandService nor WorkbenchHandlerServiceHandler are notified about the fact. This results in loss of menu/toolbar updates from subsequent handlers implementing IElementUpdater.

Here is a part of the test demonstrating the problem (after fixing irrelevant issues):

    @Test
    public void testRestoreContributedUI() throws Exception {

        Field iconField = CommandContributionItem.class.getDeclaredField("icon");
        iconField.setAccessible(true);

        Field labelField = CommandContributionItem.class.getDeclaredField("label");
        labelField.setAccessible(true);

        String menuId = "org.eclipse.ui.tests.Bug275126";
        IWorkbenchWindow window = UITestCase.openTestWindow();
        MenuManager manager = new MenuManager("testRestoreContributedUI", menuId);
        manager.createContextMenu(window.getShell());
        IMenuService menuService = fWorkbench.getService(IMenuService.class);
        menuService.populateContributionManager(manager, MenuUtil.menuUri(menuId));

        String text1 = "text1";
        String text2 = "text2";

        // contributed from plugin.xml
        String contributedLabel = "Contributed Label";

        MenuItem menuItem = getTheOnlyItem(manager);

        // default handler
        assertEquals(contributedLabel, menuItem.getText());
        assertNotNull(menuItem.getImage());

        UpdatingHandler handler1 = new UpdatingHandler(text1);
        activation1 = handlerService.activateHandler(CMD3_ID, handler1, new ActiveContextExpression(CONTEXT_TEST1,
                new String[] { ISources.ACTIVE_CONTEXT_NAME }));
        UpdatingHandler handler2 = new UpdatingHandler(text2);
        activation2 = handlerService.activateHandler(CMD3_ID, handler2, new ActiveContextExpression(CONTEXT_TEST2,
                new String[] { ISources.ACTIVE_CONTEXT_NAME }));

        contextActivation1 = contextService.activateContext(CONTEXT_TEST1);
        UITestCase.processEvents();
        menuItem = getTheOnlyItem(manager);
        assertEquals(text1, menuItem.getText()); // old text is detected, as new IElementUpdater was not visited upon activation
        assertNotNull(menuItem.getImage());
               contextService.deactivateContext(contextActivation1);
     }
    private static MenuItem getTheOnlyItem(MenuManager manager) {
        Menu menu = manager.getMenu();
        menu.setVisible(true);
        try {
            UITestCase.processEventsUntil(() -> menu.isVisible(), 10000);
            assertEquals(1, menu.getItemCount());
            return Objects.requireNonNull(menu.getItem(0));
        } finally {
            menu.setVisible(false);
        }
    }

The following change fixes the immediate problem, but leaks listeners and does not take changed command parameters into account:

    @Override
    public void updateElement(UIElement element, Map parameters) {
        final IEclipseContext executionContext = getExecutionContext(null);
        if (executionContext == null) {
            return;
        }

        executionContext.runAndTrack(new RunAndTrack() {
            @Override
            public boolean changed(IEclipseContext context) {
                Object handler = HandlerServiceImpl.lookUpHandler(executionContext, commandId);
                if (handler instanceof IElementUpdater) {
                    runExternalCode(() -> ((IElementUpdater) handler).updateElement(element, parameters));
                }
                return true;
            }
        });
    }
basilevs commented 10 months ago

Similar problem of failure to track handler removal is observed in https://github.com/eclipse-platform/eclipse.platform.ui/pull/1291