jbb01 / QED

MIT License
11 stars 2 forks source link

Feature: Export contact information to address book #38

Open jrpie opened 1 year ago

jrpie commented 1 year ago

This implements #6

jbb01 commented 1 year ago

First of all, I want to thank you for your contribution. Although I was thinking about solving that issue using the contacts provider API, this solution seems sufficient too.

I'm kinda busy right now so I haven't found time to review the PR in depth, but from a cursory look these are the main two points I have to criticize:

jrpie commented 1 year ago

Although I was thinking about solving that issue using the contacts provider API, this solution seems sufficient too.

I was also thinking about that, but came to the conclusion, that - at least for me - having the entire QEDDB copied into my address book is not what I'd want.

the list of contact types supported by Actions::exportToAddressBook is not consistent with the contact types supported by PersonInfoFragment.CONTACT_ICONS and PersonInfoFragment.CONTACT_ACTIONS

Yes, Actions::exportToAddressBook definitely needs some work. Most of it should probably be moved to a helper class. What do you think about creating a helper class for contact detail classification, that combines the logic for PersonInfoFragment.CONTACT_ICONS, PersonInfoFragment.CONTACT_ACTIONS and selecting the appropriate types for export? That way everything would be in one place. Btw. who is using phön? :joy:

the proposed changes don't affect the person info bottom sheet

Thanks, I wasn't aware this exists.

Another point I'd like to mention is the problem of adding a new menu icon to the toolbar. I am not sure about the best way to do this. I see the following the options:

  1. Allow subclasses of MainInfoFragment to add additional MenuProviders (this is what I have implemented, but it implies that MainInfoFragment::onViewCreated can no longer be final),
  2. add a generic export option, say isExportSupported(), to InfoFragment similary to how isOpenInBrowserSupported() works (might be questionable, if PersonInfoFragments ends up being the only thing using it) or
  3. don't derive PersonInfoFragment from InfoFragment (seems like a bad idea).
jbb01 commented 1 year ago

I was also thinking about that, but came to the conclusion, that - at least for me - having the entire QEDDB copied into my address book is not what I'd want.

I completly agree. I was thinking about selecting people to be exported with something like a favorite button.

Most of it should probably be moved to a helper class.

I'd suggest introducing a Contact (ContactInformation?, possibly nested in Person) class containing those features and also replacing the awkward Pair<String, String> currently in use.

Btw. who is using phön?

dkddiermsü

Another point I'd like to mention is the problem of adding a new menu icon to the toolbar.

The usual way would definetly be allowing additional menu providers. The reason it's handled differently for "open in browser" is that the bottom sheet does not have a toolbar until it is fully expanded and the "open in browser" button appears next to the title until the sheet is fully expanded when the title disappears behind the toolbar.

Maybe we can do without the export button until the sheet is fully expanded so we only need special handling for the "open in browser" button. I'm not completly sure if a toolbar can handle multiple menu providers, but if so we could automatically add an open in browser menu provider inside InfoFragment and still allow additional menu providers.