From dd4697d174130bf59266b5dc4a66e3feba4c8544 Mon Sep 17 00:00:00 2001 From: peter Date: Tue, 13 May 2014 12:09:29 +0200 Subject: [PATCH] IDEA-124379 Avoid completion lookup glitching * advertise all unused features to keep advertiser width constant * show a random advertisement also after the lookup is reused * group advertisement changes together to avoid lookup shrinking and growing back * remove hacky LookupImpl.myAdText kept for historical reasons --- .../CompletionProgressIndicator.java | 98 +++++++++++-------- .../DefaultCompletionContributor.java | 34 +++---- .../impl/CompletionServiceImpl.java | 6 +- .../codeInsight/lookup/impl/LookupImpl.java | 38 ++----- .../template/impl/TemplateState.java | 2 +- 5 files changed, 79 insertions(+), 99 deletions(-) diff --git a/platform/lang-impl/src/com/intellij/codeInsight/completion/CompletionProgressIndicator.java b/platform/lang-impl/src/com/intellij/codeInsight/completion/CompletionProgressIndicator.java index 88fad83cb8c5..7a96fd03fae0 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/completion/CompletionProgressIndicator.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/completion/CompletionProgressIndicator.java @@ -68,14 +68,18 @@ import com.intellij.util.messages.MessageBusConnection; import com.intellij.util.ui.update.MergingUpdateQueue; import com.intellij.util.ui.update.Update; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import org.jetbrains.annotations.TestOnly; import javax.swing.*; +import java.awt.*; import java.awt.event.KeyAdapter; import java.awt.event.KeyEvent; import java.beans.PropertyChangeEvent; import java.beans.PropertyChangeListener; import java.util.List; +import java.util.Queue; +import java.util.concurrent.ConcurrentLinkedQueue; import java.util.concurrent.atomic.AtomicReference; /** @@ -117,10 +121,12 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement } }; private volatile int myCount; + private boolean myLookupUpdated; private final ConcurrentHashMap myItemSorters = new ConcurrentHashMap( ContainerUtil.identityStrategy()); private final PropertyChangeListener myLookupManagerListener; + private final Queue myAdvertiserChanges = new ConcurrentLinkedQueue(); private final int myStartCaret; public CompletionProgressIndicator(final Editor editor, @@ -137,6 +143,13 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement myLookup = (LookupImpl)parameters.getLookup(); myStartCaret = myEditor.getCaretModel().getOffset(); + myAdvertiserChanges.offer(new Runnable() { + @Override + public void run() { + myLookup.getAdvertiser().clearAdvertisements(); + } + }); + myLookup.setArranger(new CompletionLookupArranger(parameters, this)); myLookup.addLookupListener(myLookupListener); @@ -177,9 +190,9 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement if (!CodeInsightSettings.getInstance().SELECT_AUTOPOPUP_SUGGESTIONS_BY_CHARS) { myLookup.setFocusDegree(LookupImpl.FocusDegree.SEMI_FOCUSED); if (FeatureUsageTracker.getInstance().isToBeAdvertisedInLookup(CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_CONTROL_DOT, getProject())) { - myLookup.addAdvertisement("Press " + - CompletionContributor.getActionShortcut(IdeActions.ACTION_CHOOSE_LOOKUP_ITEM_DOT) + - " to choose the selected (or first) suggestion and insert a dot afterwards", null); + addAdvertisement("Press " + + CompletionContributor.getActionShortcut(IdeActions.ACTION_CHOOSE_LOOKUP_ITEM_DOT) + + " to choose the selected (or first) suggestion and insert a dot afterwards", null); } } else { myLookup.setFocusDegree(LookupImpl.FocusDegree.FOCUSED); @@ -188,12 +201,12 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement if (!myEditor.isOneLineMode() && FeatureUsageTracker.getInstance() .isToBeAdvertisedInLookup(CodeCompletionFeatures.EDITING_COMPLETION_CONTROL_ARROWS, getProject())) { - myLookup.addAdvertisement(CompletionContributor.getActionShortcut(IdeActions.ACTION_LOOKUP_DOWN) + " and " + - CompletionContributor.getActionShortcut(IdeActions.ACTION_LOOKUP_UP) + - " will move caret down and up in the editor", null); + addAdvertisement(CompletionContributor.getActionShortcut(IdeActions.ACTION_LOOKUP_DOWN) + " and " + + CompletionContributor.getActionShortcut(IdeActions.ACTION_LOOKUP_UP) + + " will move caret down and up in the editor", null); } } else if (DumbService.isDumb(getProject())) { - myLookup.addAdvertisement("The results might be incomplete while indexing is in progress", MessageType.WARNING.getPopupBackground()); + addAdvertisement("The results might be incomplete while indexing is in progress", MessageType.WARNING.getPopupBackground()); } ProgressManager.checkCanceled(); @@ -245,33 +258,12 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement if (myLookup.isAvailableToUser()) { return; } - final List list = CompletionContributor.forParameters(myParameters); - for (final CompletionContributor contributor : list) { - if (myLookup.getAdvertisementText() != null) return; + for (final CompletionContributor contributor : CompletionContributor.forParameters(myParameters)) { if (!myLookup.isCalculating() && !myLookup.isVisible()) return; @SuppressWarnings("deprecation") String s = contributor.advertise(myParameters); - if (myLookup.getAdvertisementText() != null) return; - if (s != null) { - myLookup.setAdvertisementText(s); - ApplicationManager.getApplication().invokeLater(new Runnable() { - @Override - public void run() { - if (isAutopopupCompletion() && !myLookup.isAvailableToUser()) { - return; - } - if (!CompletionServiceImpl.isPhase(CompletionPhase.BgCalculation.class, CompletionPhase.ItemsCalculated.class)) { - return; - } - if (CompletionServiceImpl.getCompletionPhase().indicator != CompletionProgressIndicator.this) { - return; - } - - updateLookup(); - } - }, myQueue.getModalityState()); - return; + addAdvertisement(s, null); } } } @@ -320,6 +312,19 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement ApplicationManager.getApplication().assertIsDispatchThread(); if (isOutdated() || !shouldShowLookup()) return false; + while (true) { + Runnable action = myAdvertiserChanges.poll(); + if (action == null) break; + action.run(); + } + + if (!myLookupUpdated) { + if (myLookup.getAdvertisements().isEmpty() && !isAutopopupCompletion() && !DumbService.isDumb(getProject())) { + DefaultCompletionContributor.addDefaultAdvertisements(myParameters, myLookup); + } + myLookup.getAdvertiser().showRandomText(); + } + boolean justShown = false; if (!myLookup.isShown()) { if (hideAutopopupIfMeaningless()) { @@ -330,18 +335,12 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement PerformanceWatcher.getInstance().dumpThreads(true); } - if (StringUtil.isEmpty(myLookup.getAdvertisementText()) && !isAutopopupCompletion() && !DumbService.isDumb(getProject())) { - final String text = DefaultCompletionContributor.getDefaultAdvertisementText(myParameters); - if (text != null) { - myLookup.setAdvertisementText(text); - } - } - if (!myLookup.showLookup()) { return false; } justShown = true; } + myLookupUpdated = true; myLookup.refreshUi(true, justShown); hideAutopopupIfMeaningless(); if (justShown) { @@ -351,8 +350,13 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement } private boolean shouldShowLookup() { - if (isAutopopupCompletion() && myLookup.isCalculating() && Registry.is("ide.completion.delay.autopopup.until.completed")) { - return false; + if (isAutopopupCompletion()) { + if (myCount == 0) { + return false; + } + if (myLookup.isCalculating() && Registry.is("ide.completion.delay.autopopup.until.completed")) { + return false; + } } return true; } @@ -532,7 +536,7 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement final Boolean aBoolean = new WriteCommandAction(getProject()) { @Override - protected void run(Result result) throws Throwable { + protected void run(@NotNull Result result) throws Throwable { if (!explicit) { setMergeCommand(); } @@ -550,7 +554,7 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement public void restorePrefix(@NotNull final Runnable customRestore) { new WriteCommandAction(getProject()) { @Override - protected void run(Result result) throws Throwable { + protected void run(@NotNull Result result) throws Throwable { setMergeCommand(); customRestore.run(); @@ -718,6 +722,7 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement final Language language = PsiUtilCore.getLanguageAtOffset(parameters.getPosition().getContainingFile(), parameters.getOffset()); for (CompletionConfidence confidence : CompletionConfidenceEP.forLanguage(language)) { + //noinspection deprecation final ThreeState result = confidence.shouldFocusLookup(parameters); if (result != ThreeState.UNSURE) { LOG.debug(confidence + " has returned shouldFocusLookup=" + result); @@ -771,6 +776,17 @@ public class CompletionProgressIndicator extends ProgressIndicatorBase implement return result; } + public void addAdvertisement(@NotNull final String text, @Nullable final Color bgColor) { + myAdvertiserChanges.offer(new Runnable() { + @Override + public void run() { + myLookup.addAdvertisement(text, bgColor); + } + }); + + myQueue.queue(myUpdate); + } + private static class ModifierTracker extends KeyAdapter { private final JComponent myContentComponent; diff --git a/platform/lang-impl/src/com/intellij/codeInsight/completion/DefaultCompletionContributor.java b/platform/lang-impl/src/com/intellij/codeInsight/completion/DefaultCompletionContributor.java index 20d20ccd7559..9e112ad382a4 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/completion/DefaultCompletionContributor.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/completion/DefaultCompletionContributor.java @@ -18,51 +18,41 @@ package com.intellij.codeInsight.completion; import com.intellij.codeInsight.documentation.actions.ShowQuickDocInfoAction; import com.intellij.codeInsight.hint.actions.ShowImplementationsAction; import com.intellij.codeInsight.lookup.LookupElement; +import com.intellij.codeInsight.lookup.impl.LookupImpl; import com.intellij.lang.LangBundle; import com.intellij.openapi.actionSystem.IdeActions; import com.intellij.openapi.editor.Editor; import com.intellij.openapi.util.text.StringUtil; import org.jetbrains.annotations.NotNull; -import org.jetbrains.annotations.Nullable; - -import java.util.Random; /** * @author peter */ public class DefaultCompletionContributor extends CompletionContributor { - @Nullable - public static String getDefaultAdvertisementText(@NotNull final CompletionParameters parameters) { - final Random random = new Random(); - if (random.nextInt(5) < 2 && CompletionUtil.shouldShowFeature(parameters, CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_DOT_ETC)) { - return LangBundle.message("completion.dot.etc.ad"); + static void addDefaultAdvertisements(@NotNull final CompletionParameters parameters, LookupImpl lookup) { + if (CompletionUtil.shouldShowFeature(parameters, CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_DOT_ETC)) { + lookup.addAdvertisement(LangBundle.message("completion.dot.etc.ad"), null); } - if (random.nextInt(5) < 2 && CompletionUtil.shouldShowFeature(parameters, CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_SMART_ENTER)) { + if (CompletionUtil.shouldShowFeature(parameters, CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_SMART_ENTER)) { final String shortcut = getActionShortcut(IdeActions.ACTION_CHOOSE_LOOKUP_ITEM_COMPLETE_STATEMENT); if (shortcut != null) { - return LangBundle.message("completion.smart.enter.ad", shortcut); + lookup.addAdvertisement(LangBundle.message("completion.smart.enter.ad", shortcut), null); } } - if (random.nextInt(5) < 2 && - CompletionUtil.shouldShowFeature(parameters, CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_DOT_ETC)) { - return LangBundle.message("completion.dot.etc.ad"); - } - if (random.nextInt(5) < 2 && - CompletionUtil.shouldShowFeature(parameters, CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_SMART_ENTER)) { + if (CompletionUtil.shouldShowFeature(parameters, CodeCompletionFeatures.EDITING_COMPLETION_FINISH_BY_SMART_ENTER)) { final String shortcut = getActionShortcut(IdeActions.ACTION_CHOOSE_LOOKUP_ITEM_COMPLETE_STATEMENT); if (shortcut != null) { - return LangBundle.message("completion.smart.enter.ad", shortcut); + lookup.addAdvertisement(LangBundle.message("completion.smart.enter.ad", shortcut), null); } } - if (random.nextInt(5) < 2 && - (CompletionUtil.shouldShowFeature(parameters, ShowQuickDocInfoAction.CODEASSISTS_QUICKJAVADOC_FEATURE) || + if ((CompletionUtil.shouldShowFeature(parameters, ShowQuickDocInfoAction.CODEASSISTS_QUICKJAVADOC_FEATURE) || CompletionUtil.shouldShowFeature(parameters, ShowQuickDocInfoAction.CODEASSISTS_QUICKJAVADOC_LOOKUP_FEATURE))) { final String shortcut = getActionShortcut(IdeActions.ACTION_QUICK_JAVADOC); if (shortcut != null) { - return LangBundle.message("completion.quick.javadoc.ad", shortcut); + lookup.addAdvertisement(LangBundle.message("completion.quick.javadoc.ad", shortcut), null); } } @@ -70,11 +60,9 @@ public class DefaultCompletionContributor extends CompletionContributor { CompletionUtil.shouldShowFeature(parameters, ShowImplementationsAction.CODEASSISTS_QUICKDEFINITION_LOOKUP_FEATURE)) { final String shortcut = getActionShortcut(IdeActions.ACTION_QUICK_IMPLEMENTATIONS); if (shortcut != null) { - return LangBundle.message("completion.quick.implementations.ad", shortcut); + lookup.addAdvertisement(LangBundle.message("completion.quick.implementations.ad", shortcut), null); } } - - return null; } @Override diff --git a/platform/lang-impl/src/com/intellij/codeInsight/completion/impl/CompletionServiceImpl.java b/platform/lang-impl/src/com/intellij/codeInsight/completion/impl/CompletionServiceImpl.java index 7f3384b2bad0..f916c293ddbc 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/completion/impl/CompletionServiceImpl.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/completion/impl/CompletionServiceImpl.java @@ -31,6 +31,7 @@ import com.intellij.psi.Weigher; import com.intellij.psi.WeighingService; import com.intellij.psi.impl.DebugUtil; import com.intellij.util.Consumer; +import com.intellij.util.containers.ContainerUtil; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -68,14 +69,15 @@ public class CompletionServiceImpl extends CompletionService{ @Override public String getAdvertisementText() { final CompletionProgressIndicator completion = getCompletionService().getCurrentCompletion(); - return completion == null ? null : completion.getLookup().getAdvertisementText(); + return completion == null ? null : ContainerUtil.getFirstItem(completion.getLookup().getAdvertisements()); } @Override public void setAdvertisementText(@Nullable final String text) { + if (text == null) return; final CompletionProgressIndicator completion = getCompletionService().getCurrentCompletion(); if (completion != null) { - completion.getLookup().setAdvertisementText(text); + completion.addAdvertisement(text, null); } } 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 57cb3d9bd596..e776b30b7f98 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 @@ -112,7 +112,6 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable private FocusDegree myFocusDegree = FocusDegree.FOCUSED; private volatile boolean myCalculating; private final Advertiser myAdComponent; - private volatile String myAdText; volatile int myLookupTextWidth = 50; private boolean myChangeGuard; private volatile LookupArranger myArranger; @@ -294,18 +293,6 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable } } - public void setAdvertisementText(@Nullable String text) { - myAdText = text; - if (StringUtil.isNotEmpty(text)) { - addAdvertisement(text, null); - } - } - - public String getAdvertisementText() { - return myAdText; - } - - public String getAdditionalPrefix() { return myOffsets.getAdditionalPrefix(); } @@ -712,6 +699,10 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable return true; } + public Advertiser getAdvertiser() { + return myAdComponent; + } + public boolean mayBeNoticed() { return myStampShown > 0 && System.currentTimeMillis() - myStampShown > 300; } @@ -1109,7 +1100,6 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable } public void markReused() { - myAdComponent.clearAdvertisements(); synchronized (myList) { myArranger = myArranger.createEmptyCopy(); } @@ -1121,24 +1111,8 @@ public class LookupImpl extends LightweightHint implements LookupEx, Disposable return; } - Runnable runnable = new Runnable() { - @Override - public void run() { - if (!myDisposed) { - myAdComponent.addAdvertisement(text, bgColor); - if (myShown) { - requestResize(); - refreshUi(false, false); - } - } - } - }; - if (ApplicationManager.getApplication().isDispatchThread()) { - runnable.run(); - } - else { - ApplicationManager.getApplication().invokeLater(runnable); - } + myAdComponent.addAdvertisement(text, bgColor); + requestResize(); } public boolean isLookupDisposed() { diff --git a/platform/lang-impl/src/com/intellij/codeInsight/template/impl/TemplateState.java b/platform/lang-impl/src/com/intellij/codeInsight/template/impl/TemplateState.java index d0c98be6c976..9a0b20d96718 100644 --- a/platform/lang-impl/src/com/intellij/codeInsight/template/impl/TemplateState.java +++ b/platform/lang-impl/src/com/intellij/codeInsight/template/impl/TemplateState.java @@ -591,7 +591,7 @@ public class TemplateState implements Disposable { lookup.setStartCompletionWhenNothingMatches(true); } - lookup.setAdvertisementText(advertisingText); + lookup.addAdvertisement(advertisingText, null); lookup.refreshUi(true, true); ourLookupShown = true; lookup.addLookupListener(new LookupAdapter() {