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

Merged
merged 34 commits into from Jul 11, 2018

Conversation

3 participants
@akshatbahety
Contributor

akshatbahety commented Jun 9, 2018

So the feature is in very early stages as we discussed the basic rudimentary setup is there I have used a basic contains check to look out for the suggestions of XPath.

Working on the UI that's a part I need help around @oowekyala
maybe some guidelines.

akshatbahety added some commits Jun 8, 2018

Start on the autocomplete feature made basic UI upgrade to show the p…
…opup items for suggestions working on positioning the context menu directly under the code
The basic working is there the suggestions are being filtered on the …
…text entered by the user but I think there could be a better way (maybe or maybe my approach is good) need to revamp somethings in the UI @oowekyala need some guidance here.

@akshatbahety akshatbahety changed the title from Feature to XPath AutoComplete Jun 9, 2018

@oowekyala oowekyala changed the title from XPath AutoComplete to [ui] XPath AutoComplete Jun 9, 2018

private List<String> getFileList(final File folder) {

This comment has been minimized.

@oowekyala

oowekyala Jun 9, 2018

Member

This is bound to your particular file system. Explore the package, not a folder. Create your object with the name of the language in the constructor, then construct the name of the language-specific AST package from it and retrieve the classes using this solution https://stackoverflow.com/a/520344/6245827

@@ -113,9 +121,53 @@ 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.richChanges()
.subscribe(t -> autoComplete());

This comment has been minimized.

@oowekyala

oowekyala Jun 9, 2018

Member

This is wrong, we don't display autocomplete every time. Use plainTextChanges() instead and filter the changes which touch a string segment made up of only letters, that is directly after a /. Then pass this segment to the autocomplete method

for (String s : suggestions) {
if (s.contains(list.get(list.size() - 1))) {

This comment has been minimized.

@oowekyala

oowekyala Jun 9, 2018

Member

We aren't necessarily editing the last segment, so you need to bind this to caret position

for (String s : suggestions) {
if (s.contains(list.get(list.size() - 1))) {
MenuItem m = new MenuItem(s);
resultToDisplay.add(m);

This comment has been minimized.

@oowekyala

oowekyala Jun 9, 2018

Member

You never clear the items, so you're just adding more each time

File folder = new File("G:\\pmd\\pmd-java\\src\\main\\java\\net\\sourceforge\\pmd\\lang\\java\\ast\\");
XPathSuggestions xPathSuggestions = new XPathSuggestions(folder);
List<String> suggestions = xPathSuggestions.getXPathSuggestions();

This comment has been minimized.

@oowekyala

oowekyala Jun 9, 2018

Member

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

@akshatbahety akshatbahety force-pushed the akshatbahety:feature branch from b50e095 to 7f99c20 Jun 10, 2018

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 10, 2018

I have pushed some changes as you mentioned @oowekyala but there are still a couple left working on the following
The user input of string to process
I have got the language version working but there is some issue working on it
here is the snippet
parent.getLanguageVersion().getName().replaceAll("[0-9]","").replaceAll("//s","").toLowerCase();
and Working on the caret position issue.

Will be pushing some more changes at night.

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 10, 2018

Solved the language issue will push the commit together with other changes.

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 10, 2018

The Language and String Issue are worked out things seem to be working fine.
Working on the implementation now
So that when user selects a contextmenu text should be added to the code area probably with a forward slash.

@@ -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("/"))

This comment has been minimized.

@jsotuyod

jsotuyod Jun 10, 2018

Member

these 2 checks could be merged together in matches("[a-zA-Z/]")

try {
if (t.getInserted().equals("/")) {
autoComplete(xpathExpressionArea.getText().substring(xpathExpressionArea.getText().indexOf("/")));
} else if (!t.getInserted().equals("/")) {

This comment has been minimized.

@jsotuyod

jsotuyod Jun 10, 2018

Member

if A
else if not A ?

This can be reduced to a simple if / else

.subscribe(t -> {
try {
if (t.getInserted().equals("/")) {
autoComplete(xpathExpressionArea.getText().substring(xpathExpressionArea.getText().indexOf("/")));

This comment has been minimized.

@jsotuyod

jsotuyod Jun 10, 2018

Member

why are we interested only in the first slash of the expression?

} catch (IOException e) {
e.printStackTrace();
} catch (ClassNotFoundException e) {
e.printStackTrace();

This comment has been minimized.

@jsotuyod

jsotuyod Jun 10, 2018

Member

remember to work these exceptions out eventually

public class XPathSuggestions {
private List<String> xPathSuggestions = new ArrayList<>();
String packagename;

This comment has been minimized.

@jsotuyod

jsotuyod Jun 10, 2018

Member

if this is intented to be package-private, document it. If it's not, please change it to proper visibility (private?)

classes.addAll(findClasses(directory, packageName));
}
return classes.toArray(new Class[classes.size()]);

This comment has been minimized.

@jsotuyod

jsotuyod Jun 10, 2018

Member

remember to 0-size arrays when doing toArray(). PMD will enforce this.

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");

This comment has been minimized.

@jsotuyod

jsotuyod Jun 10, 2018

Member

given that XPathSuggestions knows about this package (it filters out on XPathSuggestions .evaluateXpathSuggestions, I'd consider just having the constructor receive the language name, and have the rest of the logic moved there. This way, there is a single class that needs to care about what packages are expected to contain AST nodes for a given language.

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 10, 2018

The feature is working on a basic model of inserting strings into the CodeArea

Some design things are left such as not displaying multiple context menu
displaying the context menu below the carat.

@oowekyala oowekyala self-assigned this Jun 11, 2018

@oowekyala

Still way to go but a good start

try {
if (xpathExpressionArea.getText().contains("/")) {
if (!t.getInserted().equals("/")) {
autoComplete(xpathExpressionArea.getText().substring(xpathExpressionArea.getText().lastIndexOf("/")));

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

This takes all the text after the last /, which could contain eg several predicates, with possibly whitespace that you shouldn't strip (they start with [). Here's the outline of a better procedure

  • if the inserted text contains /, cut out the part between the last / that's within the inserted text and the end of the inserted text
    • else cut out the part between the last / that's before the insertion point (t.getPosition()) and the end of the inserted text
  • continue only if that segment is only letters
  • compute suggestions from it and update the menu.
    • if a suggestion is selected, then replace this text by the selected suggestion (but nothing else).

Edit: you also need to consider removals. Let's do that as the next step.

Give a more descriptive name to t as well

});
}
private void autoComplete(String s) throws IOException, ClassNotFoundException {

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

Give a more descriptive name to the parameter (eg input or whatever but s)

String language = parent.getLanguageVersion().getName().replaceAll("[0-9]", "").replaceAll("//s", "").toLowerCase().trim();
XPathSuggestions xPathSuggestions = new XPathSuggestions(language);
List<String> suggestions = xPathSuggestions.getXPathSuggestions();

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

The XPathSuggestions should encapsulate the algorithm to find suggestions from the input string. The call should look like xpathSuggestions.getSuggestions(input) and return only the suggestions that match the string, the definiton of "x matches s" being defined by that procedure.

Then you could for example stream the list, map it to menu items, and take only the first 5 to not overwhelm the user.

List<String> foo = new ArrayList<>();
for (Class c : fileNameList) {
if (c.getName().contains("AST")) {

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

Abstracting that into a function (isAstNode(Class)) would be better. The filtering misses some things too, for example, it should filter out classes whose names contain a $ symbol (nested classes), and classes that are not concrete.

public List<String> getXPathSuggestions() throws IOException, ClassNotFoundException {
return evaluateXpathSuggestions(createList(getClasses(packagename)));

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

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

return evaluateXpathSuggestions(createList(getClasses(packagename)));
}
private List<String> createList(Class[] classArray) {

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

Please use more descriptive names and add documentation comments.

return classes;
}
private List<String> evaluateXpathSuggestions(List<String> fileNameList) {

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

I don't see the point of having a separate method, just merge that into the createList

private String packagename;
private String language;
public XPathSuggestions(String language) {

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

It would be better to pass a Language instance and let the constructor get the string name it needs.

private String language;
public XPathSuggestions(String language) {
this.packagename = "net.sourceforge.pmd.lang." + language + ".ast";

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

Don't use a field for that. Pass it as parameter to your list creation routine and only store the result into a field

private void autoComplete(String s) throws IOException, ClassNotFoundException {
autoCompletePopup = new ContextMenu();

This comment has been minimized.

@oowekyala

oowekyala Jun 11, 2018

Member

You still create a new context menu each time!

This comment has been minimized.

@akshatbahety

akshatbahety Jun 11, 2018

Contributor

Will make the update, sorry for the confusion before

@jsotuyod jsotuyod referenced this pull request Jun 11, 2018

Open

[ui] New rule designer pending issues #714

11 of 21 tasks complete

@oowekyala oowekyala added this to In progress in Rule Designer Jun 11, 2018

@akshatbahety akshatbahety force-pushed the akshatbahety:feature branch from 3c13278 to cbf8e6a Jun 12, 2018

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 12, 2018

I have completed most pr review updates , the isASTNode(Class) update is pending will push it in the next commit

int indexOfSlash = reverseStringSearch('/', xpathExpressionArea.getText(), m.getInsertionEnd());
String input = xpathExpressionArea.getText().substring(
indexOfSlash + 1, m.getInsertionEnd());
m.getRemovalEnd();

This comment has been minimized.

@jsotuyod

jsotuyod Jun 14, 2018

Member

this seems to be useless

.getCharacterBoundsOnScreen(positionFrom, positionTo).get().getMaxY());
}
private int reverseStringSearch(char search, String input, int begin) {

This comment has been minimized.

@jsotuyod

jsotuyod Jun 14, 2018

Member

isn't this whole method equivalent to input.lastIndexOf(search, begin - 1)?

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

Oh yes indeed.

return Arrays.asList(classArray).stream()
.filter(files -> files.getSimpleName().startsWith("AST"))
.map(m -> m.getSimpleName().substring(("AST").length()))

This comment has been minimized.

@jsotuyod

jsotuyod Jun 14, 2018

Member

useless parenthesis around "AST"

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)));

This comment has been minimized.

@jsotuyod

jsotuyod Jun 14, 2018

Member

this 6 seems rather magical... ".class".length() I assume?

EventStream<KeyEvent> keyPressed = EventStreams.eventsOf(xpathExpressionArea, KeyEvent.KEY_PRESSED);
EventStream<Integer> keyCombo = keyPressed
.filter(key -> (key.getCode().equals(KeyCode.CONTROL) && key.getCode().equals(KeyCode.SPACE)))

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

The first condition should be key.isControlDown()

EventStream<KeyEvent> keyPressed = EventStreams.eventsOf(xpathExpressionArea, KeyEvent.KEY_PRESSED);
EventStream<Integer> keyCombo = keyPressed

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

You don't need to create so many local variables. Just append your treatment here to the previous stream

.getCharacterBoundsOnScreen(positionFrom, positionTo).get().getMaxY());
}
});
EventStream<Integer> changesEventStream = xpathExpressionArea.plainTextChanges().map(m -> m.getInsertionEnd());

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

This is where you should consider getRemovalEnd. Also, write each processing step on a new line to ease commenting.

.map(key -> xpathExpressionArea.getCaretPosition());
EventStream<Integer> keyEvents = EventStreams.merge(keyCombo, changesEventStream);
keyEvents.map(m -> {

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

Same here, you may just append this treatment to the initialiser expression of the keyEvents variable. Use a better name for m.

.get()
.getMaxX(), xpathExpressionArea
.getCharacterBoundsOnScreen(positionFrom, positionTo).get().getMaxY());
autoCompletePopup.show(xpathExpressionArea, xpathExpressionArea.getCharacterBoundsOnScreen(slashPosition, slashPosition + input.length()).get().getMaxX(),

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

Again, you're calling getCharacterBoundsOnScreen twice. Call it once and use Optional::ifPresent because the get() call is unsage

.getCharacterBoundsOnScreen(positionFrom, positionTo).get().getMaxY());
}
private int reverseStringSearch(char search, String input, int begin) {

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

Oh yes indeed.

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 14, 2018

there are a few bugs sorting them and pushing code very soon

int indexOfSlash = reverseStringSearch('/', xpathExpressionArea.getText(), m);
String input = xpathExpressionArea.getText().substring(indexOfSlash + 1, m);
EventStreams.merge(keyCombo, changesEventStream).map(searchPoint -> {
int indexOfSlash = xpathExpressionArea.getText().lastIndexOf(",", searchPoint - 1);

This comment has been minimized.

@oowekyala

oowekyala Jun 14, 2018

Member

Instead of using the index of the slash, we could use the index of the character right after to avoid having to add +1 (eg when replacing, which for now misses a character) each time.

String input = xpathExpressionArea.getText().substring(indexOfSlash + 1, searchPoint);
System.out.println(input);

This comment has been minimized.

@jsotuyod

jsotuyod Jun 14, 2018

Member

sysout? debug logs?

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 16, 2018

I'll work on the completing this feature from Monday.

@oowekyala

The feature is not far from fully usable.

There are still a few bugs, eg.

Exception in thread "JavaFX Application Thread" java.lang.StringIndexOutOfBoundsException: String index out of range: 46
	at java.lang.String.substring(String.java:1963)
	at net.sourceforge.pmd.util.fxdesigner.XPathPanelController.lambda$initialize$5(XPathPanelController.java:145)
	at org.reactfx.MappedStream.lambda$observeInputs$0(MappedStream.java:25)

Another thing is that sometimes the caret jumps at index 0. Can you investigate that?

Thanks for your efforts @akshatbahety

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 21, 2018

@oowekyala will surely look into it and revert back
I was getting the issue when removing text using anything other than backspace it's more of like time delay in getting the carat position and the text length being updated.

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 26, 2018

Fixed the basic bugs, I hope it can be incorporated please tell me if anything else needs to be done

@oowekyala

This comment has been minimized.

Member

oowekyala commented Jun 27, 2018

@akshatbahety I pushed a merge and a bugfix, please pull before doing additionnal changes

There's still something weird about the contextmenu, though atm I don't know how to work around it. When you select the last item in the dropdown other items are replaced with an up-arrow, which is not cool, especially when there's a single result.

Also, the build fails, because of PMD violations:

[INFO] PMD Failure: net.sourceforge.pmd.util.fxdesigner.XPathPanelController:177 Rule:UseCollectionIsEmpty Priority:1 Substitute calls to size() == 0 (or size() != 0, size() > 0, size() < 1) with calls to isEmpty().
[INFO] PMD Failure: net.sourceforge.pmd.util.fxdesigner.XPathPanelController:197 Rule:ControlStatementBraces Priority:1 This statement should have braces.
[INFO] PMD Failure: net.sourceforge.pmd.util.fxdesigner.model.Style:11 Rule:UseUtilityClass Priority:1 All methods are static.  Consider using a utility class instead. Alternatively, you could add a private constructor or make the class abstract to silence this warning..
[INFO] PMD Failure: net.sourceforge.pmd.util.fxdesigner.model.Style:15 Rule:UseLocaleWithCaseConversions Priority:1 When doing a String.toLowerCase()/toUpperCase() call, use a Locale.
[INFO] PMD Failure: net.sourceforge.pmd.util.fxdesigner.model.Style:15 Rule:UseLocaleWithCaseConversions Priority:1 When doing a String.toLowerCase()/toUpperCase() call, use a Locale.

Other than than I think you're nearly done. The code needs to be modularized, and some other things need to be done before it's released, but I could do that myself. For example, I think the String.contains check is too naïve to be of practical use as a matching algo. If you want to tackle that, you're welcome to do it, otherwise just tell me and include it in my todo list

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jun 29, 2018

@oowekyala I am still working on the Design Issue as you mentioned, and about the algorithm to match nodes, I would really like to work on it.

@oowekyala

This comment has been minimized.

Member

oowekyala commented Jun 30, 2018

Great, the new algo will be for another PR though

@jsotuyod

This comment has been minimized.

Member

jsotuyod commented Jul 9, 2018

@oowekyala shall we merge this as is?

@oowekyala

This comment has been minimized.

Member

oowekyala commented Jul 9, 2018

@jsotuyod I guess we could, unless @akshatbahety has some news about the context menu glitch. If it's fixed in a few days we could wait for it. Otherwise better merge

@akshatbahety

This comment has been minimized.

Contributor

akshatbahety commented Jul 11, 2018

@jsotuyod @oowekyala I haven't found a bug fix till now. I think it would be better to merge it. I'll create another PR once I find a bug fix.

@oowekyala oowekyala merged commit 7f3c93d into pmd:master Jul 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Rule Designer automation moved this from In progress to Done Jul 11, 2018

oowekyala added a commit that referenced this pull request Jul 11, 2018

@oowekyala

This comment has been minimized.

Member

oowekyala commented Jul 11, 2018

Alrighty then, it's merged. Good job @akshatbahety ! I'll modularise the code in a future PR. Looking forward to your future updates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment