Move publish to new screen by planarvoid · Pull Request #10183 · wordpress-mobile/WordPress-Android

planarvoid · 2019-07-09T08:42:54Z

This PR is the first step of the improved post scheduling. It introduces a new screen that appears when the user clicks on the "Publish" button in post settings.
In this PR you can click on the "Time and Date" to select a date from Date/Time picker and the label gets filled correctly. Going back to the Post settings sets the scheduled date.
I'll add tests in another PR.

To test:

Go to Post editor
Click on Menu/Post settings
Click on "Publish"
A new screen opens
Click on "Time and Date"
Select a Date from a date picker
Select a Time from a time picker
The "Time and Date" label is now set
Go back
The "Publish" field has now a label same as the "Time and Date"

Overall notice that the date selection behaviour should be the same as before.

Update release notes:

If there are user facing changes, I have added an item to RELEASE-NOTES.txt.

peril-wordpress-mobile · 2019-07-09T08:42:59Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

khaykov

Good job with new scheduling! Post status thing looks pretty complicated 😅 I have a couple of comments:

Post time is updated without pressing the update button. It's pretty easy to mess up stuff without even noticing.
What is the difference between Publish label and Scheduled? Also, what is the difference between IMMEDIATELY and NOW in date picker? Button said NOW, but I messed with time somehow and it says IMMEDIATELY.
Selecting future date, opening dialog again and selecting NOW/IMMEDIATELY sometimes results in Publish label and sometimes in Published. Doing it a couple of time cycles between those.

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPublishedSettingsFragment.kt

khaykov · 2019-07-10T03:22:42Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPublishedSettingsViewModel.kt

+                // having the app be smart about it - we don't want to accidentally publish a post.
+                finalPostStatus = DRAFT
+                // show toast only once, when time is shown
+                ToastUtils.showToast(


Should we move the Toast away from VM, into fragment?

totally, I actually did that in the following PR but I'll move the change here 👍

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPublishedSettingsFragment.kt

khaykov · 2019-07-10T04:05:37Z

Also, I found a crash, that I think might have been introduced in this branch (maybe something to do with ViewPager?), since I can't reproduce it in develop.
Steps to reproduce:

Open Post.
Open Post settings.
Navigate back to post.
Rotate device, and rotate it back again.
Observe the following crash:

31250-31250/org.wordpress.android.beta E/AndroidRuntime: FATAL EXCEPTION: main
    Process: org.wordpress.android.beta, PID: 31250
    kotlin.UninitializedPropertyAccessException: lateinit property viewModel has not been initialized
        at org.wordpress.android.ui.posts.HistoryListFragment.onSaveInstanceState(HistoryListFragment.kt:60)
        at androidx.fragment.app.Fragment.performSaveInstanceState(Fragment.java:2626)
        at androidx.fragment.app.FragmentManagerImpl.saveFragmentBasicState(FragmentManager.java:2910)
        at androidx.fragment.app.FragmentManagerImpl.saveAllState(FragmentManager.java:2971)
        at androidx.fragment.app.FragmentController.saveAllState(FragmentController.java:134)
        at androidx.fragment.app.FragmentActivity.onSaveInstanceState(FragmentActivity.java:591)
        at androidx.appcompat.app.AppCompatActivity.onSaveInstanceState(AppCompatActivity.java:510)
        at org.wordpress.android.ui.posts.EditPostActivity.onSaveInstanceState(EditPostActivity.java:807)
        at android.app.Activity.performSaveInstanceState(Activity.java:1414)
        at android.app.Instrumentation.callActivityOnSaveInstanceState(Instrumentation.java:1300)
        at android.app.ActivityThread.callCallActivityOnSaveInstanceState(ActivityThread.java:4541)
        at android.app.ActivityThread.handleRelaunchActivity(ActivityThread.java:4492)
        at android.app.ActivityThread.-wrap19(ActivityThread.java)
        at android.app.ActivityThread$H.handleMessage(ActivityThread.java:1483)
        at android.os.Handler.dispatchMessage(Handler.java:102)
        at android.os.Looper.loop(Looper.java:154)
        at android.app.ActivityThread.main(ActivityThread.java:6119)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

planarvoid · 2019-07-10T12:30:53Z

hey @khaykov, thanks for the review!

Post time is updated without pressing the update button. It's pretty easy to mess up stuff without even noticing.

I've checked the previous implementation and this behaves the same way. It's a pretty bad experience but I'd rather not change it now 😞

What is the difference between Publish label and Scheduled?

If I understand it correctly, Publish label is shown for local drafts with a created date set to the future where you haven't clicked the Schedule button yet

...what is the difference between IMMEDIATELY and NOW in date picker? Button said NOW, but I messed with time somehow and it says IMMEDIATELY.

Good question, I have no idea why it was introduced but we show the IMMEDIATELY status only when the post is a draft. However, when you select a date, the status changes to SCHEDULED and it's not draft any more so we show NOW. I don't think this is a great experience either.

Selecting future date, opening dialog again and selecting NOW/IMMEDIATELY sometimes results in Publish label and sometimes in Published. Doing it a couple of time cycles between those.

edit: I've found the issue and man, what a hack it was. Previously we were relying on updating the status twice after both date and time were set. The first set changed the status from Draft to Published and the second one from Published to Scheduled. Since I've removed the first update, the first iteration set the status from Draft to Published (and showed the "Published" label) and the second iteration from Published to Scheduled (and showed the "Publish" label)!!! It took me 3 hours to find this but it should be fixed now.

Also, I found a crash...
kotlin.UninitializedPropertyAccessException: lateinit property viewModel has not been initialized
at org.wordpress.android.ui.posts.HistoryListFragment.onSaveInstanceState(HistoryListFragment.kt:60)

I'm pretty sure this happens in the post History which was implemented recently and has nothing to do with my PR. It's strange it's not happening in develop. I've fixed it by initializing the view model in onCreate instead of onViewCreated

khaykov

Thanks for the changes! I haven't used post settings that munch and didn't realize the logic comes from there :)

