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

Morph addressbook to entrybook #21

Conversation

epicfailname
Copy link

@epicfailname epicfailname commented Feb 25, 2019

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 of Entry)
  • Minimal changes made to reflect the morphed state on the user-side
  • Several internal functions and names retain their old names

Note to self: Changing fields in Entry must change equivalent labels in PersonCard/PersonCardHandle
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
@epicfailname epicfailname changed the title [WIP] Morph addressback to entrybook Morph addressbook to entrybook Feb 27, 2019
@epicfailname
Copy link
Author

epicfailname commented Feb 27, 2019

Resolves #30
Resolves part of #19

@thomastanck thomastanck self-requested a review February 27, 2019 10:47
@epicfailname epicfailname added this to the v1.1 milestone Feb 27, 2019
@epicfailname epicfailname self-assigned this Feb 27, 2019
Copy link

@thomastanck thomastanck left a comment

Choose a reason for hiding this comment

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

LGTM

rlrh
rlrh previously requested changes Feb 28, 2019
Copy link

@rlrh rlrh left a 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.

src/main/java/seedu/address/ui/EntryCard.java Outdated Show resolved Hide resolved
@rlrh rlrh self-requested a review February 28, 2019 03:17
@rlrh rlrh dismissed their stale review February 28, 2019 03:18

Combining requested changes into one review

Copy link

@rlrh rlrh left a 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);
Copy link

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.

Copy link
Author

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">
Copy link

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

@rlrh rlrh Feb 28, 2019

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)

Copy link
Author

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

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.

@epicfailname epicfailname merged commit e796313 into CS2103-AY1819S2-W10-1:master Feb 28, 2019
@epicfailname epicfailname deleted the morph-addressback-to-entrybook branch March 19, 2019 16:48
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.

None yet

3 participants