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

Add option to edit structured addresses (new) #57

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

Conversation

stephanritscher
Copy link
Contributor

@stephanritscher stephanritscher commented Jan 28, 2024

What is it?

  • Bugfix
  • Feature
  • Codebase improvement

Description of the changes in your PR

  • Introduce option to edit structured addresses as requested.
  • Based on this fork,
  • In the current state, the PR changes the dependency to my fork of Commons. This needs to be revisited before merging.

Before/After Screenshots/Screen Record

  • Before:
  • After:

Fixes the following issue(s)

Relies on the following changes

Acknowledgement

@raycadle
Copy link

This PR would make Fossify Contacts so much better. I've stayed with Simple Contacts Pro SE, as the structured address feature allows my contacts to integrate very well into my Nextcloud instance.

@stephanritscher
Copy link
Contributor Author

@naveensingh What do you think about the feature respectively pull request?

@naveensingh
Copy link
Member

@stephanritscher Sounds like a good idea but I'm curious, why wasn't this merged in the original Simple Contacts?

Note: I'm yet to review this or any other PR.

@stephanritscher
Copy link
Contributor Author

Tibbi didn't like having additional options and didn't regard this as important feature.

@alensiljak
Copy link

Note: I'm yet to review this or any other PR.

This is implemented and works well in Simple Contacts Pro SE. Probably the sooner it is merged, the less issues there would be in the process.
It is a great feature. I've been fixing addresses for the last 30 years and if it is only because someone didn't find it important enough than it is just plain sad. Please!

@@ -82,6 +89,7 @@
<string name="phone_numbers">Phone numbers</string>
<string name="emails">Emails</string>
<string name="addresses">Addresses</string>
<string name="structured_addresses">Structured Addresses (edit mode)</string>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<string name="structured_addresses">Structured Addresses (edit mode)</string>
<string name="structured_addresses">Structured addresses</string>

The (edit mode) part doesn't seem necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just wanted to manage the expectation here. The contact view is not influenced by this switch, only the edit mode shows the structured address.

@naveensingh
Copy link
Member

naveensingh commented Oct 25, 2024

@stephanritscher

I did some testing and found a few issues:

  • When the address fields are empty and I disable structured addresses, the structured address fields are still visible.

  • When the address fields are filled and I disable structured addresses, the app crashes:

    Process: org.fossify.contacts, PID: 21824
    java.lang.NullPointerException: Missing required view with ID: org.fossify.contacts:id/contact_city
        at org.fossify.contacts.databinding.ItemEditStructuredAddressBinding.bind(ItemEditStructuredAddressBinding.java:173)
        at org.fossify.contacts.activities.EditContactActivity.setupAddresses(EditContactActivity.kt:621)
        at org.fossify.contacts.activities.EditContactActivity.setupEditContact(EditContactActivity.kt:496)
        at org.fossify.contacts.activities.EditContactActivity.gotContact(EditContactActivity.kt:178)
        at org.fossify.contacts.activities.EditContactActivity.access$startChoosePhotoIntent(EditContactActivity.kt:55)
        at org.fossify.contacts.activities.EditContactActivity.access$gotContact(EditContactActivity.kt:55)
        at org.fossify.contacts.activities.EditContactActivity$initContact$1.invoke$lambda$0(EditContactActivity.kt:164)
        at android.os.Handler.handleCallback(Handler.java:984)
      ...
    
    
  • When I enable/disable structured addresses and press back, the app crashes. Logs:

    Process: org.fossify.contacts, PID: 32013
    java.lang.NullPointerException: Missing required view with ID: org.fossify.contacts:id/contact_city
        at org.fossify.contacts.databinding.ItemEditStructuredAddressBinding.bind(ItemEditStructuredAddressBinding.java:173)
        at org.fossify.contacts.activities.EditContactActivity.getFilledAddresses(EditContactActivity.kt:1212)
        at org.fossify.contacts.activities.EditContactActivity.fillContactValues(EditContactActivity.kt:1136)
        at org.fossify.contacts.activities.EditContactActivity.hasContactChanged(EditContactActivity.kt:367)
        at org.fossify.contacts.activities.EditContactActivity.onBackPressed(EditContactActivity.kt:299)
        at android.app.Activity.onKeyUp(Activity.java:4058)
    ...
    
    Process: org.fossify.contacts, PID: 31881
    java.lang.NullPointerException: Missing required view with ID: org.fossify.contacts:id/contact_address
        at org.fossify.contacts.databinding.ItemEditAddressBinding.bind(ItemEditAddressBinding.java:86)
        at org.fossify.contacts.activities.EditContactActivity.getFilledAddresses(EditContactActivity.kt:1238)
        at org.fossify.contacts.activities.EditContactActivity.fillContactValues(EditContactActivity.kt:1136)
        at org.fossify.contacts.activities.EditContactActivity.hasContactChanged(EditContactActivity.kt:367)
        at org.fossify.contacts.activities.EditContactActivity.onBackPressed(EditContactActivity.kt:299)
        at android.app.Activity.onKeyUp(Activity.java:4046)
        at android.view.KeyEvent.dispatch(KeyEvent.java:2931)
        at android.app.Activity.dispatchKeyEvent(Activity.java:4421)
    ...
    
  • Not sure if intended but when I enable "Structured addresses (edit mode)" and disable the Addresses field, structured addresses fields are not shown.

@naveensingh naveensingh added the testers needed We need testers for this issue or pull request label Oct 25, 2024
@bege10
Copy link

bege10 commented Oct 26, 2024

* When the address fields are empty and I disable structured addresses, the structured address fields are still visible.

* When the address fields are filled and I disable structured addresses, the app crashes:
* When I enable/disable structured addresses and press back, the app crashes.

The same issues are in the app of @stephanritscher .
I never came across these crashes because I edited the visible fields only in the app settings. A reasonable solution could be to remove these settings from the edit dialog.

* Not sure if intended but when I enable "Structured addresses (edit mode)" and disable the `Addresses` field, structured addresses fields are not shown.

This makes sense to me. If I don't want to see the addresses it doesn't matter what the setting for structured addresses is. Structured addresses is kind of a sub-setting of addresses. Maybe the UI could reflect that.

Actually a new feature but related to this one:
Don't disable the not visible fields completely but hide them behind a "hidden fields" button at the bottom like the "more fields" button in LineageOS contacts app.

@stephanritscher
Copy link
Contributor Author

stephanritscher commented Oct 26, 2024

@naveensingh @bege10 Thanks a lot for testing. I didn't come across crashes for a while which shows you know the corner cases of the app much better than me. I will have a look and try to fix the issues.

@stephanritscher stephanritscher force-pushed the structured-addresses-pr branch from b068c6e to 3f345b1 Compare October 28, 2024 20:22
@stephanritscher
Copy link
Contributor Author

Hi,

I think I fixed the issues. I also rebased to fossify/master and updated stephanritscher/FossifyCommons accordingly.

One thing I noticed when testing is that editing a contact present on multiple accounts with merge option enabled behaves differently (and worse in my setup) than SimpleContacts did. Previously it opened the "main" account (probably the one providing most fields or similar), now FossifyContacts seems to open always a "side" account on my device with barely any contact data - might be the first account, but not lexicographically). Before noticing a different account was opened, I thought I introduced a bug, but I verified the behaviour with the app you published in F-Droid. Was there an intentional change related to this?

Best regards,
Stephan

@bege10
Copy link

bege10 commented Oct 29, 2024 via email

@DiagonalArg
Copy link

You may have seen this already, but I was looking through the RFC for this partly related feature request, and found that there is mention of address components in the May 2024 extension to the RFC

This specification modifies the definition of the ADR property. It extends its structured value with additional address components to better support the variety of international addresses. It separates the address parts, which currently are typically combined in street address component values, into distinct components.

Perhaps it may be relevant. And, thank you for doing this @stephanritscher . I use Nextcloud too, and had been using your app.

@DiagonalArg
Copy link

DiagonalArg commented Nov 29, 2024

Note also the related request for structured name fields, which include standard properties ORG and TITLE (which Nextcloud uses), along with NICKNAME, and various extensions as I've posted in #193 , #194 and another poster in #168

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
testers needed We need testers for this issue or pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] Add support for structured addresses
6 participants