My Profile

[oguzkocer]

I wanted to open this PR to get some early feedback. @tonyr59h and I were discussing that his "Site Settings" implementation might have some components we can use in "My Profile" as well. I am also not completely sure I have the best approach to a couple things.

The first question I had is about the layout. When we were working on the v1 of Calypsoification of the app with @nbradbury we opted not to use a list view for the Me fragment and I've taken a similar approach. I've used styles extensively here, so the code feels clear to me and there is not much need for a lot of switch statements. Having said that, it looks like "Site Settings" is implemented with list view, so I wanted to check in if I had the best approach for this.

My second question is about the usage of dialogs. I have looked into the WPAlertDialogFragment and it doesn't have a builder with an input in it. So for now, I have added the dialog inside the MyProfileActivity. Should I be looking to move that implementation to WPAlertDialogFragment instead? Also, note that the current dialog only works for "first name" and I thought I'd pass a tag to showInputDialog method and use a switch statement to make it work for all labels. I actually wanted to make it work with some kind of callback mechanism but couldn't really find a way to do that. Would that be possible? If not, I don't think the method would be generic enough to add inside of WPAlertDialogFragment.

For reference, here is the complete design for my profile page: https://cloudup.com/cGvJQYVBE0B And here are some screenshots from the actual implementation: https://cloudup.com/ciJn7zQjKfx

Anyone can review the PR, I am just ping Maxime to add a name here. Thanks! Needs review: @maxme

[maxme]

The first question I had is about the layout. When we were working on the v1 of Calypsoification of the app with @nbradbury we opted not to use a list view for the Me fragment and I've taken a similar approach. I've used styles extensively here, so the code feels clear to me and there is not much need for a lot of switch statements. Having said that, it looks like "Site Settings" is implemented with list view, so I wanted to check in if I had the best approach for this.

I think the only reasons to use a list view in that case are:

• Load data from an external source (network? db?)
• Have a huge list of similar options

If we don't have one of these criteria, it seems easier (and faster to make changes) to have a simple static layout.

My second question is about the usage of dialogs. I have looked into the WPAlertDialogFragment and it doesn't have a builder with an input in it. So for now, I have added the dialog inside the MyProfileActivity. Should I be looking to move that implementation to WPAlertDialogFragment instead? Also, note that the current dialog only works for "first name" and I thought I'd pass a tag to showInputDialog method and use a switch statement to make it work for all labels. I actually wanted to make it work with some kind of callback mechanism but couldn't really find a way to do that. Would that be possible? If not, I don't think the method would be generic enough to add inside of WPAlertDialogFragment.

Yep, we should have a generic "input" dialog somewhere. You can add it to WPAlertDialogFragment or maybe create a new GenericDialog utility class. It should be possible to pass a Callback interface with abstract method or even directly use a OnClickListener.

[nbradbury]

If we don't have one of these criteria, it seems easier (and faster to make changes) to have a simple static layout.

Agreed.

[oguzkocer]

Thank you both. @maxme the information will be retrieved from network and will probably be saved into the db (or somewhere), but it should be fairly static and it wouldn't change very often. If I am interpreting your comments correctly, it's still fine to go with the static view approach, right?

[nbradbury]

If I am interpreting your comments correctly, it's still fine to go with the static view approach, right?

Yes!

[oguzkocer]

Awesome!

[maxme]

Thank you both. @maxme the information will be retrieved from network and will probably be saved into the db (or somewhere)

Yes, I mean if the list structure itself comes from the network (in that case, you don't have much choices, static won't work ;)) - for instance: Notification Settings (list of blogs).

[oguzkocer]

Ahhh, I misunderstood that part, thanks for the correction!

[tonyr59h]

Have you considered using a PreferenceFragment for the layout? I think it would be simpler than using the custom my_profile_activity layout and you could take advantage of some existing work. You can re-use some of the custom preferences that are used in Site Settings which include WPEditTextPreference, WPSwitchPreference, DetailListPreference (for language picker), and WPPreference. You'd also get hint, styles, and custom fonts text for free.

This is the bulk of the relevant work on Settings.

[oguzkocer]

@tonyr59h I think that link is dead, but I checked Site Settings from here instead. It looks like it might be a good fit, thanks a lot for pointing it out. It's still a bit more work than it might worth it though, especially since the current approach is almost done. There are also some slight design differences, but I guess that's not a big deal since you're already using it in Site Settings and we can look into the slight changes together for consistency.

I am not sure which way to go, I'd love a second opinion. @nbradbury & @maxme what do you guys think? Do you think it'd be worth to give PreferenceFragment a try here?

[maxme]

I am not sure which way to go, I'd love a second opinion. @nbradbury & @maxme what do you guys think? Do you think it'd be worth to give PreferenceFragment a try here?

PreferenceFragment will help to write this kind of settings screen, only if you want to store data in SharedPreferences (ie. not in the DB, can't be queried). With a PreferenceFragment you basically just have to write the UI, persistence will be automatically managed.

It's less flexible than a plain fragment, and in the past we had some issues with styling (I don't think this is a problem anymore).

[oguzkocer]

This PR is mostly ready for review. There are only a couple things left to handle.

• I need to add analytics tracking and I've submitted a PR to add the necessary values. I am just waiting for that to get merged in.
• If you test it with a 2fa enabled account, the call for /me/settings will fail. We're working on whitelisting the apps to get around this, so no changes will be made on the PR for this.
• I need to add in some error handling for both GET & POST requests to /me/settings. What do we generally do for this? I've browsed the code quite a bit, but I am still not sure on how to handle it. I think we generally ignore the GET request and don't show an error to the user but we'll need to show something for the POST request. Should I just show a Toast with a generic message that says the profile couldn't be saved?

Once these are resolved the PR should be ready to merge. Since these won't need much change in the code, I think it'd be great if we can get the PR reviewed, so I can fix any issues come up and we can merge it sooner.

P.S: For our previous discussion on whether to use PreferenceFragment or not, after playing with it a bit, I've decided not to go with it. The reason I wanted to use a custom view was, we have an Account model in our db and I wanted to keep these information (first name, last name, about me) in the db within this model. I am going to work on Account Settings after this PR, so maybe I'll give it a go there, but I am not sure about that either.

[oguzkocer]

If you test it with a 2fa enabled account, the call for /me/settings will fail. We're working on whitelisting the apps to get around this, so no changes will be made on the PR for this.

This has been fixed on the api side, so only analytics and error handling issues left.

[oguzkocer]

Analytics has been added for both My profile and Account Settings screens. All that's left to add is error handling which we could discuss during the review. I am just not sure what to do with VolleyError and what kind of message we should show to the user. @maxme I am adding you as the reviewer, but anyone can review it. Feel free to remove yourself if you don't have the time :)

[nbradbury]

@oguzkocer I haven't reviewed the code yet, but I found quite a few problems while simply using My Profile:

• When I first visit My Profile, you can see the labels reposition themselves as their entries are filled in.
• Capitalization in labels should follow Google's recommendation and use sentence-style caps (ex: "First name" instead of "First Name").
• It seems odd to have the labels appear larger than the items below them. I think they should match the preference style seen in our account settings and notification settings.
• When I tap an item, the dialog doesn't include the existing entry. So, when I tap "First name" the dialog should include the existing first name (and have it selected so it's easy to delete).
• HTML entities aren't being handled correctly - for example, here's my About Me.
• If I haven't logged into wp.com but do have a self-hosted site, My Profile remains visible (but doesn't work).
• Related to the above, if I have a self-hosted site and disconnect from wp.com, My Profile remains visible.
• If I rotate the device while changes are being saved, the app crashes.
• If I change any entries and then back out without hitting SAVE, I lose my changes without warning.
• If I change my public display name and hit SAVE, it reverts to my previous display name.
• If I clear any entry and hit SAVE, it reverts to the original entry.
• The SAVE button is enabled even when I haven't changed anything.
• My two cents: I'd drop the SAVE button and change the OK button in the dialogs to SAVE. When the user taps SAVE in a dialog, it would save in the background silently (no progress dialog).
• As an aside, I think a lot of people will go to My Profile thinking they can change their avatar and be annoyed that they can't.

