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

Ionicc issue 314 #594

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

Ionicc issue 314 #594

wants to merge 7 commits into from

Conversation

ionicc
Copy link

@ionicc ionicc commented Jun 7, 2017

Fixes the permission issue and has some new (Non-Working and Not affecting the existing code)

Added a feature to import all contacts in the phone to Olympus app
Permission patch for RestComm#314 and working on RestComm#368
@ionicc
Copy link
Author

ionicc commented Jun 7, 2017

After clicking on import contacts you may sometimes notice that the MainActivity is not being added with the new contacts. Just switch to any other activity or close the app and start it again.

@atsakiridis atsakiridis mentioned this pull request Jun 7, 2017
@atsakiridis
Copy link
Collaborator

@ionicc thanks for the new push. A couple of comments:

  • It still crashes for me even with new code. Some things I'd like you to check:
    • in MainActivity the checkPermissions() functionality should probably go away as you moved it to the Fragment
    • The permission handling seems wrong, which I think causes the crash. Remember that dynamic permissions work asynchronously. That is you cannot request for them and directly request contacts without first waiting for the dynamic permission callback. Please read https://developer.android.com/training/permissions/requesting.html for more details on how to handle that properly.
  • Can you debug why the contacts aren't added properly some times and you have to switch to another activity for that to work? We cannot ask our users to do that :D
  • What happens if you hit 'Import all contact from phone' twice? Do they get added twice? Because that would we wrong
  • Please try not to change the amount of whitespace when editing files because then it's difficult to diff, for example in GitHub it's like you have changed 100% of the file which is not the case. Can you try to revert to previous whitespace amount? I think it was 3 spaces
  • From now on try to use separate branches per features. Because right now in the same branch I see both changes for the Contacts, as well as for the multiple domain fix
  • On the multiple domain fix, I see that you have already started to update SQLite. I thought we agreed not to go that way unless there is no other way because the implementing & maintaining overhead is big. Did you check of any other ways to do that? By the way let's not complicate things too much with doing many issues in parallel. For now I'd say let's focus on the Contacts issue and leave the other one with multiple domains for later.
  • I realized that adding so many contacts makes it very tough to spot them, so I think we need to introduce also search logic in the Contacts screen, otherwise it will become unusable. Don't bother with that yet though. Let's first fix all other issues first and we can discuss later.
  • Also to get the contacts actually working we need to make SDK deal with spaces and dash characters. Again this is not something you need to work on, just putting it here to have everything in one place. Solution for this is probably: Introduce number validation in SDK #576

@ionicc
Copy link
Author

ionicc commented Jun 7, 2017

  1. Could you provide me the stack trace as the It's working for me over here
  2. This area seems glitchy because the method that's responsible for the update of the data set is
    ContactAdapter.notifyDataSetChanged(). I am implementing it but still the view in the MainAcitivity is
    not changing until lifecycle gets destroyed and resets
  3. No, The contacts do not get added again if you click on Import from phone twice
  4. I'm sorry about the whitespace. Android Studio was informing me again and again that the white
    space is 3 instead of 4 and I didn't know that I should keep it at 3.
  5. I couldn't find any way other than a SQLiteDB to do it, So I did it with that. Please inforrm me if you find a
    better way to do it :)
    6

-> Asking for permissions when the dialog invokes
-> Changed the Whitespace to 3 spaces
-> Added comments and made the code readable
->Added TODOs
->Removed the test Accounts ListPreference in  PreferenceScreen
-Added better ladder for Permissions checking
-Added a possible solution to ListView update bug
-Made the permissions checking Asynchronous
-Added TODOs to mention bugs and point out some left-out code
The images for the DTMF Keypad have been updated with better images and
the numbers have the corresponding letters below them now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants