Skip to content

codereview #2 of 12 (Moroldo) from 0 (Fasan)#114

Open
lucamoroz wants to merge 4 commits intoPrandiniUniPD:codereviewfrom
lucamoroz:2codereview2
Open

codereview #2 of 12 (Moroldo) from 0 (Fasan)#114
lucamoroz wants to merge 4 commits intoPrandiniUniPD:codereviewfrom
lucamoroz:2codereview2

Conversation

@lucamoroz
Copy link
Copy Markdown
Collaborator

Dear Mr. Fasan,
I hope this message finds you well.
The file involved in this review is SearchResultActivity, an activity that allow the user to manually search for any ingredient and display the ingredient data.
Please feel free to ask for anything about the code.

@lucamoroz lucamoroz requested a review from Giofasan January 10, 2019 18:02
/**
* Size of the dropdown suggestions list
*/
private static final int MAX_DROPDOWN_SUGGESTIONS = 40;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion 40 as Size of the dropdown list is too high. I recommend reducing it to 10/20.

}

@Override
protected void onNewIntent(Intent intent) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend adding a brief comment on what this function does.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a comment soon, thank you for the report


//update textview text and set cursor to the end
mAutoCompleteTextView.setText(query);
mAutoCompleteTextView.setSelection(query.length());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand what the setSelection function does

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sets the cursor at the end of the word, so the user can eventually append some text after choosing any suggestion

//update TextView and set cursor at the end of the text
mAutoCompleteTextView.setText(ingredientClicked);
mAutoCompleteTextView.setSelection(mAutoCompleteTextView.getText().length());
mSearchButton.performClick();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you add a comment to explain what the function .performClick () does.
Like this "Call this view's OnClickListener, if it is defined. Performs all normal actions associated with clicking".

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok thank you for reporting that

}

@Override
protected FilterResults performFiltering(CharSequence constraint) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you use a CharSequence variable instead of a String variable since then you have to convert the CharSequence variable to String?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I'm overriding this class functions:
https://developer.android.com/reference/android/widget/Filter

* starts with the constraint query or contains a substring that starts with the constraint
* query.
*/
private class AutoCompleteTVSetup implements Runnable {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great idea to use a thread

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants