-
Notifications
You must be signed in to change notification settings - Fork 5
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
Morph addressbook to entrybook #21
Morph addressbook to entrybook #21
Conversation
Note to self: Changing fields in Entry must change equivalent labels in PersonCard/PersonCardHandle
f860f93
to
4f31e47
Compare
NameContainsKeywordsPredicate -> TitleContainsKeyWordsPredicate
addressbook.json -> entrybook.json
Person -> Entry AddressBook -> EntryBook
It will be created automatically if not found, with the correct names and fields (corresponding to entry book). Important to note that for this to take effect, you'll need to delete preference.json and data/addressbook.json first.
Person -> Entry AddressBook -> EntryBook
"Address" still present internally, just not visible
Internally, "Address" field is now a optional input field (still works if you use "add" command with "a/" prefix), with a default placeholder if none is given. Tests requiring non-null "Address" field are commented off.
Title: n/ -> ti/ (Used to be Name) Comment: p/ -> c/ (Used to be Phone) Link: e/ -> l/ (Used to be Email)
Title: Accepts non-empty, any value Comment: Accepts non-empty, any value (Might want to make optional field in future) Link: protocol//path
GuiRobot wait time set back to 250ms
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.
LGTM
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.
Looks good, just that it'd be better to add some comments on important changes for future reference.
Combining requested changes into one review
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.
Looks good. But I think ParserUtil#parseAddress
should not be modified, instead AddCommandParser
should handle a missing address argument through Optional.orElse
, and another benefit is that EditCommandParser
will not need to be modified too.
Also, do you want to mark this PR as resolving part of #19?
title.setText(entry.getTitle().fullTitle); | ||
comment.setText(entry.getComment().value); | ||
address.setText(entry.getAddress().value); | ||
address.setManaged(false); |
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.
What does this line do? I think we should add a comment about it here for future reference.
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.
Makes the Address label invisible, graphics pipeline will not manage it. And agreed, will do
@@ -12,7 +12,7 @@ | |||
<?import javafx.scene.layout.VBox?> | |||
|
|||
<fx:root type="javafx.stage.Stage" xmlns="http://javafx.com/javafx/8" xmlns:fx="http://javafx.com/fxml/1" | |||
title="Address App" minWidth="450" minHeight="600" onCloseRequest="#handleExit"> | |||
title="Entrybook App" minWidth="450" minHeight="600" onCloseRequest="#handleExit"> |
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.
Small detail but since it's user facing, should we rename this to "Readme App" or just "Readme" instead? Or we can do it in another PR once we have settled on a name.
Comment comment = ParserUtil.parseComment(argMultimap.getValue(PREFIX_COMMENT).get()); | ||
Link link = ParserUtil.parseLink(argMultimap.getValue(PREFIX_LINK).get()); | ||
// Address address = ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).get()); | ||
Address address = ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS)); |
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.
So as not to let the changes propagate too far, instead of modifying ParserUtil
, we can process a missing address argument right here using Optional.orElse
:
Address address = ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).orElse("Default address"));
So ParserUtil#parseAddress
can still retain the same signature as others - Address parseAddress(String address)
instead of Address parseAddress(Optional<String> maybeAddress)
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.
Good idea!
} | ||
if (argMultimap.getValue(PREFIX_ADDRESS).isPresent()) { | ||
editPersonDescriptor.setAddress(ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS).get())); | ||
editPersonDescriptor.setAddress(ParserUtil.parseAddress(argMultimap.getValue(PREFIX_ADDRESS))); |
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.
If you take my suggestion to handle a missing address argument in AddCommandParser
, then this line can go back to the original.
Change title to Readme
Morphs application and features to operate on entries for links and feeds, instead of persons and addresses
Person
->Entry
AddressBook
->EntryBook
Name
->Title
Phone
->Comment
Email
->Link
Address
(made invisible, unused field. Kept to pass regression tests, and as a placeholder for future properties ofEntry
)