New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ui] XPath AutoComplete #1182
[ui] XPath AutoComplete #1182
Changes from 9 commits
7a6e40c
eddb9be
6a88b5d
0a85e86
2aaa468
67da35b
7f99c20
9770c88
44c464c
48c4e2a
2af2aaf
51e0180
cbf8e6a
a7113b7
a5c8308
05a733d
a3d8a80
0447fd3
3ec5136
da24020
23d0005
ee2678a
1e839a6
4bb34b1
32136e0
2db426f
7e07208
39ec32e
978d77a
9c71a70
edeefa7
118ee1d
4b8dba8
7f3c93d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ | |
import java.io.IOException; | ||
import java.net.URL; | ||
import java.time.Duration; | ||
import java.util.ArrayList; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.Objects; | ||
|
@@ -28,6 +29,7 @@ | |
import net.sourceforge.pmd.util.fxdesigner.model.ObservableXPathRuleBuilder; | ||
import net.sourceforge.pmd.util.fxdesigner.model.XPathEvaluationException; | ||
import net.sourceforge.pmd.util.fxdesigner.model.XPathEvaluator; | ||
import net.sourceforge.pmd.util.fxdesigner.model.XPathSuggestions; | ||
import net.sourceforge.pmd.util.fxdesigner.util.DesignerUtil; | ||
import net.sourceforge.pmd.util.fxdesigner.util.beans.SettingsOwner; | ||
import net.sourceforge.pmd.util.fxdesigner.util.beans.SettingsPersistenceUtil.PersistentProperty; | ||
|
@@ -50,6 +52,8 @@ | |
import javafx.scene.control.MenuItem; | ||
import javafx.scene.control.TextArea; | ||
import javafx.scene.control.TitledPane; | ||
import javafx.scene.input.KeyCode; | ||
import javafx.scene.input.KeyEvent; | ||
import javafx.scene.input.MouseButton; | ||
import javafx.scene.input.MouseEvent; | ||
import javafx.stage.Modality; | ||
|
@@ -84,6 +88,8 @@ public class XPathPanelController implements Initializable, SettingsOwner { | |
@SuppressWarnings("PMD.SingularField") | ||
private ChoiceBox<String> xpathVersionChoiceBox; | ||
|
||
private ContextMenu autoCompletePopup; | ||
|
||
|
||
public XPathPanelController(DesignerRoot owner, MainDesignerController mainController) { | ||
this.designerRoot = owner; | ||
|
@@ -112,9 +118,66 @@ public void initialize(URL location, ResourceBundle resources) { | |
// Reevaluate XPath anytime the expression or the XPath version changes | ||
.or(xpathVersionProperty().changes()) | ||
.subscribe(tick -> parent.refreshXPathResults()); | ||
|
||
xpathExpressionArea.plainTextChanges() | ||
.filter(t -> (t.getInserted().matches("[a-zA-Z]")) || t.getInserted().matches("/")) | ||
.subscribe(t -> { | ||
try { | ||
if (t.getInserted().equals("/")) { | ||
autoComplete(xpathExpressionArea.getText().substring(xpathExpressionArea.getText().indexOf("/"))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we interested only in the first slash of the expression? |
||
} else if (!t.getInserted().equals("/")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if A This can be reduced to a simple if / else |
||
autoComplete(xpathExpressionArea.getText().substring(xpathExpressionArea.getText().lastIndexOf("/"))); | ||
} | ||
} catch (IOException e) { | ||
e.printStackTrace(); | ||
} catch (ClassNotFoundException e) { | ||
e.printStackTrace(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remember to work these exceptions out eventually |
||
} | ||
}); | ||
} | ||
|
||
private void autoComplete(String s) throws IOException, ClassNotFoundException { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Give a more descriptive name to the parameter (eg input or whatever but |
||
|
||
autoCompletePopup = new ContextMenu(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You still create a new context menu each time! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will make the update, sorry for the confusion before |
||
List<MenuItem> resultToDisplay = new ArrayList<>(); | ||
|
||
String language = parent.getLanguageVersion().getName().replaceAll("[0-9]", "").replaceAll("//s", "").toLowerCase().trim(); | ||
XPathSuggestions xPathSuggestions = new XPathSuggestions("net.sourceforge.pmd.lang." + language + ".ast"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. given that |
||
|
||
List<String> suggestions = xPathSuggestions.getXPathSuggestions(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The suggestions depend on the input string entered by the user. Add a parameter to your method which contains that and retrieve it from the change stream There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The XPathSuggestions should encapsulate the algorithm to find suggestions from the input string. The call should look like Then you could for example stream the list, map it to menu items, and take only the first 5 to not overwhelm the user. |
||
|
||
for (String s1 : suggestions) { | ||
if (s1.contains(s.replace("/", "").trim())) { | ||
MenuItem m = new MenuItem(s1); | ||
if (!resultToDisplay.contains(m)) { | ||
resultToDisplay.add(m); | ||
} | ||
} | ||
} | ||
|
||
//TODO: Work on the implementation of the Result to be selected and added to the Code Area | ||
autoCompletePopup.getItems().addAll(resultToDisplay); | ||
|
||
if (xpathExpressionArea.getText().length() > 0) { | ||
|
||
xpathExpressionArea.addEventHandler(KeyEvent.KEY_TYPED, t -> { | ||
if (t.getCode() != KeyCode.ESCAPE) { | ||
xpathExpressionArea.setContextMenu(autoCompletePopup); | ||
} | ||
}); | ||
|
||
xpathExpressionArea.addEventHandler(KeyEvent.KEY_PRESSED, t -> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the handler should probably belong to the popup |
||
if (t.getCode() == KeyCode.ESCAPE) { | ||
autoCompletePopup.hide(); | ||
} | ||
}); | ||
|
||
|
||
} | ||
} | ||
|
||
|
||
|
||
private void initGenerateXPathFromStackTrace() { | ||
|
||
ContextMenu menu = new ContextMenu(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
/** | ||
* BSD-style license; for more info see http://pmd.sourceforge.net/license.html | ||
*/ | ||
|
||
package net.sourceforge.pmd.util.fxdesigner.model; | ||
|
||
import java.io.File; | ||
import java.io.IOException; | ||
import java.net.URL; | ||
import java.util.ArrayList; | ||
import java.util.Arrays; | ||
import java.util.Enumeration; | ||
import java.util.List; | ||
|
||
|
||
public class XPathSuggestions { | ||
private List<String> xPathSuggestions = new ArrayList<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's the difference between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make the field final and name it better There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make the field final. And a better name would be "availableNodeNames" or something like that |
||
String packagename; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if this is intented to be package-private, document it. If it's not, please change it to proper visibility (private?) |
||
|
||
public XPathSuggestions(String packageName) { | ||
this.packagename = packageName; | ||
} | ||
|
||
|
||
public List<String> getXPathSuggestions() throws IOException, ClassNotFoundException { | ||
return evaluateXpathSuggestions(createList(getClasses(packagename))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So you reevaluate everything each time. Keep in mind your list of all node names only be evaluated once, eg in the constructor. Also, throwing checked exceptions here seems really sloppy. The exceptions should be taken care of when the list is created in the constructor, and should be recovered from in this class. Clients of this class have no idea how to recover from them |
||
} | ||
|
||
private List<String> createList(Class[] classArray) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please use more descriptive names and add documentation comments. |
||
List<Class> fileNameList = Arrays.asList(classArray); | ||
List<String> foo = new ArrayList<>(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "foo"? |
||
|
||
for (Class c : fileNameList) { | ||
|
||
if (c.getName().contains("AST")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Abstracting that into a function ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. c.getSimpleName().startsWith("AST") |
||
foo.add(c.getName()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're looking for c.getSimpleName.substring("AST".length()) |
||
} | ||
} | ||
|
||
return foo; | ||
} | ||
|
||
private static Class[] getClasses(String packageName) throws ClassNotFoundException, IOException { | ||
|
||
ClassLoader classLoader = Thread.currentThread().getContextClassLoader(); | ||
assert classLoader != null; | ||
|
||
String path = packageName.replace('.', '/'); | ||
Enumeration<URL> resources = classLoader.getResources(path); | ||
|
||
List<File> dirs = new ArrayList<>(); | ||
|
||
while (resources.hasMoreElements()) { | ||
URL resource = resources.nextElement(); | ||
dirs.add(new File(resource.getFile())); | ||
} | ||
|
||
ArrayList<Class> classes = new ArrayList<>(); | ||
|
||
for (File directory : dirs) { | ||
classes.addAll(findClasses(directory, packageName)); | ||
} | ||
|
||
return classes.toArray(new Class[classes.size()]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remember to 0-size arrays when doing |
||
} | ||
|
||
|
||
private static List<Class> findClasses(File directory, String packageName) throws ClassNotFoundException { | ||
|
||
List<Class> classes = new ArrayList<>(); | ||
if (!directory.exists()) { | ||
return classes; | ||
} | ||
|
||
File[] files = directory.listFiles(); | ||
for (File file : files) { | ||
if (file.isDirectory()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need to recurse here for our use case |
||
assert !file.getName().contains("."); | ||
classes.addAll(findClasses(file, packageName + "." + file.getName())); | ||
} else if (file.getName().endsWith(".class")) { | ||
classes.add(Class.forName(packageName + "." + file.getName().substring(0, file.getName().length() - 6))); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this 6 seems rather magical... |
||
} | ||
} | ||
|
||
return classes; | ||
} | ||
|
||
private List<String> evaluateXpathSuggestions(List<String> fileNameList) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see the point of having a separate method, just merge that into the |
||
for (String s : fileNameList) { | ||
//Check if the package name should be hardcoded | ||
xPathSuggestions.add(s.replace("AST", "").replace(".java", "").replace("net.sourceforge.pmd.lang.ast" + ".", "")); | ||
} | ||
return xPathSuggestions; | ||
} | ||
|
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these 2 checks could be merged together in
matches("[a-zA-Z/]")