Closed tonyr59h closed 8 years ago
aforcier
commented
8 years ago Following our conversation, I gave the number picker a quick test on an API 18 Samsung S3 to check for OEM landmines on their part (since reflection was used for styling and the internals may have been changed).
Everything seems to work, no crashes. :+1:
There's only a border issue that's likely API-level related and not specific to the Samsung:
mattmiklic
commented
8 years ago It's looking good now; most of my comments are fairly minor at this point. Roughly in order of importance:
On all dialogs that include just a text input (site title, tagline, adding items to the moderation/blacklist, etc.); when the dialog appears the input is focused but the keyboard isn't visible; you have to tap the input to bring up the keyboard. Could we show the keyboard automatically?
On the Hold for Moderation and Blacklist screens, if I select two or more items and tap the Delete icon, only one item is deleted.
On screens where we include a picker, ideally the borders will appear directly above and below the picker, separating it from any other text or header in the dialog. Here are two examples (these screens also have a few font issues I have noted in red):
The Discussion settings screen has a few items that are in Roboto instead of Open Sans, and the border color is off.
mattmiklic
commented
8 years ago I just noticed one bigger issue; "Close after" is missing the toggle switch to turn that setting on or off.
tonyr59h
commented
8 years ago Thanks for your feedback @daniloercoli, all your points are addressed in 9383f4f7c680b335f52f4fe3266b1b67892325fa
The design has been updated based on your response @mattmiklic. The one issue I haven't fixed is the Discussion dialog dividers, it's still on my list though :+1:
@kwonye all the functionality is in now. It's ready for a developer review :)
daniloercoli
commented
8 years ago Not related to this PR, but I'm getting a crash in site settings for a .com test site here: https://github.com/wordpress-mobile/WordPress-Android/blob/9383f4f7c680b335f52f4fe3266b1b67892325fa/WordPress/src/main/java/org/wordpress/android/ui/prefs/DotComSiteSettings.java#L310
params.put(MODERATION_KEYS_KEY, builder.substring(0, builder.length() - 1));
builder.length() is equal to 1 in my case.
daniloercoli
commented
8 years ago Also getting a crash If I change a site setting and then immediately go back to the main site screen. Seems that the error handler doesn't check if the fragment is still attached to the activity.
Not sure why the error handler is called from fetchCategories->onResponse method. Seems that the issue is the call to notifyUpdatedOnUiThread(null) that in order go to protected void notifyUpdatedOnUiThread(final Exception error) {
12-10 10:03:03.125 5048-5048/org.wordpress.android E/AndroidRuntime: FATAL EXCEPTION: main
Process: org.wordpress.android, PID: 5048
java.lang.IllegalStateException: Fragment SiteSettingsFragment{c92018c} not attached to Activity
at android.app.Fragment.getResources(Fragment.java:788)
at org.wordpress.android.ui.prefs.SiteSettingsFragment.setPreferencesFromSiteSettings(SiteSettingsFragment.java:625)
at org.wordpress.android.ui.prefs.SiteSettingsFragment.onSettingsUpdated(SiteSettingsFragment.java:383)
at org.wordpress.android.ui.prefs.SiteSettingsInterface$1.run(SiteSettingsInterface.java:501)
at android.app.Activity.runOnUiThread(Activity.java:5247)
at org.wordpress.android.ui.prefs.SiteSettingsInterface.notifyUpdatedOnUiThread(SiteSettingsInterface.java:498)
at org.wordpress.android.ui.prefs.DotComSiteSettings$5.onResponse(DotComSiteSettings.java:341)
at org.wordpress.android.ui.prefs.DotComSiteSettings$5.onResponse(DotComSiteSettings.java:329)
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:5221)
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:899)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:694)Not related to this PR, but I'm getting a crash in site settings for a .com test site here
I'd say it's related ;) Fixed in 4b9a7e09fef44ca093a47726e1b37a6f77bac26d
tonyr59h
commented
8 years ago Also getting a crash If I change a site setting and then immediately go back to the main site screen.
I think 678ce0864c382b81e82fae6f34cb14c77be8b014 should address this one. The Activity is checked to make sure it's not finishing before calling the listener and the fragment makes sure it's added before updating.
mattmiklic
commented
8 years ago Hey Tony, just took a look and have a few things to mention:
All of this is on a Nexus 5 (let me know if you need any more info on my setup for the crashes).
kwonye
commented
8 years ago Regarding bcee50e, are we effectively disabling settings for self-hosted (4.5 isn't out yet)? If so, I'd like to see the settings field disabled instead of what it does now, which is just kick you out of the screen immediately and say there was an error.
Update: We cannot disable that field because the settings activity is the only place to remove the site from the app.
kwonye
commented
8 years ago WPSwitch:
tonyr59h
commented
8 years ago Can't turn on related posts / Default format is still not showing up even for WordPress.com sites
Both of these are working on my end. I'll sync up tomorrow morning so we can go through it. In the meantime can you post your test device specs?
kwonye
commented
8 years ago Both of these are working on my end. I'll sync up tomorrow morning so we can go through it. In the meantime can you post your test device specs?
Interesting! I'd imagine others didn't have issues either because they didn't report it. I'll investigate.
Device was Nexus 6P, 6.0
tonyr59h
commented
8 years ago Good catch with the dialog switches @kwonye. Are you still seeing the default format not set originally? I've tried on two sites and can't reproduce. For the first site I did a fresh install to make sure there was no cached data.
kwonye
commented
8 years ago Are you still seeing the default format not set originally? Yes, @mattmiklic and @daniloercoli do you guys mind seeing if it works on your end please?
When opening settings, the Default Format is not set.
kwonye
commented
8 years ago Also, for sites where I do not have access, do not show settings. Right now it is shown and it just kicks me out saying "couldn't retrieve site info". (Only sites where I am an administrator can settings be changed)
It is like this on the web: (Here I am an editor)
tonyr59h
commented
8 years ago Also, for sites where I do not have access, do not show settings.
Good idea! Added in 8de264efe19c4620f983d8cf2de6a6ed57fc64f6
tonyr59h
commented
8 years ago @kwonye I just went through a full-pass on WP.com, starting with a fresh install. I'm still unable to reproduce the default post format bug you're seeing. Does it save any changes you make to it? If so then I don't think we should consider it a blocking issue, barring anyone else seeing it.
I tested with HTC One M8, Android 5.1.
daniloercoli
commented
8 years ago Default post formats crash
12-15 09:33:42.978 3224-3224/org.wordpress.android E/AndroidRuntime: FATAL EXCEPTION: main
Process: org.wordpress.android, PID: 3224
java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.Object java.util.Map.get(java.lang.Object)' on a null object reference
at org.wordpress.android.ui.prefs.SiteSettingsInterface.getDefaultPostFormatDisplay(SiteSettingsInterface.java:218)
at org.wordpress.android.ui.prefs.SiteSettingsFragment.onPreferenceChange(SiteSettingsFragment.java:400)
at android.preference.Preference.callChangeListener(Preference.java:928)
at org.wordpress.android.ui.prefs.DetailListPreference.access$500(DetailListPreference.java:30)
at org.wordpress.android.ui.prefs.DetailListPreference$DetailListAdapter.changeSelection(DetailListPreference.java:279)
at org.wordpress.android.ui.prefs.DetailListPreference$DetailListAdapter.access$400(DetailListPreference.java:217)
at org.wordpress.android.ui.prefs.DetailListPreference$DetailListAdapter$2.onClick(DetailListPreference.java:266)
at android.view.View.performClick(View.java:4756)
at android.view.View$PerformClick.run(View.java:19749)
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:5221)
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:899)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:694)The crash above happens when Post Formats are not yet retrieved from the server:
Steps to repro:
mattmiklic
commented
8 years ago Took a look at the latest and it's looking pretty good. Noticed a few things:
I tried toggling every setting on my dotcom test blog and it worked great. The perception of speed is really enhanced now that it saves immediately for each option.
tonyr59h
commented
8 years ago The perception of speed is really enhanced now that it saves immediately for each option.
Awesome :) Now to mimic that sense of speed on self-hosted...
Good testing @daniloercoli, I never saw that crash. It's probably best to disable all preferences until we get all the remote data.
tonyr59h
commented
8 years ago Recent commits address all of Matt's comments. @kwonye ready for another go.
daniloercoli
commented
8 years ago Getting an issue with Default Category. The modal could be empty under some circumstances (slow network connection):
daniloercoli
commented
8 years ago Another round of testing on getPostFormats.
The app does a 1st call to wp.getPostFormats as soon as the user switches site in the site picker, or when synching blog content in background/AsynchTask. In this 1st case the XML-RPC call made to the server is the following:
<methodCall>
<methodName>wp.getPostFormats</methodName>
<params>
<param>
<value>
<i4>15592836</i4>
</value>
</param>
<param>
<value>
<string>daniloercoli</string>
</value>
</param>
<param>
<value>
<string />
</value>
</param>
<param>
<value>
<string>show-supported</string>
</value>
</param>
</params>
</methodCall>Note that the parameter show-supported is missing from its value.
The response is the following:
<methodResponse>
<params>
<param>
<value>
<struct>
<member>
<name>standard</name>
<value>
<string>Standard</string>
</value>
</member>
<member>
<name>aside</name>
<value>
<string>Aside</string>
</value>
</member>
<member>
<name>chat</name>
<value>
<string>Chiacchierare</string>
</value>
</member>
<member>
<name>gallery</name>
<value>
<string>Gallery</string>
</value>
</member>
<member>
<name>link</name>
<value>
<string>Link</string>
</value>
</member>
<member>
<name>image</name>
<value>
<string>Immagine</string>
</value>
</member>
<member>
<name>quote</name>
<value>
<string>Quote</string>
</value>
</member>
<member>
<name>status</name>
<value>
<string>Status</string>
</value>
</member>
<member>
<name>video</name>
<value>
<string>Video</string>
</value>
</member>
<member>
<name>audio</name>
<value>
<string>Audio</string>
</value>
</member>
</struct>
</value>
</param>
</params>
</methodResponse>The server replied ignoring the show-supported parameter.
The 2nd call to wp.getPostFormats is made when the user taps on the Settings menu item.
In this case the call is correctly made to the server, including the value for the parameter show-supported.
<methodCall>
<methodName>wp.getPostFormats</methodName>
<params>
<param>
<value>
<i4>15592836</i4>
</value>
</param>
<param>
<value>
<string>daniloercoli</string>
</value>
</param>
<param>
<value>
<string />
</value>
</param>
<param>
<value>
<struct>
<member>
<name>show-supported</name>
<value>
<string>true</string>
</value>
</member>
</struct>
</value>
</param>
</params>
</methodCall>And the response is the following:
<methodResponse>
<params>
<param>
<value>
<struct>
<member>
<name>all</name>
<value>
<struct>
<member>
<name>standard</name>
<value>
<string>Standard</string>
</value>
</member>
<member>
<name>aside</name>
<value>
<string>Aside</string>
</value>
</member>
<member>
<name>chat</name>
<value>
<string>Chiacchierare</string>
</value>
</member>
<member>
<name>gallery</name>
<value>
<string>Gallery</string>
</value>
</member>
<member>
<name>link</name>
<value>
<string>Link</string>
</value>
</member>
<member>
<name>image</name>
<value>
<string>Immagine</string>
</value>
</member>
<member>
<name>quote</name>
<value>
<string>Quote</string>
</value>
</member>
<member>
<name>status</name>
<value>
<string>Status</string>
</value>
</member>
<member>
<name>video</name>
<value>
<string>Video</string>
</value>
</member>
<member>
<name>audio</name>
<value>
<string>Audio</string>
</value>
</member>
</struct>
</value>
</member>
<member>
<name>supported</name>
<value>
<array>
<data>
<value>
<string>aside</string>
</value>
<value>
<string>image</string>
</value>
<value>
<string>video</string>
</value>
<value>
<string>link</string>
</value>
<value>
<string>quote</string>
</value>
<value>
<string>gallery</string>
</value>
<value>
<string>status</string>
</value>
<value>
<string>audio</string>
</value>
</data>
</array>
</value>
</member>
</struct>
</value>
</param>
</params>
</methodResponse>I bet we can unify the 2 calls above, and reuse the response from the first call if already there, and speed up the first setup of the site settings UI. (We already have the postFormats in the db and the call to upgrade it could be postponed in favour of other settings calls).
kwonye
commented
8 years ago kwonye
commented
8 years ago self-hosted issue resolved with #3562
tonyr59h
commented
8 years ago Was @daniloercoli's comment addressed?
Addressed in 46eb536402d80744ed7ba3221ab68b98850c4759
kwonye
commented
8 years ago Manual testing all looks good! :shipit:
This PR will cover #3434, #3441, and #3477.
Not ready for developer review, opening this for @mattmiklic to verify that the design is correct and to visualize the diff for myself.Edit: All the issues should be covered now, ready for review!