From 07986b875f64f086d4c1e13b63c8ea699ff384eb Mon Sep 17 00:00:00 2001 From: peter Date: Wed, 23 Nov 2011 20:09:49 +0100 Subject: [PATCH] avoid race conditions leading to minimal lookup size --- .../codeInsight/lookup/impl/Advertiser.java | 31 +++++-------- .../lookup/impl/LookupActionsStep.java | 1 + .../codeInsight/lookup/impl/LookupImpl.java | 44 ++++++++++++------- .../codeInsight/lookup/impl/LookupModel.java | 23 +++++----- 4 files changed, 52 insertions(+), 47 deletions(-) diff --git a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/Advertiser.java b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/Advertiser.java index a5dab4e0dbdc..1587c722ce6d 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/Advertiser.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/Advertiser.java @@ -16,6 +16,7 @@ package com.intellij.codeInsight.lookup.impl; import com.google.common.collect.ImmutableMap; +import com.intellij.openapi.application.ApplicationManager; import com.intellij.util.ui.GridBag; import com.intellij.util.ui.UIUtil; import org.jetbrains.annotations.NotNull; @@ -25,7 +26,6 @@ import java.awt.*; import java.awt.event.MouseAdapter; import java.awt.event.MouseEvent; import java.awt.font.TextAttribute; -import java.util.ArrayList; import java.util.List; import java.util.Random; import java.util.concurrent.CopyOnWriteArrayList; @@ -38,13 +38,12 @@ public class Advertiser { private final JPanel myComponent = new JPanel(new GridBagLayout()) { @Override public Dimension getPreferredSize() { - List texts = getTexts(); - if (texts.isEmpty()) { + if (myTexts.isEmpty()) { return new Dimension(-1, 0); } int maxSize = 0; - for (String label : texts) { + for (String label : myTexts) { maxSize = Math.max(maxSize, createLabel(label).getPreferredSize().width); } @@ -78,11 +77,10 @@ public class Advertiser { } private void updateAdvertisements() { - List texts = getTexts(); - myNextLabel.setVisible(texts.size() > 1); + myNextLabel.setVisible(myTexts.size() > 1); myTextPanel.removeAll(); - if (!texts.isEmpty()) { - String text = texts.get(myCurrentItem % texts.size()); + if (!myTexts.isEmpty()) { + String text = myTexts.get(myCurrentItem % myTexts.size()); myTextPanel.add(createLabel(text)); } myComponent.revalidate(); @@ -95,17 +93,14 @@ public class Advertiser { return label; } - private synchronized List getTexts() { - return new ArrayList(myTexts); - } - public void showRandomText() { int count = myTexts.size(); myCurrentItem = count > 0 ? new Random().nextInt(count) : 0; updateAdvertisements(); } - public synchronized void clearAdvertisements() { + public void clearAdvertisements() { + ApplicationManager.getApplication().assertIsDispatchThread(); myTexts.clear(); myCurrentItem = 0; updateAdvertisements(); @@ -116,14 +111,10 @@ public class Advertiser { return font.deriveFont((float)(font.getSize() - 2)); } - public synchronized void addAdvertisement(@NotNull String text) { + public void addAdvertisement(@NotNull String text) { + ApplicationManager.getApplication().assertIsDispatchThread(); myTexts.add(text); - UIUtil.invokeLaterIfNeeded(new Runnable() { - @Override - public void run() { - updateAdvertisements(); - } - }); + updateAdvertisements(); } public JComponent getAdComponent() { diff --git a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupActionsStep.java b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupActionsStep.java index 5adab0d266fa..258a6815cd3d 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupActionsStep.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupActionsStep.java @@ -60,6 +60,7 @@ public class LookupActionsStep extends BaseListPopupStep im myLookup.hide(); } else if (result == LookupElementAction.Result.REFRESH_ITEM) { myLookup.updateLookupWidth(myLookupElement); + myLookup.requestResize(); myLookup.refreshUi(false); } else if (result instanceof LookupElementAction.Result.ChooseItem) { myLookup.setCurrentItem(((LookupElementAction.Result.ChooseItem)result).item); diff --git a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupImpl.java b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupImpl.java index 6ed8cd6df493..615779107f72 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupImpl.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupImpl.java @@ -47,7 +47,7 @@ import com.intellij.openapi.ui.popup.JBPopupFactory; import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.Disposer; import com.intellij.openapi.util.IconLoader; -import com.intellij.openapi.util.Pair; +import com.intellij.openapi.util.Trinity; import com.intellij.openapi.util.registry.Registry; import com.intellij.openapi.util.text.StringUtil; import com.intellij.psi.PsiDocumentManager; @@ -70,6 +70,7 @@ import com.intellij.util.containers.ContainerUtil; import com.intellij.util.ui.AbstractLayoutManager; import com.intellij.util.ui.AsyncProcessIcon; import com.intellij.util.ui.ButtonlessScrollBarUI; +import com.intellij.util.ui.UIUtil; import gnu.trove.TObjectHashingStrategy; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -125,7 +126,7 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable private volatile String myAdText; private volatile int myLookupTextWidth = 50; private boolean myChangeGuard; - private LookupModel myModel = new LookupModel(EMPTY_LOOKUP_ITEM); + private volatile LookupModel myModel = new LookupModel(EMPTY_LOOKUP_ITEM); private LookupModel myPresentableModel = myModel; @SuppressWarnings("unchecked") private final Map myMatchers = new ConcurrentHashMap(TObjectHashingStrategy.IDENTITY); private LookupHint myElementHint = null; @@ -300,6 +301,10 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable myLookupTextWidth = Math.max(maxWidth, myLookupTextWidth); myModel.setItemPresentation(item, presentation); + } + + public void requestResize() { + ApplicationManager.getApplication().assertIsDispatchThread(); myResizePending = true; } @@ -350,7 +355,7 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable myAdditionalPrefix += c; myInitialPrefix = null; myFrozenItems.clear(); - myResizePending = true; + requestResize(); refreshUi(false); ensureSelectionVisible(); } @@ -381,7 +386,7 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable myAdditionalPrefix = myAdditionalPrefix.substring(0, len - 1); myInitialPrefix = null; myFrozenItems.clear(); - myResizePending = true; + requestResize(); if (myPresentableModel == myModel) { refreshUi(false); ensureSelectionVisible(); @@ -390,15 +395,15 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable return true; } - private void updateList() { + private boolean updateList() { if (!ApplicationManager.getApplication().isUnitTestMode()) { ApplicationManager.getApplication().assertIsDispatchThread(); } checkValid(); - final Pair,Iterable>> snapshot = myPresentableModel.getModelSnapshot(); + final Trinity, Iterable>, Boolean> snapshot = myPresentableModel.getModelSnapshot(); - final LinkedHashSet items = matchingItems(snapshot); + final LinkedHashSet items = matchingItems(snapshot.first); checkMinPrefixLengthChanges(items); @@ -465,6 +470,7 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable myList.setSelectedIndex(0); } } + return snapshot.third; } private static boolean shouldSkip(LookupElement element) { @@ -475,9 +481,9 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable return myList.getFirstVisibleIndex() <= myList.getSelectedIndex() && myList.getSelectedIndex() <= myList.getLastVisibleIndex(); } - private LinkedHashSet matchingItems(Pair, Iterable>> snapshot) { + private LinkedHashSet matchingItems(final List elements) { final LinkedHashSet items = new LinkedHashSet(); - for (LookupElement element : snapshot.first) { + for (LookupElement element : elements) { if (prefixMatches(element)) { items.add(element); } @@ -554,6 +560,7 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable model.addElement(item); updateLookupWidth(item); + requestResize(); } private static LookupElementPresentation renderItemApproximately(LookupElement item) { @@ -1329,7 +1336,7 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable boolean selectionVisible = isSelectionVisible(); - updateList(); + boolean itemsChanged = updateList(); if (isVisible()) { LOG.assertTrue(!ApplicationManager.getApplication().isUnitTestMode()); @@ -1340,13 +1347,13 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable updateScrollbarVisibility(); - if (myResizePending) { + if (myResizePending || itemsChanged) { myMaximumHeight = Integer.MAX_VALUE; } Rectangle rectangle = calculatePosition(); myMaximumHeight = rectangle.height; - if (myResizePending) { + if (myResizePending || itemsChanged) { myResizePending = false; pack(); } @@ -1373,12 +1380,17 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable public void markReused() { myAdComponent.clearAdvertisements(); myModel = new LookupModel(null); - myResizePending = true; + requestResize(); } - public void addAdvertisement(@NotNull String text) { - myAdComponent.addAdvertisement(text); - myResizePending = true; + public void addAdvertisement(@NotNull final String text) { + UIUtil.invokeLaterIfNeeded(new Runnable() { + @Override + public void run() { + myAdComponent.addAdvertisement(text); + requestResize(); + } + }); } public boolean isLookupDisposed() { diff --git a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupModel.java b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupModel.java index d110f6e951ae..db4e9cce94b5 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupModel.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/lookup/impl/LookupModel.java @@ -16,12 +16,14 @@ package com.intellij.codeInsight.lookup.impl; import com.intellij.codeInsight.completion.PrefixMatcher; -import com.intellij.codeInsight.lookup.*; -import com.intellij.openapi.util.Pair; +import com.intellij.codeInsight.lookup.Classifier; +import com.intellij.codeInsight.lookup.LookupArranger; +import com.intellij.codeInsight.lookup.LookupElement; +import com.intellij.codeInsight.lookup.LookupElementPresentation; +import com.intellij.openapi.util.Trinity; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.SortedList; import gnu.trove.THashMap; -import gnu.trove.THashSet; import gnu.trove.TObjectHashingStrategy; import org.jetbrains.annotations.Nullable; @@ -45,6 +47,8 @@ public class LookupModel { private LookupArranger myArranger; private Classifier myRelevanceClassifier; @Nullable public LookupElement preselectedItem; + private int stamp; + private int lastAccess; public LookupModel(LookupElement preselectedItem) { this.preselectedItem = preselectedItem; @@ -70,6 +74,7 @@ public class LookupModel { mySortedItems.add(item); // ProcessCanceledException may occur in these two lines, then this element is considered not added myItems.add(item); + stamp++; } } @@ -86,17 +91,13 @@ public class LookupModel { } } - public Pair, Iterable>> getModelSnapshot() { + public Trinity, Iterable>, Boolean> getModelSnapshot() { synchronized (lock) { final List sorted = new ArrayList(mySortedItems); final Iterable> groups = myRelevanceClassifier.classify(sorted); - return Pair.create(sorted, groups); - } - } - - public void collectGarbage() { - synchronized (lock) { - myItemPresentations.keySet().retainAll(new THashSet(myItems, TObjectHashingStrategy.IDENTITY)); + boolean changed = lastAccess != stamp; + lastAccess = stamp; + return Trinity.create(sorted, groups, changed); } }