From 941dc4d294d99d0f540cae7d45eb6484e63ccb1a Mon Sep 17 00:00:00 2001 From: peter Date: Fri, 7 Dec 2018 19:36:59 +0100 Subject: [PATCH] async toolbar action update fixes invoke EDT stuff in a correct modality state allow (wrong and racy, but still used) DataContext creation from non-EDT non-cancellable read action (IDEA-203825, IDEA-203645) --- .../BackgroundableDataProvider.java | 2 - .../intellij/ide/impl/DataManagerImpl.java | 79 ++++----------- .../actionSystem/impl/ActionToolbarImpl.java | 8 +- .../actionSystem/impl/ActionUpdater.java | 1 + .../actionSystem/impl/AsyncDataContext.java | 96 +++++++++++++++++++ 5 files changed, 121 insertions(+), 65 deletions(-) create mode 100644 platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/AsyncDataContext.java diff --git a/platform/editor-ui-api/src/com/intellij/openapi/actionSystem/BackgroundableDataProvider.java b/platform/editor-ui-api/src/com/intellij/openapi/actionSystem/BackgroundableDataProvider.java index b99221d3cf95..f00576765fd1 100644 --- a/platform/editor-ui-api/src/com/intellij/openapi/actionSystem/BackgroundableDataProvider.java +++ b/platform/editor-ui-api/src/com/intellij/openapi/actionSystem/BackgroundableDataProvider.java @@ -1,7 +1,6 @@ // Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. package com.intellij.openapi.actionSystem; -import com.intellij.openapi.application.ApplicationManager; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -17,7 +16,6 @@ public interface BackgroundableDataProvider extends DataProvider { @Nullable @Override default Object getData(@NotNull String dataId) { - ApplicationManager.getApplication().assertIsDispatchThread(); DataProvider async = createBackgroundDataProvider(); return async == null ? null : async.getData(dataId); } diff --git a/platform/platform-impl/src/com/intellij/ide/impl/DataManagerImpl.java b/platform/platform-impl/src/com/intellij/ide/impl/DataManagerImpl.java index 5a0840af6c03..1d1e8bcc4219 100644 --- a/platform/platform-impl/src/com/intellij/ide/impl/DataManagerImpl.java +++ b/platform/platform-impl/src/com/intellij/ide/impl/DataManagerImpl.java @@ -6,7 +6,6 @@ import com.intellij.ide.IdeEventQueue; import com.intellij.ide.ProhibitAWTEvents; import com.intellij.ide.impl.dataRules.*; import com.intellij.openapi.actionSystem.*; -import com.intellij.openapi.actionSystem.impl.ActionUpdateEdtExecutor; import com.intellij.openapi.application.AccessToken; import com.intellij.openapi.application.ApplicationManager; import com.intellij.openapi.application.ModalityState; @@ -24,9 +23,7 @@ import com.intellij.openapi.wm.ex.WindowManagerEx; import com.intellij.openapi.wm.impl.FloatingDecorator; import com.intellij.reference.SoftReference; import com.intellij.util.KeyedLazyInstanceEP; -import com.intellij.util.containers.ConcurrentFactoryMap; import com.intellij.util.containers.ContainerUtil; -import com.intellij.util.containers.JBIterable; import com.intellij.util.ui.SwingHelper; import gnu.trove.THashSet; import org.jetbrains.annotations.NonNls; @@ -37,9 +34,12 @@ import org.jetbrains.concurrency.Promise; import javax.swing.*; import java.awt.*; +import java.lang.ref.Reference; import java.lang.ref.WeakReference; -import java.util.List; -import java.util.*; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ConcurrentMap; @@ -53,12 +53,10 @@ public class DataManagerImpl extends DataManager { } @Nullable - private Object getData(@NotNull String dataId, List> hierarchy, Map dataProviders) { + private Object getData(@NotNull String dataId, final Component focusedComponent) { try (AccessToken ignored = ProhibitAWTEvents.start("getData")) { - for (WeakReference reference : hierarchy) { - Component component = SoftReference.dereference(reference); - if (component == null) continue; - DataProvider dataProvider = dataProviders.get(component); + for (Component c = focusedComponent; c != null; c = c.getParent()) { + final DataProvider dataProvider = getDataProviderEx(c); if (dataProvider == null) continue; Object data = getDataFromProvider(dataProvider, dataId, null); if (data != null) return data; @@ -93,9 +91,8 @@ public class DataManagerImpl extends DataManager { } } - @SuppressWarnings("deprecation") @Nullable - public static DataProvider getDataProviderEx(Component component) { + public static DataProvider getDataProviderEx(Object component) { DataProvider dataProvider = null; if (component instanceof DataProvider) { dataProvider = (DataProvider)component; @@ -318,54 +315,13 @@ public class DataManagerImpl extends DataManager { // To prevent memory leak we have to wrap passed component into // the weak reference. For example, Swing often remembers menu items // that have DataContext as a field. - private final List> myHierarchy; - @SuppressWarnings("deprecation") - private final Map myProviders = new ConcurrentFactoryMap() { - @Nullable - @Override - protected DataProvider create(Component key) { - return ActionUpdateEdtExecutor.computeOnEdt(() -> { - DataProvider provider = getDataProviderEx(key); - if (provider == null) return null; - - if (provider instanceof BackgroundableDataProvider) { - return ((BackgroundableDataProvider)provider).createBackgroundDataProvider(); - } - return dataKey -> { - boolean bg = !ApplicationManager.getApplication().isDispatchThread(); - return ActionUpdateEdtExecutor.computeOnEdt(() -> { - long start = System.currentTimeMillis(); - try { - return provider.getData(dataKey); - } - finally { - long elapsed = System.currentTimeMillis() - start; - if (elapsed > 100 && bg) { - LOG.warn("Slow data provider " + provider + " took " + elapsed + "ms on " + dataKey + - ". Consider speeding it up and/or implementing BackgroundableDataProvider."); - } - } - }); - }; - }); - } - - @NotNull - @Override - protected ConcurrentMap createMap() { - return ContainerUtil.createConcurrentWeakKeySoftValueMap(); - } - }; + private final Reference myRef; private Map myUserData; private final Map myCachedData = ContainerUtil.createWeakValueMap(); public MyDataContext(@Nullable Component component) { myEventCount = -1; - List hierarchy = JBIterable.generate(component, Component::getParent).toList(); - for (Component each : hierarchy) { - myProviders.get(each); - } - myHierarchy = ContainerUtil.map(hierarchy, WeakReference::new); + myRef = component == null ? null : new WeakReference<>(component); } public void setEventCount(int eventCount, Object caller) { @@ -401,7 +357,7 @@ public class DataManagerImpl extends DataManager { @Nullable private Object doGetData(@NotNull String dataId) { - Component component = SoftReference.dereference(ContainerUtil.getFirstItem(myHierarchy)); + Component component = SoftReference.dereference(myRef); if (PlatformDataKeys.IS_MODAL_CONTEXT.is(dataId)) { if (component == null) { return null; @@ -415,16 +371,19 @@ public class DataManagerImpl extends DataManager { return component != null ? ModalityState.stateForComponent(component) : ModalityState.NON_MODAL; } if (CommonDataKeys.EDITOR.is(dataId) || CommonDataKeys.HOST_EDITOR.is(dataId)) { - Editor editor = (Editor)((DataManagerImpl)DataManager.getInstance()).getData(dataId, myHierarchy, myProviders); - return validateEditor(editor); + return validateEditor((Editor)calcData(dataId, component)); } - return ((DataManagerImpl)DataManager.getInstance()).getData(dataId, myHierarchy, myProviders); + return calcData(dataId, component); + } + + protected Object calcData(@NotNull String dataId, Component component) { + return ((DataManagerImpl)DataManager.getInstance()).getData(dataId, component); } @Override @NonNls public String toString() { - return "component=" + SoftReference.dereference(ContainerUtil.getFirstItem(myHierarchy)); + return "component=" + SoftReference.dereference(myRef); } @Override diff --git a/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionToolbarImpl.java b/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionToolbarImpl.java index faa9849a3f3c..69e8b427d3fd 100644 --- a/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionToolbarImpl.java +++ b/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionToolbarImpl.java @@ -1130,9 +1130,11 @@ public class ActionToolbarImpl extends JPanel implements ActionToolbar, QuickAct private void updateActionsImpl(boolean transparentOnly, boolean forced) { DataContext dataContext = getDataContext(); - ActionUpdater updater = new ActionUpdater(LaterInvocator.isInModalContext(), myPresentationFactory, dataContext, myPlace, - false, true, transparentOnly); - if (myAlreadyUpdated && Registry.is("actionSystem.update.actions.asynchronously") && ourToolbars.contains(this)) { + boolean async = myAlreadyUpdated && Registry.is("actionSystem.update.actions.asynchronously") && ourToolbars.contains(this) && isShowing(); + ActionUpdater updater = new ActionUpdater(LaterInvocator.isInModalContext(), myPresentationFactory, + async ? new AsyncDataContext(dataContext) : dataContext, + myPlace, false, true, transparentOnly); + if (async) { if (myLastUpdate != null) myLastUpdate.cancel(); myLastUpdate = updater.expandActionGroupAsync(myActionGroup, false); diff --git a/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionUpdater.java b/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionUpdater.java index 6ccbee351c71..f11b014728b8 100644 --- a/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionUpdater.java +++ b/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/ActionUpdater.java @@ -146,6 +146,7 @@ class ActionUpdater { CancellablePromise> expandActionGroupAsync(ActionGroup group, boolean hideDisabled) { AsyncPromise> promise = new AsyncPromise<>(); ProgressIndicator indicator = new ProgressIndicatorBase(true); + indicator.setModalityProgress(indicator); promise.onError(__ -> { indicator.cancel(); ActionUpdateEdtExecutor.computeOnEdt(() -> { diff --git a/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/AsyncDataContext.java b/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/AsyncDataContext.java new file mode 100644 index 000000000000..d4a2b182f91d --- /dev/null +++ b/platform/platform-impl/src/com/intellij/openapi/actionSystem/impl/AsyncDataContext.java @@ -0,0 +1,96 @@ +// Copyright 2000-2018 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package com.intellij.openapi.actionSystem.impl; + +import com.intellij.ide.DataManager; +import com.intellij.ide.ProhibitAWTEvents; +import com.intellij.ide.impl.DataManagerImpl; +import com.intellij.openapi.actionSystem.BackgroundableDataProvider; +import com.intellij.openapi.actionSystem.DataContext; +import com.intellij.openapi.actionSystem.DataProvider; +import com.intellij.openapi.actionSystem.PlatformDataKeys; +import com.intellij.openapi.application.AccessToken; +import com.intellij.openapi.application.ApplicationManager; +import com.intellij.openapi.diagnostic.Logger; +import com.intellij.reference.SoftReference; +import com.intellij.util.containers.ConcurrentFactoryMap; +import com.intellij.util.containers.ContainerUtil; +import com.intellij.util.containers.JBIterable; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.awt.*; +import java.lang.ref.WeakReference; +import java.util.List; +import java.util.Map; +import java.util.concurrent.ConcurrentMap; + +import static com.intellij.ide.impl.DataManagerImpl.getDataProviderEx; + +class AsyncDataContext extends DataManagerImpl.MyDataContext { + private static final Logger LOG = Logger.getInstance(AsyncDataContext.class); + private final List> myHierarchy; + @SuppressWarnings("deprecation") + private final Map myProviders = new ConcurrentFactoryMap() { + @Nullable + @Override + protected DataProvider create(Component key) { + return ActionUpdateEdtExecutor.computeOnEdt(() -> { + DataProvider provider = getDataProviderEx(key); + if (provider == null) return null; + + if (provider instanceof BackgroundableDataProvider) { + return ((BackgroundableDataProvider)provider).createBackgroundDataProvider(); + } + return dataKey -> { + boolean bg = !ApplicationManager.getApplication().isDispatchThread(); + return ActionUpdateEdtExecutor.computeOnEdt(() -> { + long start = System.currentTimeMillis(); + try { + return provider.getData(dataKey); + } + finally { + long elapsed = System.currentTimeMillis() - start; + if (elapsed > 100 && bg) { + LOG.warn("Slow data provider " + provider + " took " + elapsed + "ms on " + dataKey + + ". Consider speeding it up and/or implementing BackgroundableDataProvider."); + } + } + }); + }; + }); + } + + @NotNull + @Override + protected ConcurrentMap createMap() { + return ContainerUtil.createConcurrentWeakKeySoftValueMap(); + } + }; + + AsyncDataContext(DataContext syncContext) { + super(syncContext.getData(PlatformDataKeys.CONTEXT_COMPONENT)); + ApplicationManager.getApplication().assertIsDispatchThread(); + Component component = getData(PlatformDataKeys.CONTEXT_COMPONENT); + List hierarchy = JBIterable.generate(component, Component::getParent).toList(); + for (Component each : hierarchy) { + myProviders.get(each); + } + myHierarchy = ContainerUtil.map(hierarchy, WeakReference::new); + } + + @Override + protected Object calcData(@NotNull String dataId, Component focused) { + try (AccessToken ignored = ProhibitAWTEvents.start("getData")) { + for (WeakReference reference : myHierarchy) { + Component component = SoftReference.dereference(reference); + if (component == null) continue; + DataProvider dataProvider = myProviders.get(component); + if (dataProvider == null) continue; + Object data = ((DataManagerImpl)DataManager.getInstance()).getDataFromProvider(dataProvider, dataId, null); + if (data != null) return data; + } + } + return null; + } + +}