I have one comment in code, and one here:
.

..what is the difference between IMMEDIATELY and NOW in date picker? Button said NOW, but I >messed with time somehow and it says IMMEDIATELY.

Good question, I have no idea why it was introduced but we show the IMMEDIATELY status only when the post is a draft. However, when you select a date, the status changes to SCHEDULED and it's not draft any more so we show NOW. I don't think this is a great experience either.

I compared the effect of changing the published post status (not the post date) to the button label with develop and there is one difference:
When you manually change post status from Publish to Draft, button in date picker reflects this change (switches from now to immediate and back). In this branch does not happen. Maybe changes to the post model not correctly picked up by Publish settings VM/Fragment?

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPublishSettingsFragment.kt

planarvoid · 2019-07-11T08:06:23Z

@khaykov thanks for the check! I was not indeed reacting to the post status change in the post settings. This should now be fixed. I think there should be a singleton class like PostProvider keeps the post and triggers the UI updates. I think I'll try to create it in a separate PR because this is a mess.

khaykov

Thanks for the fixes! Works well now :)

planarvoid added 3 commits July 9, 2019 10:32

Set published date from a new screen

80321cb

Remove Publish action from toolbar

91bdf84

Remove unused code

790ee03

planarvoid added the Posting/Editing label Jul 9, 2019

planarvoid added this to the 12.9 milestone Jul 9, 2019

planarvoid requested a review from khaykov July 9, 2019 08:42

planarvoid self-assigned this Jul 9, 2019

planarvoid added 3 commits July 9, 2019 10:44

Remove logs

1510ce3

Cleanup

6f18d09

Remove unused import

8483d93

khaykov suggested changes Jul 10, 2019

View reviewed changes

planarvoid added 2 commits July 10, 2019 13:56

Move toast into fragment

ff39fd3

Only show time dialog once

fe110e5

planarvoid added 3 commits July 10, 2019 15:06

Move view model initialization to onCreate to fix crash

8cbb77b

logs

35f1915

Fix label setting

45f7fef

khaykov reviewed Jul 11, 2019

View reviewed changes

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostPublishSettingsFragment.kt Show resolved Hide resolved

planarvoid added 2 commits July 11, 2019 09:46

Show toast only once

bd035ab

Update publish screen on post status change

fc24e7b

Fix lint issue

1329b12

wordpress-mobile deleted a comment from wpmobilebot Jul 11, 2019

khaykov approved these changes Jul 11, 2019

View reviewed changes

khaykov merged commit 5ca4848 into feature/post_scheduling_readonly Jul 11, 2019

shiki mentioned this pull request Aug 16, 2019

Crash on editor + History with potential data loss on 13.1 #10407

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move publish to new screen#10183

Move publish to new screen#10183
khaykov merged 14 commits intofeature/post_scheduling_readonlyfrom
feature/move_publish_to_new_screen

planarvoid commented Jul 9, 2019 •

edited

Loading

Uh oh!

peril-wordpress-mobile bot commented Jul 9, 2019 •

edited

Loading

Uh oh!

khaykov left a comment

Uh oh!

Uh oh!

khaykov Jul 10, 2019

Uh oh!

planarvoid Jul 10, 2019

Uh oh!

Uh oh!

khaykov commented Jul 10, 2019

Uh oh!

planarvoid commented Jul 10, 2019 •

edited

Loading

Uh oh!

khaykov left a comment

Uh oh!

Uh oh!

planarvoid commented Jul 11, 2019

Uh oh!

khaykov left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

planarvoid commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khaykov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

khaykov Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

planarvoid Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

khaykov commented Jul 10, 2019

Uh oh!

planarvoid commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

khaykov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

planarvoid commented Jul 11, 2019

Uh oh!

khaykov left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

planarvoid commented Jul 9, 2019 •

edited

Loading

peril-wordpress-mobile bot commented Jul 9, 2019 •

edited

Loading

planarvoid commented Jul 10, 2019 •

edited

Loading