[oguzkocer]

Oh, that's a lot of issues. Thanks a lot for the detailed test @nbradbury & sorry about wasting your time, some of those issues should have been fixed before asking the review :( I'll start working on them tomorrow morning!

[oguzkocer]

HTML entities aren't being handled correctly - for example, here's my About Me. If I clear any entry and hit SAVE, it reverts to the original entry. If I change my public display name and hit SAVE, it reverts to my previous display name.

I've fixed the issues mentioned above.

When I tap an item, the dialog doesn't include the existing entry. So, when I tap "First name" the dialog should include the existing first name (and have it selected so it's easy to delete).

I have debated myself before sending this PR whether to add the existing entry or not. The reason I decided against it was due to my expectation for the user to change the record. I wasn't entirely sure on it, so I've changed it to @nbradbury 's suggestion.

When I first visit My Profile, you can see the labels reposition themselves as their entries are filled in. Capitalization in labels should follow Google's recommendation and use sentence-style caps (ex: "First name" instead of "First Name"). It seems odd to have the labels appear larger than the items below them. I think they should match the preference style seen in our account settings and notification settings.

The issues mentioned above are all related to the design I followed: https://cloudup.com/cGvJQYVBE0B I'd like to get @rickybanister 's opinion on them before I make any changes. A bit of context for the first issue:

The only time we should ever see that happening is when the user installs a fresh copy and visits the My Profile page for the first time. I agree that labels jumping around is not the best experience and assuming that most users have their my profile data filled in, I'd suggest we just leave the extra space for the labels even if there is nothing to show. But again, this shouldn't be an issue in most cases.

If I haven't logged into wp.com but do have a self-hosted site, My Profile remains visible (but doesn't work). Related to the above, if I have a self-hosted site and disconnect from wp.com, My Profile remains visible.

I've wanted to hide the My Profile label when the user is not logged in with WordPress.com and honestly I forgot to handle that case. Sorry about that! But before I go ahead and do that, @nbradbury & @rickybanister do you think that would be the best way to handle it?

If I rotate the device while changes are being saved, the app crashes.

@nbradbury after spending quite a bit of time, I've failed to reproduce this crash. I've tried it with both emulator and my S6. Can you reliably reproduce it? If so, any steps you could provide that I might be missing. It could also be related to your device and OS version, so if you could share those, that might help as well. Also, could you share the crash log? It might be enough to fix the issue.

As an aside, I think a lot of people will go to My Profile thinking they can change their avatar and be annoyed that they can't.

This is something we'd like to add as part of the Calypsoification of /me, but before we work on that, there are a couple pieces we'd like to take care of. Once those are done, it's in our todo list to add the ability to change your gravatar from My Profile page.

My two cents: I'd drop the SAVE button and change the OK button in the dialogs to SAVE. When the user taps SAVE in a dialog, it would save in the background silently (no progress dialog).

I like this suggestion a lot. @rickybanister what do you think?

If I change any entries and then back out without hitting SAVE, I lose my changes without warning. The SAVE button is enabled even when I haven't changed anything.

I'll fix both of these depending on if we'll get rid of the save button or not.

Looking forward to your feedback and thanks again @nbradbury for taking the time to throughly test the PR!

[nbradbury]

after spending quite a bit of time, I've failed to reproduce this crash. I've tried it with both emulator and my S6. Can you reliably reproduce it?

