From bd6db785819f23041314c8065ffe821ca028a96f Mon Sep 17 00:00:00 2001 From: Sergey Malenkov Date: Tue, 2 May 2017 02:02:26 +0300 Subject: [PATCH] IDEA-165089 refactor code after code review --- .../options/schemes/AbstractSchemesPanel.java | 11 + .../options/schemes/EditableSchemesCombo.java | 2 +- .../options/schemes/SchemesModel.java | 11 - .../openapi/keymap/impl/ui/KeymapPanel.java | 29 +-- .../keymap/impl/ui/KeymapSchemeManager.java | 193 +++++++++++++++--- .../keymap/impl/ui/KeymapSelector.java | 166 ++------------- 6 files changed, 200 insertions(+), 212 deletions(-) diff --git a/platform/platform-impl/src/com/intellij/application/options/schemes/AbstractSchemesPanel.java b/platform/platform-impl/src/com/intellij/application/options/schemes/AbstractSchemesPanel.java index 380cac5eeebc..a79baa59622b 100644 --- a/platform/platform-impl/src/com/intellij/application/options/schemes/AbstractSchemesPanel.java +++ b/platform/platform-impl/src/com/intellij/application/options/schemes/AbstractSchemesPanel.java @@ -190,6 +190,17 @@ public abstract class AbstractSchemesPanel { @Override protected int getIndent(@NotNull T scheme) { - return mySchemesPanel.getModel().getIndent(scheme); + return mySchemesPanel.getIndent(scheme); } @NotNull diff --git a/platform/platform-impl/src/com/intellij/application/options/schemes/SchemesModel.java b/platform/platform-impl/src/com/intellij/application/options/schemes/SchemesModel.java index 32e96e8ca96b..327948bb0603 100644 --- a/platform/platform-impl/src/com/intellij/application/options/schemes/SchemesModel.java +++ b/platform/platform-impl/src/com/intellij/application/options/schemes/SchemesModel.java @@ -53,17 +53,6 @@ public interface SchemesModel { */ boolean isProjectScheme(@NotNull T scheme); - /** - * Returns an indent to calculate a left margin for the scheme name in the combo box. - * By default, all names are aligned to the left. - * - * @param scheme The scheme to check. - * @return An indent that shows a nesting level for the specified scheme. - */ - default int getIndent(@NotNull T scheme) { - return 0; - } - /** * @param scheme The scheme to check. * @return True if scheme's name can be edited. diff --git a/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapPanel.java b/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapPanel.java index bb58e32c4a7d..d1f7f36cba3a 100644 --- a/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapPanel.java +++ b/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapPanel.java @@ -62,10 +62,12 @@ import java.beans.PropertyChangeListener; import java.util.List; import java.util.Map; +import static com.intellij.openapi.actionSystem.impl.ActionToolbarImpl.updateAllToolbarsImmediately; + public class KeymapPanel extends JPanel implements SearchableConfigurable, Configurable.NoScroll, KeymapListener, Disposable { private JCheckBox preferKeyPositionOverCharOption; - private final KeymapSelector myKeymapSelector = new KeymapSelector(this::currentKeymapChanged); + private final KeymapSchemeManager myManager = new KeymapSelector(this::currentKeymapChanged).getManager(); private final ActionsTree myActionsTree = new ActionsTree(); private FilterComponent myFilterComponent; private TreeExpansionMonitor myTreeExpansionMonitor; @@ -77,7 +79,7 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi public KeymapPanel() { setLayout(new BorderLayout()); JPanel keymapPanel = new JPanel(new BorderLayout()); - keymapPanel.add(myKeymapSelector, BorderLayout.NORTH); + keymapPanel.add(myManager.getSchemesPanel(), BorderLayout.NORTH); keymapPanel.add(createKeymapSettingsPanel(), BorderLayout.CENTER); IdeFrame ideFrame = IdeFocusManager.getGlobalInstance().getLastFocusedFrame(); @@ -127,7 +129,7 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi @Override public void quickListRenamed(final QuickList oldQuickList, final QuickList newQuickList) { - myKeymapSelector.visitMutableKeymaps(keymap -> { + myManager.visitMutableKeymaps(keymap -> { String actionId = oldQuickList.getActionId(); Shortcut[] shortcuts = keymap.getShortcuts(actionId); if (shortcuts.length != 0) { @@ -158,7 +160,7 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi } private void currentKeymapChanged() { - currentKeymapChanged(myKeymapSelector.getSelectedKeymap()); + currentKeymapChanged(myManager.getSelectedKeymap()); } private void currentKeymapChanged(Keymap selectedKeymap) { @@ -422,7 +424,7 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi private static Keymap createKeymapCopyIfNeededAndPossible(Component parent, Keymap keymap) { if (parent instanceof KeymapPanel) { KeymapPanel panel = (KeymapPanel)parent; - return panel.myKeymapSelector.getMutableKeymap(keymap); + return panel.myManager.getMutableKeymap(keymap); } return keymap; } @@ -438,18 +440,19 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi if (preferKeyPositionOverCharOption != null) { preferKeyPositionOverCharOption.setSelected(KeyboardSettingsExternalizable.getInstance().isNonEnglishKeyboardSupportEnabled()); } - myKeymapSelector.reset(); + myManager.reset(); } @Override public void apply() throws ConfigurationException { - String error = myKeymapSelector.apply(); + String error = myManager.apply(); if (error != null) throw new ConfigurationException(error); + updateAllToolbarsImmediately(); } @Override public boolean isModified() { - return myKeymapSelector.isModified(); + return myManager.isModified(); } public void selectAction(String actionId) { @@ -488,7 +491,7 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi @Nullable public Shortcut[] getCurrentShortcuts(@NotNull String actionId) { - Keymap keymap = myKeymapSelector.getSelectedKeymap(); + Keymap keymap = myManager.getSelectedKeymap(); return keymap == null ? null : keymap.getShortcuts(actionId); } @@ -496,7 +499,7 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi String actionId = myActionsTree.getSelectedActionId(); if (actionId == null) return; - Keymap selectedKeymap = myKeymapSelector.getSelectedKeymap(); + Keymap selectedKeymap = myManager.getSelectedKeymap(); if (selectedKeymap == null) return; DefaultActionGroup group = createEditActionGroup(actionId, selectedKeymap); @@ -566,7 +569,7 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi group.add(new DumbAwareAction("Remove " + KeymapUtil.getShortcutText(shortcut)) { @Override public void actionPerformed(@NotNull AnActionEvent e) { - Keymap keymap = myKeymapSelector.getMutableKeymap(selectedKeymap); + Keymap keymap = myManager.getMutableKeymap(selectedKeymap); keymap.removeShortcut(actionId, shortcut); if (StringUtil.startsWithChar(actionId, '$')) { keymap.removeShortcut(KeyMapBundle.message("editor.shortcut", actionId.substring(1)), shortcut); @@ -587,12 +590,12 @@ public class KeymapPanel extends JPanel implements SearchableConfigurable, Confi }); } } - if (myKeymapSelector.canResetActionInKeymap(selectedKeymap, actionId)) { + if (myManager.canResetActionInKeymap(selectedKeymap, actionId)) { group.add(new Separator()); group.add(new DumbAwareAction("Reset Shortcuts") { @Override public void actionPerformed(@NotNull AnActionEvent event) { - myKeymapSelector.resetActionInKeymap(selectedKeymap, actionId); + myManager.resetActionInKeymap(selectedKeymap, actionId); repaintLists(); } }); diff --git a/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSchemeManager.java b/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSchemeManager.java index 08116cf5a1fa..57de16800b57 100644 --- a/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSchemeManager.java +++ b/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSchemeManager.java @@ -15,20 +15,27 @@ */ package com.intellij.openapi.keymap.impl.ui; +import com.intellij.application.options.schemes.AbstractSchemeActions; +import com.intellij.application.options.schemes.SchemesModel; import com.intellij.openapi.keymap.Keymap; import com.intellij.openapi.keymap.KeymapManager; import com.intellij.openapi.keymap.impl.KeymapManagerImpl; import com.intellij.openapi.util.Condition; import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; import java.util.ArrayList; import java.util.Comparator; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Objects; +import java.util.function.Consumer; import java.util.function.Predicate; +import static com.intellij.openapi.keymap.KeyMapBundle.message; import static com.intellij.openapi.util.SystemInfo.isMac; +import static com.intellij.openapi.util.text.StringUtil.isEmptyOrSpaces; import static com.intellij.openapi.util.text.StringUtil.naturalCompare; import static java.util.stream.Collectors.toList; @@ -37,62 +44,180 @@ import static java.util.stream.Collectors.toList; * * @author Sergey.Malenkov */ -final class KeymapSchemeManager { +final class KeymapSchemeManager extends AbstractSchemeActions implements SchemesModel { private static final Condition FILTER = keymap -> !isMac || !KeymapManager.DEFAULT_IDEA_KEYMAP.equals(keymap.getName()); private final ArrayList list = new ArrayList<>(); + private final KeymapSelector selector; + + KeymapSchemeManager(KeymapSelector selector) { + super(selector); + this.selector = selector; + } + + Keymap getSelectedKeymap() { + KeymapScheme scheme = selector.getSelectedScheme(); + return scheme == null ? null : scheme.getCurrent(); + } + + Keymap getMutableKeymap(Keymap keymap) { + KeymapScheme scheme = find(keymap); + if (scheme == null) return null; + if (scheme.isMutable()) return scheme.getMutable(); + + String name = message("new.keymap.name", keymap.getPresentableName()); + //noinspection ForLoopThatDoesntUseLoopVariable + for (int i = 1; containsScheme(name, false); i++) { + name = message("new.indexed.keymap.name", keymap.getPresentableName(), i); + } + return copyScheme(scheme, name).getMutable(); + } + + void visitMutableKeymaps(Consumer consumer) { + for (KeymapScheme scheme : list) { + if (scheme.isMutable()) { + consumer.accept(scheme.getMutable()); + } + } + } + + @Override + protected Class getSchemeType() { + return KeymapScheme.class; + } + + @Override + protected void onSchemeChanged(@Nullable KeymapScheme scheme) { + selector.notifyConsumer(scheme); + } + + @Override + public boolean isProjectScheme(@NotNull KeymapScheme scheme) { + return false; + } + + @Override + public boolean canDuplicateScheme(@NotNull KeymapScheme scheme) { + return true; + } + + @Override + protected void duplicateScheme(@NotNull KeymapScheme parent, @NotNull String name) { + copyScheme(parent, name); + } + + @NotNull + private KeymapScheme copyScheme(@NotNull KeymapScheme parent, @NotNull String name) { + KeymapScheme scheme = parent.copy(name); + list.add(scheme); + selector.selectKeymap(scheme, true); + return scheme; + } + + @Override + public boolean canDeleteScheme(@NotNull KeymapScheme scheme) { + return scheme.isMutable(); + } + + @Override + public void removeScheme(@NotNull KeymapScheme scheme) { + list.remove(scheme); + selector.selectKeymap(getSchemeToSelect(scheme.getParent()), true); + } + + @Override + public boolean canRenameScheme(@NotNull KeymapScheme scheme) { + return scheme.isMutable(); + } + + @Override + protected void renameScheme(@NotNull KeymapScheme scheme, @NotNull String name) { + scheme.setName(name); + selector.selectKeymap(scheme, true); + } + + @Override + public boolean containsScheme(@NotNull String name, boolean projectScheme) { + return null != find(scheme -> scheme.contains(name)); + } + + @Override + public boolean differsFromDefault(@NotNull KeymapScheme scheme) { + return scheme.canReset(); + } + + @Override + public boolean canResetScheme(@NotNull KeymapScheme scheme) { + return scheme.isMutable(); + } + + @Override + protected void resetScheme(@NotNull KeymapScheme scheme) { + scheme.reset(); + selector.selectKeymap(scheme, true); + } + + boolean canResetActionInKeymap(Keymap mutable, String actionId) { + KeymapScheme scheme = find(mutable); + return scheme != null && scheme.canReset(actionId); + } + + void resetActionInKeymap(Keymap mutable, String actionId) { + KeymapScheme scheme = find(mutable); + if (scheme == null) return; + scheme.reset(actionId); + selector.selectKeymap(scheme, false); + } + + private KeymapScheme find(Keymap keymap) { + return keymap == null ? null : find(scheme -> scheme.contains(keymap)); + } /** * @param predicate a predicate to test a scheme * @return a first scheme that belongs to the specified predicate, or {@code null} */ - KeymapScheme find(@NotNull Predicate predicate) { + private KeymapScheme find(@NotNull Predicate predicate) { for (KeymapScheme scheme : list) { if (predicate.test(scheme)) return scheme; } return null; } - /** - * @param scheme a scheme to add into the list of schemes - * @return the same scheme to select - */ - KeymapScheme add(@NotNull KeymapScheme scheme) { - list.add(scheme); - return scheme; - } - - /** - * @param scheme a scheme to remove from the list of schemes - * @return a scheme to select - */ - KeymapScheme remove(@NotNull KeymapScheme scheme) { - list.remove(scheme); - return getSchemeToSelect(scheme.getParent()); - } - /** * Initializes a list of schemes from loaded keymaps. * - * @return a scheme to select - * @see KeymapSelector#reset() + * @see KeymapPanel#reset() */ - KeymapScheme reset() { + void reset() { list.clear(); getKeymaps().forEach(keymap -> list.add(new KeymapScheme(keymap))); - return getSchemeToSelect(null); + selector.selectKeymap(getSchemeToSelect(null), true); } /** - * @param selected a selected scheme to activate corresponding keymap - * @return the same scheme to select - * @see KeymapSelector#apply() + * Applies a changes in the internal list of schemes. + * + * @return the error message if changes cannot be applied + * @see KeymapPanel#apply() */ - KeymapScheme apply(KeymapScheme selected) { + String apply() { + HashSet set = new HashSet<>(); + for (KeymapScheme scheme : list) { + String name = scheme.getName(); + if (isEmptyOrSpaces(name)) { + return message("configuration.all.keymaps.should.have.non.empty.names.error.message"); + } + if (!set.add(name)) { + return message("configuration.all.keymaps.should.have.unique.names.error.message"); + } + } + KeymapScheme selected = selector.getSelectedScheme(); Keymap active = selected == null ? null : selected.getOriginal(); List keymaps = list.stream().map(scheme -> scheme.apply()).collect(toList()); KeymapManagerImpl manager = (KeymapManagerImpl)KeymapManager.getInstance(); manager.setKeymaps(keymaps, active, FILTER); - return selected; + selector.notifyConsumer(selected); + return null; } /** @@ -124,11 +249,11 @@ final class KeymapSchemeManager { } /** - * @param selected a selected scheme to test * @return {@code true} if the current list of schemes differs from the list of loaded keymaps - * @see KeymapSelector#isModified() + * @see KeymapPanel#isModified() */ - boolean isModified(KeymapScheme selected) { + boolean isModified() { + KeymapScheme selected = selector.getSelectedScheme(); Keymap active = selected == null ? null : selected.getOriginal(); if (!Objects.equals(active, KeymapManager.getInstance().getActiveKeymap())) return true; @@ -140,8 +265,8 @@ final class KeymapSchemeManager { return keymaps.hasNext() || schemes.hasNext(); } - List getSchemes(boolean sorted) { - if (sorted) list.sort(COMPARATOR); + List getSchemes() { + list.sort(COMPARATOR); return list; } diff --git a/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSelector.java b/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSelector.java index c24abe18cb3b..8f6eb9dd50df 100644 --- a/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSelector.java +++ b/platform/platform-impl/src/com/intellij/openapi/keymap/impl/ui/KeymapSelector.java @@ -23,18 +23,15 @@ import com.intellij.openapi.ui.MessageType; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.util.HashSet; import java.util.function.Consumer; -import static com.intellij.openapi.actionSystem.impl.ActionToolbarImpl.updateAllToolbarsImmediately; import static com.intellij.openapi.keymap.KeyMapBundle.message; -import static com.intellij.openapi.util.text.StringUtil.isEmptyOrSpaces; /** * @author Sergey.Malenkov */ -final class KeymapSelector extends SimpleSchemesPanel implements SchemesModel { - private final KeymapSchemeManager manager = new KeymapSchemeManager(); +final class KeymapSelector extends SimpleSchemesPanel { + private KeymapSchemeManager manager; private final Consumer consumer; private String messageReplacement; private boolean messageShown; @@ -44,87 +41,15 @@ final class KeymapSelector extends SimpleSchemesPanel implements S this.consumer = consumer; } - /** - * @see KeymapPanel#reset() - */ - void reset() { - selectKeymap(manager.reset(), true); - } - - /** - * @see KeymapPanel#apply() - */ - String apply() { - HashSet set = new HashSet<>(); - for (KeymapScheme scheme : manager.getSchemes(false)) { - String name = scheme.getName(); - if (isEmptyOrSpaces(name)) { - return message("configuration.all.keymaps.should.have.non.empty.names.error.message"); - } - if (!set.add(name)) { - return message("configuration.all.keymaps.should.have.unique.names.error.message"); - } - } - notifyConsumer(manager.apply(getSelectedScheme())); - updateAllToolbarsImmediately(); - return null; - } - - /** - * @see KeymapPanel#isModified() - */ - boolean isModified() { - return manager.isModified(getSelectedScheme()); - } - - void visitMutableKeymaps(Consumer consumer) { - for (KeymapScheme scheme : manager.getSchemes(false)) { - if (scheme.isMutable()) { - consumer.accept(scheme.getMutable()); - } - } - } - - Keymap getSelectedKeymap() { - KeymapScheme scheme = getSelectedScheme(); - return scheme == null ? null : scheme.getCurrent(); - } - - Keymap getMutableKeymap(Keymap keymap) { - KeymapScheme scheme = find(keymap); - if (scheme == null) return null; - if (scheme.isMutable()) return scheme.getMutable(); - - String name = message("new.keymap.name", keymap.getPresentableName()); - //noinspection ForLoopThatDoesntUseLoopVariable - for (int i = 1; containsScheme(name, false); i++) { - name = message("new.indexed.keymap.name", keymap.getPresentableName(), i); - } - KeymapScheme copy = manager.add(scheme.copy(name)); - selectKeymap(copy, true); - return copy.getMutable(); - } - - boolean canResetActionInKeymap(Keymap mutable, String actionId) { - KeymapScheme scheme = find(mutable); - return scheme != null && scheme.canReset(actionId); - } - - void resetActionInKeymap(Keymap mutable, String actionId) { - KeymapScheme scheme = find(mutable); - if (scheme == null) return; - scheme.reset(actionId); - selectKeymap(scheme, false); - } - - private KeymapScheme find(Keymap keymap) { - return keymap == null ? null : manager.find(scheme -> scheme.contains(keymap)); + public KeymapSchemeManager getManager() { + if (manager == null) manager = new KeymapSchemeManager(this); + return manager; } @NotNull @Override public SchemesModel getModel() { - return this; + return getManager(); } @Override @@ -134,73 +59,11 @@ final class KeymapSelector extends SimpleSchemesPanel implements S @Override protected AbstractSchemeActions createSchemeActions() { - return new AbstractSchemeActions(this) { - @Override - protected Class getSchemeType() { - return KeymapScheme.class; - } - - @Override - protected void onSchemeChanged(@Nullable KeymapScheme scheme) { - if (!internal) notifyConsumer(scheme); - } - - @Override - protected void resetScheme(@NotNull KeymapScheme scheme) { - scheme.reset(); - selectKeymap(scheme, true); - } - - @Override - protected void renameScheme(@NotNull KeymapScheme scheme, @NotNull String name) { - scheme.setName(name); - selectKeymap(scheme, true); - } - - @Override - protected void duplicateScheme(@NotNull KeymapScheme parent, @NotNull String name) { - selectKeymap(manager.add(parent.copy(name)), true); - } - }; + return getManager(); } @Override - public void removeScheme(@NotNull KeymapScheme scheme) { - selectKeymap(manager.remove(scheme), true); - } - - @Override - public boolean canResetScheme(@NotNull KeymapScheme scheme) { - return scheme.isMutable(); - } - - @Override - public boolean canRenameScheme(@NotNull KeymapScheme scheme) { - return scheme.isMutable(); - } - - @Override - public boolean canDuplicateScheme(@NotNull KeymapScheme scheme) { - return true; - } - - @Override - public boolean canDeleteScheme(@NotNull KeymapScheme scheme) { - return scheme.isMutable(); - } - - @Override - public boolean containsScheme(@NotNull String name, boolean isProjectScheme) { - return null != manager.find(scheme -> scheme.contains(name)); - } - - @Override - public boolean isProjectScheme(@NotNull KeymapScheme scheme) { - return false; - } - - @Override - public int getIndent(@NotNull KeymapScheme scheme) { + protected int getIndent(@NotNull KeymapScheme scheme) { return scheme.isMutable() ? 1 : 0; } @@ -219,11 +82,6 @@ final class KeymapSelector extends SimpleSchemesPanel implements S return true; } - @Override - public boolean differsFromDefault(@NotNull KeymapScheme scheme) { - return scheme.canReset(); - } - @Override public void showMessage(@Nullable String message, @NotNull MessageType messageType) { messageShown = true; @@ -236,7 +94,9 @@ final class KeymapSelector extends SimpleSchemesPanel implements S super.showMessage(messageReplacement, MessageType.INFO); } - private void notifyConsumer(KeymapScheme scheme) { + void notifyConsumer(KeymapScheme scheme) { + if (internal) return; + Keymap keymap = scheme == null ? null : scheme.getParent(); messageReplacement = keymap == null ? null : message("based.on.keymap.label", keymap.getPresentableName()); if (!messageShown) clearMessage(); @@ -244,10 +104,10 @@ final class KeymapSelector extends SimpleSchemesPanel implements S consumer.accept(scheme == null ? null : scheme.getCurrent()); } - private void selectKeymap(KeymapScheme scheme, boolean reset) { + void selectKeymap(KeymapScheme scheme, boolean reset) { try { internal = true; - if (reset) resetSchemes(manager.getSchemes(true)); + if (reset) resetSchemes(getManager().getSchemes()); if (scheme != null) selectScheme(scheme); } finally {