From 390b825e915b0c37825d2f3941197bd3abe3394b Mon Sep 17 00:00:00 2001 From: Tor Norbye Date: Fri, 30 Sep 2011 11:45:26 -0700 Subject: [PATCH] Refactoring quick fix adjustments to caret and selection handling. DO NOT MERGE This changeset changes the logic in the refactoring quickfix (which adds the Android refactorings based on the current caret position, lexical context and selection). It now allows some of the refactorings to be run when there is a selection (this fixes "20393: Extract string functionality in Android XML files"), and it makes other refactorings work when there is no selection (it implicitly selects the surrounding element). It also ensures that the Extract Style refactoring won't be listed at the top if the attribute under the cursor is not a stylable attribute, and it makes some other adjustments to the proposal order. Change-Id: I1ca305d9c66ae4eb6cd9a4f45f6803bb2444abdb --- eclipse/dictionary.txt | 1 + .../refactoring/ExtractStyleRefactoring.java | 27 +++- .../layout/refactoring/RefactoringAssistant.java | 144 +++++++++++++-------- .../refactoring/RefactoringAssistantTest.java | 3 +- .../testdata/sample1a-expected-assistant2.txt | 2 +- .../testdata/sample1a-expected-assistant3.txt | 5 + .../testdata/sample1a-expected-assistant4.txt | 2 +- 7 files changed, 119 insertions(+), 65 deletions(-) diff --git a/eclipse/dictionary.txt b/eclipse/dictionary.txt index 65a89b37b..286b5e778 100644 --- a/eclipse/dictionary.txt +++ b/eclipse/dictionary.txt @@ -193,6 +193,7 @@ rect redo refactor refactoring +refactorings regex regexp regexps diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/ExtractStyleRefactoring.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/ExtractStyleRefactoring.java index 1fc26b500..a03bd6bf3 100644 --- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/ExtractStyleRefactoring.java +++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/ExtractStyleRefactoring.java @@ -250,13 +250,7 @@ public class ExtractStyleRefactoring extends VisualRefactoring { Attr attribute = (Attr) attributeMap.item(i); String name = attribute.getLocalName(); - if (name == null || name.equals(ATTR_ID) || name.startsWith(ATTR_STYLE) - || (name.startsWith(ATTR_LAYOUT_PREFIX) && - !name.startsWith(ATTR_LAYOUT_MARGIN)) - || name.equals(ATTR_TEXT) - || name.equals(ATTR_HINT) - || name.equals(ATTR_SRC) - || name.equals(ATTR_ON_CLICK)) { + if (!isStylableAttribute(name)) { // Don't offer to extract attributes that don't make sense in // styles (like "id" or "style"), or attributes that the user // probably does not want to define in styles (like layout @@ -297,6 +291,25 @@ public class ExtractStyleRefactoring extends VisualRefactoring { return Pair.of(mAvailableAttributes, withinSelection); } + /** + * Returns whether the given local attribute name is one the style wizard + * should present as a selectable attribute to be extracted. + * + * @param name the attribute name, not including a namespace prefix + * @return true if the name is one that the user can extract + */ + public static boolean isStylableAttribute(String name) { + return !(name == null + || name.equals(ATTR_ID) + || name.startsWith(ATTR_STYLE) + || (name.startsWith(ATTR_LAYOUT_PREFIX) && + !name.startsWith(ATTR_LAYOUT_MARGIN)) + || name.equals(ATTR_TEXT) + || name.equals(ATTR_HINT) + || name.equals(ATTR_SRC) + || name.equals(ATTR_ON_CLICK)); + } + IFile getStyleFile(IProject project) { return project.getFile(new Path(FD_RESOURCES + WS_SEP + FD_RES_VALUES + WS_SEP + mStyleFileName)); diff --git a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistant.java b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistant.java index 38a40f8ed..8d0ea33c3 100644 --- a/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistant.java +++ b/eclipse/plugins/com.android.ide.eclipse.adt/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistant.java @@ -19,12 +19,14 @@ package com.android.ide.eclipse.adt.internal.editors.layout.refactoring; import com.android.ide.eclipse.adt.AdtPlugin; import com.android.ide.eclipse.adt.internal.editors.AndroidXmlEditor; import com.android.ide.eclipse.adt.internal.editors.layout.LayoutEditor; +import com.android.ide.eclipse.adt.internal.editors.layout.gle2.DomUtilities; import com.android.ide.eclipse.adt.internal.refactorings.extractstring.ExtractStringRefactoring; import com.android.ide.eclipse.adt.internal.refactorings.extractstring.ExtractStringWizard; import org.eclipse.core.resources.IFile; import org.eclipse.jface.text.IDocument; import org.eclipse.jface.text.ITextSelection; +import org.eclipse.jface.text.TextSelection; import org.eclipse.jface.text.contentassist.ICompletionProposal; import org.eclipse.jface.text.contentassist.IContextInformation; import org.eclipse.jface.text.quickassist.IQuickAssistInvocationContext; @@ -41,11 +43,16 @@ import org.eclipse.swt.graphics.Point; import org.eclipse.ui.IWorkbenchWindow; import org.eclipse.ui.PlatformUI; import org.eclipse.wst.sse.core.internal.provisional.IStructuredModel; +import org.eclipse.wst.sse.core.internal.provisional.IndexedRegion; import org.eclipse.wst.sse.core.internal.provisional.text.IStructuredDocument; import org.eclipse.wst.sse.core.internal.provisional.text.IStructuredDocumentRegion; import org.eclipse.wst.sse.core.internal.provisional.text.ITextRegion; import org.eclipse.wst.sse.ui.StructuredTextEditor; import org.eclipse.wst.xml.core.internal.regions.DOMRegionContext; +import org.w3c.dom.Node; + +import java.util.ArrayList; +import java.util.List; /** * QuickAssistProcessor which helps invoke refactoring operations on text elements. @@ -53,6 +60,9 @@ import org.eclipse.wst.xml.core.internal.regions.DOMRegionContext; @SuppressWarnings("restriction") // XML model public class RefactoringAssistant implements IQuickAssistProcessor { + /** + * Creates a new {@link RefactoringAssistant} + */ public RefactoringAssistant() { } @@ -80,8 +90,10 @@ public class RefactoringAssistant implements IQuickAssistProcessor { // operations) or a value (for the extract include refactoring) boolean isValue = false; + boolean isReferenceValue = false; boolean isTagName = false; boolean isAttributeName = false; + boolean isStylableAttribute = false; IStructuredModel model = null; try { model = xmlEditor.getModelForRead(); @@ -94,16 +106,26 @@ public class RefactoringAssistant implements IQuickAssistProcessor { String value = region.getText(subRegion); // Only extract values that aren't already resources // (and value includes leading ' or ") - if (!value.startsWith("'@") && !value.startsWith("\"@")) { //$NON-NLS-1$ //$NON-NLS-2$ - isValue = true; + isValue = true; + if (value.startsWith("'@") || value.startsWith("\"@")) { //$NON-NLS-1$ //$NON-NLS-2$ + isReferenceValue = true; } } else if (type.equals(DOMRegionContext.XML_TAG_NAME) || type.equals(DOMRegionContext.XML_TAG_OPEN) || type.equals(DOMRegionContext.XML_TAG_CLOSE)) { isTagName = true; - } else if (type.equals(DOMRegionContext.XML_TAG_ATTRIBUTE_NAME) - || type.equals(DOMRegionContext.XML_TAG_ATTRIBUTE_EQUALS)) { + } else if (type.equals(DOMRegionContext.XML_TAG_ATTRIBUTE_NAME) ) { + isAttributeName = true; + String name = region.getText(subRegion); + int index = name.indexOf(':'); + if (index != -1) { + name = name.substring(index + 1); + } + isStylableAttribute = ExtractStyleRefactoring.isStylableAttribute(name); + } else if (type.equals(DOMRegionContext.XML_TAG_ATTRIBUTE_EQUALS)) { + // On the edge of an attribute name and an attribute value isAttributeName = true; + isStylableAttribute = true; } } } finally { @@ -112,70 +134,84 @@ public class RefactoringAssistant implements IQuickAssistProcessor { } } - if (isValue || isTagName || isAttributeName) { + List proposals = new ArrayList(); + if (isTagName || isAttributeName || isValue) { StructuredTextEditor structuredEditor = xmlEditor.getStructuredTextEditor(); ISelectionProvider provider = structuredEditor.getSelectionProvider(); ISelection selection = provider.getSelection(); if (selection instanceof ITextSelection) { ITextSelection textSelection = (ITextSelection) selection; - // These operations currently do not work on ranges - if (textSelection.getLength() > 0) { - // ...except for Extract Style where the actual attributes overlapping - // the selection is going to be the set of eligible attributes - if (isAttributeName && xmlEditor instanceof LayoutEditor) { - LayoutEditor editor = (LayoutEditor) xmlEditor; - return new ICompletionProposal[] { - new RefactoringProposal(editor, - new ExtractStyleRefactoring(file, editor, textSelection, null)) - }; + ITextSelection originalSelection = textSelection; + + // Most of the visual refactorings do not work on text ranges + // ...except for Extract Style where the actual attributes overlapping + // the selection is going to be the set of eligible attributes + boolean selectionOkay = false; + + if (textSelection.getLength() == 0 && !isValue) { + selectionOkay = true; + ISourceViewer textViewer = xmlEditor.getStructuredSourceViewer(); + int caretOffset = textViewer.getTextWidget().getCaretOffset(); + if (caretOffset >= 0) { + Node node = DomUtilities.getNode(textViewer.getDocument(), caretOffset); + if (node instanceof IndexedRegion) { + IndexedRegion region = (IndexedRegion) node; + int startOffset = region.getStartOffset(); + int length = region.getEndOffset() - region.getStartOffset(); + textSelection = new TextSelection(startOffset, length); + } } - return null; } - if (isAttributeName && xmlEditor instanceof LayoutEditor) { + if (isValue && !isReferenceValue) { + proposals.add(new RefactoringProposal(xmlEditor, + new ExtractStringRefactoring(file, xmlEditor, textSelection))); + } + + if (xmlEditor instanceof LayoutEditor) { LayoutEditor editor = (LayoutEditor) xmlEditor; - return new ICompletionProposal[] { - new RefactoringProposal(editor, - new ExtractStyleRefactoring(file, editor, textSelection, null)), - }; - } else if (isValue) { - if (xmlEditor instanceof LayoutEditor) { - LayoutEditor editor = (LayoutEditor) xmlEditor; - return new ICompletionProposal[] { - new RefactoringProposal(xmlEditor, - new ExtractStringRefactoring(file, xmlEditor, - textSelection)), - new RefactoringProposal(editor, - new ExtractStyleRefactoring(file, editor, - textSelection, null)), - }; - } else { - return new ICompletionProposal[] { - new RefactoringProposal(xmlEditor, - new ExtractStringRefactoring(file, xmlEditor, textSelection)) - }; + + boolean showStyleFirst = isValue || (isAttributeName && isStylableAttribute); + if (showStyleFirst) { + proposals.add(new RefactoringProposal(editor, + new ExtractStyleRefactoring(file, editor, originalSelection, + null))); } - } else if (xmlEditor instanceof LayoutEditor) { - LayoutEditor editor = (LayoutEditor) xmlEditor; - return new ICompletionProposal[] { - new RefactoringProposal(editor, - new WrapInRefactoring(file, editor, textSelection, null)), - new RefactoringProposal(editor, - new UnwrapRefactoring(file, editor, textSelection, null)), - new RefactoringProposal(editor, - new ChangeViewRefactoring(file, editor, textSelection, null)), - new RefactoringProposal(editor, - new ChangeLayoutRefactoring(file, editor, textSelection, null)), - new RefactoringProposal(editor, - new ExtractStyleRefactoring(file, editor, textSelection, null)), - new RefactoringProposal(editor, - new ExtractIncludeRefactoring(file, editor, textSelection, null)), - }; + + if (selectionOkay) { + proposals.add(new RefactoringProposal(editor, + new WrapInRefactoring(file, editor, textSelection, null))); + proposals.add(new RefactoringProposal(editor, + new UnwrapRefactoring(file, editor, textSelection, null))); + proposals.add(new RefactoringProposal(editor, + new ChangeViewRefactoring(file, editor, textSelection, null))); + proposals.add(new RefactoringProposal(editor, + new ChangeLayoutRefactoring(file, editor, textSelection, null))); + } + + // Extract Include must always have an actual block to be extracted + if (textSelection.getLength() > 0) { + proposals.add(new RefactoringProposal(editor, + new ExtractIncludeRefactoring(file, editor, textSelection, null))); + } + + // If it's not a value or attribute name, don't place it on top + if (!showStyleFirst) { + proposals.add(new RefactoringProposal(editor, + new ExtractStyleRefactoring(file, editor, originalSelection, + null))); + } + } } } - return null; + + if (proposals.size() == 0) { + return null; + } else { + return proposals.toArray(new ICompletionProposal[proposals.size()]); + } } public String getErrorMessage() { diff --git a/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistantTest.java b/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistantTest.java index 498f65aca..c107a3092 100644 --- a/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistantTest.java +++ b/eclipse/plugins/com.android.ide.eclipse.tests/src/com/android/ide/eclipse/adt/internal/editors/layout/refactoring/RefactoringAssistantTest.java @@ -44,13 +44,12 @@ public class RefactoringAssistantTest extends AdtProjectTest { } public void testAssistant3() throws Exception { - // Negative test: ensure that we don't get completion items in other parts of the XML checkFixes("sample1a.xml", "