Skip to content
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

[T7][T14-C2] #109

Open
wants to merge 394 commits into
base: master
Choose a base branch
from
Open

Conversation

qhng
Copy link

@qhng qhng commented Oct 7, 2016

No description provided.

Copy link

@okkhoy okkhoy left a comment

Choose a reason for hiding this comment

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

Hi guys,

This is a really neat and well thought of user guide. I appreciate the effort you have put into this.
Please keep up the good work!
There are some minor picks, please read the comments in detail :-)

Cheers,
Akshay


# Contributors

We welcome contributions. See [Contact Us](ContactUs.md) page for more info.
Copy link

Choose a reason for hiding this comment

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

you "contact us" asks people to email Prof. Damith :-P change that to one/all of your emails

1. Download the latest `addressbook.jar` from the [releases](../../../releases) tab.
2. Copy the file to the folder you want to use as the home folder for your Address Book.
1. Download the latest `savvytasker.jar` from the [releases](../../../releases) tab.
2. Copy the file to the folder you want to use as the home folder for your Savvy Tasker.
Copy link

Choose a reason for hiding this comment

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

about the UI Mock up:
is the calendar view shown on the right an integrated Google calendar or your own?
It looks pretty cool, however, be careful that implementing your own may need a lot of work!

Adds a task to Savvy Tasker.<br>
Format: `add TASK_NAME [s/START_DATE] [st/START_TIME] [e/END_DATE] [et/END_TIME] [l/LOCATION] [p/PRIORITY_LEVEL] [r/RECURRING_TYPE] [n/NUMBER_OF_RECURRENCE] [c/CATEGORY] [d/DESCRIPTION]`

> If DATE is entered but TIME is not entered, the task is assumed to occur all-day <br>
Copy link

Choose a reason for hiding this comment

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

💯 for capturing the constraints! good job on that!!
try making this a bullet list? this looks like a dump of text which the user may conveniently skip :-P

Format: `add TASK_NAME [s/START_DATE] [st/START_TIME] [e/END_DATE] [et/END_TIME] [l/LOCATION] [p/PRIORITY_LEVEL] [r/RECURRING_TYPE] [n/NUMBER_OF_RECURRENCE] [c/CATEGORY] [d/DESCRIPTION]`

> If DATE is entered but TIME is not entered, the task is assumed to occur all-day <br>
> If TIME is entered but DATE is not entered, DATE is assumed to be current date <br>
Copy link

Choose a reason for hiding this comment

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

what if the time has already elapsed?
e.g., i record a task for 11 am at 1 pm, your default is 1 hr duration, and the hour has passed. then think if it makes sense to use the same date or the next date? either way you need to provide feedback to the user about the default assumed when the command completes.

> If START_DATE and END_DATE are different, END_DATE must be later than START_DATE, and END_TIME does not have to be later than START_TIME <br>
> If START_DATE and END_DATE are the same, END_TIME must be later than START_TIME <br>
> For DATE the format is as follows: dd-mm-yyyy <br>
> For TIME the format is as follows: hh:MM, hh:MM defaults to 00:00 if not specified. <br>
Copy link

Choose a reason for hiding this comment

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

i m confused, isn't hh:mm means 12 hr format and HH:mm mean 24 hr? if you say hh:MM then you need to specify am/pm, no? (or it may be that I am completely confused about the formatting) do check and fix this.


> Selects the person and loads the Google search page the person at the specified `INDEX`.
> Selects the task and marks the task as done at the specified `INDEX`.
Copy link

Choose a reason for hiding this comment

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

may be you need to be specific here, to tell list the archived tasks and then unmark? since archived tasks are not shown in the default list, unmark index in the current view will mane little sense, as that task is not completed anyway. no?

Unmarks the 1st task in the results of the `find` command as done.

#### Undo the most recent operation : `undo`
Undo the most recent command that was executed.<br>
Copy link

Choose a reason for hiding this comment

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

why not multiple levels of undo? #justasking :-D


#### Clearing all entries : `clear`
Clears all entries from the address book.<br>
Clears all entries from the Savvy Task.<br>
Copy link

Choose a reason for hiding this comment

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

is this undo-able as well? need to mention that here, eitherways

