AY1920S2-CS2103T-W17-2 / main

Notably
https://ay1920s2-cs2103t-w17-2.github.io/main/
MIT License
3 stars 4 forks source link

Suggestions #1

Open ljiazh3ng opened 4 years ago

ljiazh3ng commented 4 years ago

Handled by 1 person.

This epic deals with the following:

johannagwan commented 4 years ago

Proposed API:

EDIT: Removed SuggestionResult interface.

/**
 * Represents the Suggestion Engine instance.
 * This is the class that we interact with when the user is typing an input.
 */
interface SuggestionEngine {
  void suggest(String input); // To generate the suggestions. When input cannot get corrected, the "error" will be shown in commandTextProperty.
}

/**
 * Represents the Suggestion Command instance.
 */
interface SuggestionCommand {
  void execute(Model model); // To get a list of suggestions (paths or notes that match the current input) and add this list to SuggestionModel.
}

/**
 * Represents the Suggestion Model instance.
 * The UI will directly interact with this model to display the suggestions result.
 */
interface SuggestionModel {
  ObservableList<SuggestionItem> getSuggestions(); // To get the list of suggestions saved in the model.
  void setSuggestions(List<SuggestionItem> suggestions); // To save the list of suggestions in the model.
  Property<Optional<String>> commandTextProperty(); // To show user the meaning of their input, e.g. "Create a new note". TODO: add logo.
  void setCommandInputText(String commandText);
  String getCommandInputText();
  void clearCommandInputText(); // When the text field is empty.
  void clearSuggestions();
}

/**
 * Represents the instance of the suggested item.
 */
interface SuggestionItem {
  String getDisplayText(); // To get the note path or name displayed in the UI, e.g. "open -t root/cs2103".
  Runnable run(); // The action taken after the suggestion item is chosen. Will be different depending on the type of SuggestionCommand.
}

@kevinputera @HemanshuGandhi @ljiazh3ng @firzanarmani What do you guys think about this?

ljiazh3ng commented 4 years ago

Looks good so far. I have a question regarding the Suggestion Result class. If it is already implemented as an observable list contained in the Suggestion Model. Do we still need the SuggestionResult class ? This is similar to the case where we removed the CommandResult class.

But other than that everything looks good, will have more clarity when trying to implement the interface. Good Job:D

johannagwan commented 4 years ago

Thanks @ljiazh3ng! Yes, after thinking about it, I think it's better to remove SuggestionResult. Thank you so much!

kevinputera commented 4 years ago

Hi @johannagwan , thanks for putting this up!

/**
 * Represents the Suggestion Engine instance.
 * This is the class that we interact with when the user is typing an input.
 */
interface SuggestionEngine {
  Optional<String> getCorrectedInput(String input); // Use the CorrectionEngine to correct the input in case there is a typo.
  String[] getTokenizedInput(String correctedInput); // To tokenize the input.
  SuggestionCommand getCommand(); // To make sense of the command. 
}

For SuggestionEngine, which API should be called to trigger the suggestion generation (out of the three methods provided)? I'd suggest that we stay consistent with the API Parser provides, i.e. Parser's parse method.

/**
 * Represents the Suggestion Command instance.
 */
interface SuggestionCommand {
  ObservableList<SuggestionItem> execute(Model model); // To get a list of suggestions (paths or notes that match the current input) and add this list to SuggestionModel.
}

Since your execute method gets a list of suggestions, which in turn will be given to SuggestionModel, what do you think about making execute return void? The SuggestionCommand instance can update the SuggestionModel directly inside its execution.

/**
 * Represents the Suggestion Model instance.
 */
interface SuggestionModel {
  OurDataStructureType getDataStructure(); // To be renamed once we finish our DS.
  ObservableList<SuggestionItem> getSuggestions(); // To get the list of suggestions.
  void setSuggestions(ObservableList<SuggestionItem> suggestions); // To save the list of suggestions in the model.
}

Curious, why do we need the getDataStructure method here? In addition, let's make the setSuggestions method take in a List<SuggestionItem> instead. Programming to an interface is always better!

/**
 * Represents the instance of the suggested item.
 */
interface SuggestionItem {
  String getDisplayText(); // To get the note path or name displayed in the UI, e.g. "open -t root/cs2103".
  String setDisplayText(String displayText); // To set the note path and title as the display text.
  Runnable run(); // The action taken after the suggestion item is chosen. Will be different depending on the type of SuggestionCommand.
}

For SuggestionItem, do we need the setDisplayText method? Thinking through the possible use cases, I couldn't see any instance where we'll need to update the item's display text. But I might miss out some parts!

johannagwan commented 4 years ago

Hi @kevinputera, thanks for the feedback! I have edited the API accordingly, except for the SuggestionItem's setDisplayText(). My idea is, inside the execute method in SuggestionCommand, I will:

  1. Get a list of notes that match our current input.
  2. For each of this note, I will create a SuggestionItem object, then set its displayText as the note's path/ title, and run the necessary action.
  3. Add this list into Suggestion Model using setSuggestions method.

If you have a better way to display the text for the suggestion items, please let me know! Thanks in advance.

kevinputera commented 4 years ago

Hi @kevinputera, thanks for the feedback! I have edited the API accordingly, except for the SuggestionItem's setDisplayText(). My idea is, inside the execute method in SuggestionCommand, I will:

  1. Get a list of notes that match our current input.
  2. For each of this note, I will create a SuggestionItem object, then set its displayText as the note's path/ title, and run the necessary action.
  3. Add this list into Suggestion Model using setSuggestions method.

If you have a better way to display the text for the suggestion items, please let me know! Thanks in advance.

Hey Johanna!

The choice of whether to keep setDisplayText() or to remove it is totally up to you. The difference between the two is minor, but, to answer your question:

Yes, there is a way to add the displayText in SuggestionItem without using the setDisplayItem method. We can set this during SuggestionItem creation (in the constructor), e.g. new SuggestionItem("This is the title").

Now, the difference between the two is that:

Again, it's totally up to you which you wanna proceed with. The difference between the two is not that big.

johannagwan commented 4 years ago

I see, thanks @kevinputera for the explanation! I will change it accordingly.