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]

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 🎉

[oguzkocer]

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

[nbradbury]