Exits the program.<br>
Format: `exit`
#### Alias a keyword : `alias`
Alias a keyword with shorter version of keyword <br>
Copy link

Choose a reason for hiding this comment

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

can you alias a command? e.g, alias add to + or something like that.
if so, please mention that. its a nice to have feature for advanced users.

Format: `delete INDEX`
#### Modifies a task : `modify`
Marks the task identified by the index number used in the last task listing.<br>
Format: `modify INDEX [t/TASK_NAME] [s/START_DATE] [st/START_TIME] [e/END_DATE] [et/END_TIME] [l/LOCATION] [p/PRIORITY_LEVEL] [r/RECURRING_TYPE] [n/NUMBER_OF_RECURRENCE] [c/CATEGORY] [d/DESCRIPTION]`
Copy link

Choose a reason for hiding this comment

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

may be also refer the user to add for the details of format and constraints?

@okkhoy
Copy link

okkhoy commented Oct 10, 2016

@qhng @e0003801 @ruiwen905 @tet54 good job guys! 👍
Some comments added. Please ack & close the PR after reading comments.

@qhng qhng closed this Oct 12, 2016
qhng and others added 25 commits October 18, 2016 22:41
…hods to not reset parameter.

Add AliasCommandParser
Add skeleton methods addPreprocessorSymbol and removePreprocessorSymbol in LogicManager to prepare for future alias implementation
Update ParserTest to conduct unit test on AliasCommandParser
Add setLogic() method in Command abstract class
Add LogicRequiringCommand and ModelRequiringCommand abstract classes to provide a way to get access to logic and model dependencies
Refactor *Command classes to inherit from ModelRequiringCommand and LogicRequiringCommand whereas before that they inherit directly from Command to gain dependencies.
…icManager

Modify AliasCommand to prepare for its implementation
Add preprocessing handling methods in MasterParser
Add AliasSymbolList as the collection class for storing AliasSymbol
Add DuplicateSymbolKeywordException, SymbolKeywordNotFoundException class for errors when using AliasSymbolList add and remove method
Add AliasSymbolChangedEvent for notifying observers that alias symbol has been added or removed
Add XmlAdaptedAliasSymbol which is used for serialization and deserialization of AliasSymbol
Modify SavvyTasker class to provide methods to add and remove AliasSymbol data into internal AliasSymbolList
Modify ReadOnlySavyTasker to provide methods for accessing AliasSymbolList
Modify Model interface and ModelManager class to provide methods to add and remove AliasSymbol data as well as raise AliasSymbolChangedEvent
Modify XmlSerializableSavvyTasker to be capable of being stored/loaded to for AliasSymbol data
Modify Logic interface to no longer expose AliasSymbol handling methods and undo/redo functionality - Changes should be communicated through events
Modify LogicManager to subscribe to AliasSymbolChangedEvent to handle adding and removal of AliasSymbols and removed previous addPreprocessSymbol and removePreprocessSymbol methods
Implement LogicManager.loadAllAliasSymbols() to add externally loaded symbols (from the storage) into the parser during construction stage
Implement AliasCommand.execute() to work properly
Refactor AliasCommandModel and AliasCommandParser to use consistent naming of AliasSymbol components
Fix XmlAdaptedTask showing wrong comment
Remove LogicRequiringModel class - now AliasSymbol changes are communicated through events rather than directly through LogicRequiringModel dependency
Implement UnaliasCommand.execute() to execute unalias command
Refactor UnaliasCommandModel to use consistent naming
Refactor AliasCommand to use consistent naming
Modify ParserTest to also test UnaliasCommandParser
Update UserGuide.md to reflect changes in alias and unalias command
…ying of ability of parsing a command

Add dependency from AliasCommand to Logic
Add checking in AliasCommand.execute() to prevent using command headers as aliasing keyword
Update MasterParser.parse() to enable aliasing
Update AliasCommandParser and UnaliasCommandParser to turn off aliasing
Modify ParserTest to test using MasterParser instead of individual parser.
Modify ParserTest to conduct parameterized testing.
…egistration fails, and to reject any parser with the same header as existing AliasSymbol keyword

Rollback ParserTest and separated MasterParser test into its own test case
Organised imports for all classes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants