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

Added AddressBook feature #2414

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

Conversation

asif-finimble
Copy link
Contributor

Closes #1672

@JamesSmartCell I have implemented AddressBook feature and added it to InputAddress component. Here are the details regarding the implementation. Please let me know if I need to change/update any thing.

AddressBook Activity

  • 2 modes : Normal and Contact selection
  • Normal mode
    • shows list of contacts and allows to add more contacts by clicking + button in toolbar
    • opens edit address screen on clicking any item.
    • allows swipe to delete
  • Contact selection mode: Used for selecting a contact from address book
    • OnClicking an item, it is set as activity result for the calling activity to receive the contact.
    • does not allow swipe to delete

AddEditAddress Activity

Modes: Add & Edit

  • Add

    • if ens name is entered in Address, if it resolves, address will be set in address box and the ens name will be auto filled in Name box. Also ens name will be recorded for the entered address.
    • on clicking save, if data us valid, it is added to address book and activity closes.
    • Validations:
      • Address and name should not be empty
      • Address should be valid. Utils.isAddressValid()
      • Address & Name doesnt already exist in exisiting contacts.
  • Edit

    • Address input is disabled. address cannot be edited. Only name is editable
    • on clicking save, if data is valid, the contact name is updated.
    • Validation rules
      • Name is not empty
      • Old name and new name are different
      • new name is not assigned to other contact.
      • Address is valid.

Input Address Component

  • showAddressBook attribute allows to show AddressBook text beside the paste text.
  • On clicking, AddressBook text, AddressBook Activity is opened for contact selection.
  • The host activity of input address component should handle onActivityResult to get the data returned by AddressBookActivity

TransactionDetailActivity

  • If the sender and receiver address are present in address book, their name will be shown instead of address. On Clicking copy button, the address will be copied.
  • If address is not present in AddressBook, AddressBook icon is shown instead of copy icon. Onclicking it, AddEditAddressActivity is opened with the address auto filled.

Activity Adapter

  • passing AddressBookInteract to activity adapter to resolve the to & from address names if present in address book.
  • Not posiible to resolve as address is returned in the form of 0x42c...95cs

@JamesSmartCell
Copy link
Member

@asif-finimble this is a very large PR for a small feature. Can we prune it a little to get the code change down a little.

I can see a few wins in the duplicated resources, there are a few strings which are already translated.

Also, the adapter sliding has already been written, no need for a new class to do the same thing. Please see TokensAdapter which uses the token sliding (if that's what it's for - I might have mis-read it).

Let's work together after I get back from the conference to work out how to reduce the code size; it looks like a good implementation but I'm pretty sure we can re-use a lot of existing components rather than writing new ones that duplicate function.

@asif-finimble
Copy link
Contributor Author

asif-finimble commented Feb 19, 2022

OK. I went through the code and the sliding classes are used as inner classes so they are tightly coupled. It wont work with AddressBook.

@JamesSmartCell
Copy link
Member

OK. I went through the code and the sliding classes are used as inner classes so they are tightly coupled. It wont work with AddressBook.

Ok, but then that slider should be moved out of the class rather than duplicating code. Can probably just move the slider inner class out verbatim as we know it's reliable (unless it accesses private vars, in which case it'll need a new callback interface).

Adding AddressBook should not need to touch 58 files. It should only operate on InputAddress, possibly a BottomSheetDialog. I don't think it's been fully designed yet. Let me get back to you on this one. I agree with your implementation it should be a Realm item rather than SharedPrefs.

@asif-finimble
Copy link
Contributor Author

The sliding class is only extending ItemTouchHelper.SimpleCallback and overriding the required methods for drawing the content to show for sliding and the implementation of each of them is tightly coupled and specific to that class. Even if we generalize the sliding class most of the code is different and we cannot really reuse the class. Let me what we should do for this.

As for the AddressBook, I found that the AddressBook feature was not implemented. So first I implemented the AddressBook feature by referring the zeplin designs and after that integrated it with InputAddress. So I had to create the UI, repository , interact, etc for AddressBook and that's why there are changes to 58 files. Let me know what should I do further.

@asif-finimble
Copy link
Contributor Author

@JamesSmartCell I resolved the conflicts.

/**
* This is associated ETH name of #walletAddress
*/
private String ethName;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mean ens name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

}
catch (Exception e)
{
e.printStackTrace();
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated logging with Timber.

.equalTo("walletAddress", oldContact.getWalletAddress())
.findFirst();
if (realmContact != null) {
// realmContact.setName(newContact.getName());
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment can be removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

} else {
mode = ADDRESS_OPERATION_MODE.EDIT;
setTitle(getString(R.string.title_edit_address));
contactToEdit = getIntent().getParcelableExtra(C.EXTRA_CONTACT);
Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

String name = inputViewName.getText().toString().trim();
boolean isDataValid = true;
if (address.isEmpty()) {
inputViewAddress.setError("Address should not be empty");
Copy link
Contributor

Choose a reason for hiding this comment

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

i18n

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

data.add(position, item);
notifyItemInserted(position);
} catch(Exception e) {
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Log error.

@asif-finimble
Copy link
Contributor Author

@seabornlee, code updated with changes.

@seabornlee
Copy link
Contributor

@seabornlee, code updated with changes.

LGTM, thanks a lot!

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.

Add AddressBook to InputAddress component
3 participants