Skip to content

Conversation

@jrpie
Copy link
Collaborator

@jrpie jrpie commented Sep 13, 2023

This implements #6

@jbb01
Copy link
Owner

jbb01 commented Sep 15, 2023

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:

  • 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
  • the proposed changes don't affect the person info bottom sheet

@jrpie
Copy link
Collaborator Author

jrpie commented Sep 15, 2023

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? 😂

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

jbb01 commented Sep 17, 2023

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.

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.

2 participants