Yes, I can reproduce it every time (Nexus 6 Genymotion emulator running lollipop). Note that it only occurs if you rotate while the progress dialog is showing. It's easier to reproduce if you have a slow connection (or fake one using Genymotion). Here's the crash log:

12-15 13:43:09.055 8939-8939/org.wordpress.android E/AndroidRuntime: FATAL EXCEPTION: main
Process: org.wordpress.android, PID: 8939
java.lang.IllegalArgumentException: View=com.android.internal.policy.impl.PhoneWindow$DecorView{23a3d2d4 V.E..... R......D 0,0-1368,406} not attached to window manager
at android.view.WindowManagerGlobal.findViewLocked(WindowManagerGlobal.java:396)
at android.view.WindowManagerGlobal.removeView(WindowManagerGlobal.java:322)
at android.view.WindowManagerImpl.removeViewImmediate(WindowManagerImpl.java:116)
at android.app.Dialog.dismissDialog(Dialog.java:341)
at android.app.Dialog.dismiss(Dialog.java:324)
at org.wordpress.android.ui.me.MyProfileActivity.dismissProgressDialog(MyProfileActivity.java:147)
at org.wordpress.android.ui.me.MyProfileActivity.access$100(MyProfileActivity.java:20)
at org.wordpress.android.ui.me.MyProfileActivity$2.onSuccess(MyProfileActivity.java:110)
at org.wordpress.android.models.Account$5.onResponse(Account.java:76)
at org.wordpress.android.models.Account$5.onResponse(Account.java:69)
at com.wordpress.rest.RestRequest.deliverResponse(RestRequest.java:74)
at com.wordpress.rest.RestRequest.deliverResponse(RestRequest.java:18)
at com.android.volley.ExecutorDelivery$ResponseDeliveryRunnable.run(ExecutorDelivery.java:99)
at android.os.Handler.handleCallback(Handler.java:739)
at android.os.Handler.dispatchMessage(Handler.java:95)
at android.os.Looper.loop(Looper.java:135)
at android.app.ActivityThread.main(ActivityThread.java:5254)
at java.lang.reflect.Method.invoke(Native Method)
at java.lang.reflect.Method.invoke(Method.java:372)
at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:903)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:698)

The easiest fix here is to do away with the progress dialog :)

[nbradbury]

I've wanted to hide the My Profile label when the user is not logged in with WordPress.com and honestly I forgot to handle that case. Sorry about that! But before I go ahead and do that, @nbradbury & @rickybanister do you think that would be the best way to handle it?

Hiding My Profile in this situation makes sense to me.

[nbradbury]

@oguzkocer I updated my bug list above so that fixed problems are struck out (should make it easier to keep track of things). A few new ones to add:

• If I change my public display name and save it, when I return to the Me tab the old name still displays.
• If clear my public display name and save it, the old name returns the next time I go to My Profile. My guess is we don't want to allow people to clear it.
• Having MyProfileActivity under a new me directory isn't ideal (for one thing, it's where I'd first look for the Me fragment). Maybe it belongs under prefs instead, so that's where other settings are located?

[rickybanister]

@oguzkocer I actually intended the 'OK' button in the modal to be a save button. If we want to change that to 'SAVE' I think that is fine.

I didn't see the save button that @nbradbury mentioned in the screenshots, not sure what that refers to.

Regarding the size of the labels, I was following the material guidelines as far as I'm aware. Here is the example that informed that decision:

@mattmiklic helped me adhere to the guidelines, perhaps he could offer an opinion...

[nbradbury]

I didn't see the save button that @nbradbury mentioned in the screenshots, not sure what that refers to.

Looks like it's missing from the screenshots. In the actual implementation, the "My Profile" screen has a SAVE button in the top right which saves any changes. I'd love to drop this :)

Regarding the size of the labels, I was following the material guidelines as far as I'm aware. Here is the example that informed that decision

My bad, I was confusing those labels with the orange section headers we use in Settings. Sorry about that!

[mattmiklic]

The Cancel / OK labels are I believe the standard labels for dialogs: https://www.google.com/design/spec/components/dialogs.html#dialogs-confirmation-dialogs

In all of those examples, tapping OK saves the setting. So I think it makes sense to follow that standard (and it's what we're doing in Site Settings).

FWIW: in the screenshot above everything is in Roboto instead of Open Sans.

[oguzkocer]

Hmm.. I am either delusional or there was a time we had a navigation "Save" button on the top right and I just implemented it without thinking about it. I'll change the functionality. Sorry for the extra confusion!

@nbradbury I'll look into the crash again and follow up on the new issues you brought up.

[oguzkocer]

@nbradbury I've updated the PR to save the information when the dialog is closed #d77e214. For when you're doing the actual code review, I just wanted to say that I am not sure if my approach is the best way to handle this. I wanted to make the code as reusable as possible without making it too complex, let me know if you think there is a better way to handle it.

Even though silent updates are good for UX, handling slow connections and errors can be quite complicated. For the initial draft, I'm just accepting whatever the user inputs and updating the server. For error handling, I was just thinking to revert to the original value, but if the user has a slow connection and makes multiple updates, it might show the wrong information.

P.S: I know it'd probably the best to add these notes once all the bugs are fixed, but I have been really sick for the past couple days and I keep making small mistakes, forgetting stuff etc. Just wanted to make sure to add this note here before I forget.

[nbradbury]

I think this is much better now that the user doesn't have to tap SAVE. One issue, though: if I rotate the device while my changes are being saved, the display temporarily reverts to the previous entry.

I'll start looking at the code either tonight or tomorrow.

[nbradbury]

A note about the icons: although we don't use XXXHDPI icons yet, we should still have them. We store these in /WordPress/src/future/res/drawable-xxxhdpi so we have them all when we want to enable XXXHDPI icons.

[oguzkocer]

If clear my public display name and save it, the old name returns the next time I go to My Profile. My guess is we don't want to allow people to clear it.

@nbradbury When you clear your display name, it'll actually revert to your username in the server (at least that's what happens to me). This is also the case for Calypso. It feels weird though, I agree with you there. @rickybanister is this intentional, if not should we update this in Calypso as well?

[rickybanister]

This might be a core WordPress thing—let me ask someone. I'd say for now it's fine that it reverts since that's likely been the case for a very long time.

[oguzkocer]

Just to follow up on the display name showing the username in the Me screen when the display name is empty, I just realized (while I was fixing a different bug) that we're actually doing this in the client side. As far as I can tell the display name can be empty in the db, but we change it to username in the client side for both Calypso & Android app, and most likely in iOS as well.

String displayName = defaultAccount.getDisplayName();
if (!TextUtils.isEmpty(displayName)) {
    mDisplayNameTextView.setText(displayName);
} else {
    mDisplayNameTextView.setText(defaultAccount.getUserName());
}

[oguzkocer]

Fixed the XXXHDPI icons issue, you've already mentioned the path for it in your previous comment and I didn't notice the future in there, sorry about that!

[oguzkocer]

@nbradbury I think the PR is ready for another round of review, hopefully it'll be the last one. Every issue we have discussed should be handled. A few notes:

• @rickybanister thinks we should not leave empty space for where the information will come in. The labels will rearrange themselves, most likely only once. However, as per your recommendation they'll do so with an animation, so it should be fine.
• I've left a comment about the alternative approach for updating the server. That's probably the last decision we need to make.
• Now that we removed the dialog for updating the server, I don't think the crash is an issue anymore. However, while testing the crash, I found that you couldn't see "About Me" in the landscape mode for some devices, so I wrapped the layout within a ScrollView.

[nbradbury]

This is looking good, @oguzkocer! I think we can deal with lack of connectivity as a separate issue. If you're okay with that, I'm ready to merge :tada:

[oguzkocer]

Sure thing, thanks a lot for the review @nbradbury!

[nbradbury]

:shipit: