Issue/5681 discard changes action #8005

theck13 · 2018-07-07T15:42:51Z

Fix

Add action to overflow menu to discard local changes and snackbar with undo action to revert discarded changes to post editor and preview to address #5681. See the screenshots and animation below for illustration.

Test

Go to My Site tab.
Tap Blog Posts item under Publish section.
Tap post with local changes.
Notice local changes in post editor.
Tap overflow menu action.
Tap Discard Local Changes action.
Notice local changes are removed in post editor.
Notice Local changes discarded snackbar with Undo action.
Tap Undo action.
Notice local changes are added in post editor.

…nd preview post

mzorz · 2018-07-09T20:17:26Z

Thanks for taking care of this ❤️ - code looks good @theck13 !

As with each time one comes back to EditPostActivity code and posts state flow, your PR made me wonder and thoroughly consider which each of the states for a Post are and which transformations can get us from one state to the other.

These are my findings so far:

SITUATION A: inconsistencies between undo/discard changes in Aztec

Should we maybe only show the discard local changes options when there's history in Aztec that can be undone?

Open an existing post
observe the overflow menu (correctly) does not have the "Discard local changes" option
enter a new character
observe the overflow menu (correctly) now shows the "Discard local changes" option
do not tap on that new option, but tap on Undo.
observe the action is undone and the content is exactly as what we had earlier
tap on the overflow menu icon again
observe the "Discard local changes" option is shown, but it shouldn't (there are in theory no local changes to be discarded).

This is because we're relying on the isLocallyChanged flag to reflect current state, but it's not being changed when there's nothing else to undo.
I thought of only showing the Discard local changes option when there was a valid "undo" history to use, but that also shows a false positive when editing a draft with local changes (i.e. the Discard local changes option should be visible right away upon opening the draft, but it doesn't show because we can't tell whether it was already like that before opening or not).

I tried something in this branch (try/check-undo-to-show-discard): checking the locallyChanged flag status of the Original post when opening the Editor. Then that way we know for sure the changes were there already before opening, and thus show the Discard Local Changes menu option when there's no other way to know (i.e. no change history). Wdyt?

In any case, this is probably an edge case and I don't think it's a blocker for this PR; we may continue to tackle this on a separate PR.

SITUATION B: "Discarding local changes" progress dialog sitting there forever on rotation

This one is known and happens in many other cases in the app, as it's based on how FluxC works (EventBus based). The expected end event is missed and so the progress dialog never gets dismissed.
When you hit "discard local changes" and then immediately rotate the screen, the progress dialog is going to be there forever.
Probably something we need to talk about in a more dedicated way, so as in the past not going to consider it a blocker for this PR; just leaving a note here that I was able to reproduce the same behavior here.

SITUATION C: "The post has local changes that haven't been published"

I couldn't find the situation depicted in the last 3 screenshots up here within the code, is that correct? (tried the string search trick and then read the code diff in the PR but couldn't find the behavior there either).

mzorz

Left more verbose comments in the PR itself, but generally approving this PR 👍 Feel free to merge provided a follow up to case A and an answer on case C is given

theck13 · 2018-07-10T05:38:47Z

Should we maybe only show the discard local changes options when there's history in Aztec that can be undone?

I didn't think about history/undo/redo and the steps you provided is a scenario I didn't consider. Thanks for pointing it out! @SylvesterWilmott, what's your opinion on limiting the ability to discard local changes only when there is something to undo?

I couldn't find the situation depicted in the last 3 screenshots up here within the code, is that correct?

Ahh, I should have mentioned that's the post preview. The string changes are minimal in post_preview_activity.xml and the discarding logic is in PostPreviewActivity.java. Follow the steps below to see the screenshots above.

Go to My Site tab.
Tap Blog Posts item under Publish section.
Tap Preview button on a post with local changes.
Notice snackbar with Discard Changes and Publish actions.

SylvesterWilmott · 2018-07-10T11:09:16Z

what's your opinion on limiting the ability to discard local changes only when there is something to undo?

The primary use case here is when a user is editing a post, presses back and local changes are saved with no clear way to discard the changes. Therefore the "Discard Changes" menu item in the editor is present when the "Local Changes" label is present on the post list. So currently:

Open existing post
Type a character
Press back and observe "Local Changes" label on the post list
Open post
Remove the character
Press back and observe "Local Changes" label on the post list

and

Open existing post
Type a character
Press Undo
Press back and observe "Local Changes" label on the post list

Alternatively, If you want to touch on the logic of the "Local Changes" post status. Then we will need conditions

IF
Post contains no pre-existing local changes
AND
Post has no undo history
THEN
Do not apply "Local Changes" status to post in post list
Hide "Discard Changes" menu item

Design-wise this looks great. Are we able to add more space between the text string and the buttons in the 2 button Snackbar as shown here

Thanks @theck13 !

mzorz · 2018-07-10T14:15:17Z

Thanks for providing the detailed steps to test the Preview section @theck13 ! was able to test following that.

Thanks for your input @SylvesterWilmott, several things in there!, I'll separate on points to make a distinction for each case, so it's easier to handle and take action:

1. Discard Changes main case

Therefore the "Discard Changes" menu item in the editor is present when the "Local Changes" label is present on the post list.

That is one thing - this PR works well in that case, agreed on that and I'd give this PR the go just for this alone 👍 (actually, already approved it).

2. When does the `local changes` status apply?

I just wanted to make a correction to these steps though:

Open existing post

Type a character

Press back and observe "Local Changes" label on the post list

Open post

Remove the character

Press back and observe "Local Changes" label on the post list

As of my tests, point 6 here is only true when there's a lack of connectivity that prevents the Post from being sent to the server. Otherwise, the app always tries to upload the latest version of the Post to the server, and in case that successfully happens it ends up having the Draft label in the posts list most of the times.

3. Nothing to be undone / still Local changes detected situation (posts list)

This other thing here:

Open existing post

Type a character

Press Undo

Press back and observe "Local Changes" label on the post list

Just verified and this is how it's working in develop, but in any case I think that's a bug that should be solved elsewhere. I mean, making no changes to the content should not end in a new copy of the Post sent to the server.
Whether this is an important bug or not is debatable ofc :) but that's another thing.
Issue filed #8009

4. Nothing to be undone / still Local changes detected situation (Editor)

Now, without exiting the Editor, for the same reasons above, we shouldn't show a Discard local changes option if there is nothing to undo in the Editor, unless the Post we're editing already had local changes that can be discarded at the time of opening.
Issue filed #8010

mzorz · 2018-07-10T14:18:35Z

Having tracked the issues detected while testing this,

malinajirka

Thank you @theck13 for this awesome improvement;)! Adding something into the EditPostActivity is always a challenge, but you did a great job!

I know this PR has recently been merged, but I'd like to share couple comments I didn't post in time:). I'll leave it up to you, whether you want to fix them or leave it as it is. Thanks!

malinajirka · 2018-07-10T12:04:42Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

+    }
+
+    private void showDialogError() {
+        new AlertDialog.Builder(new ContextThemeWrapper(this, R.style.Calypso_Dialog_Alert))


We might consider using BasicFragmentDialog, which automatically handles configuration changes. Wdyt?

That sounds good to me. I'll update that in a subsequent pull request since this one has been merged.

malinajirka · 2018-07-10T12:12:17Z

WordPress/src/main/java/org/wordpress/android/ui/posts/EditPostActivity.java

+                    refreshEditorContent();
+
+                    if (mViewPager != null) {
+                        Snackbar.make(mViewPager, getString(R.string.local_changes_discarded), Snackbar.LENGTH_LONG)


Users using TalkBack or SwitchControl might have troubles to locate the SnackBar (undo button) before it's dismissed -> we should probably use AccessibilityUtils.getSnackbarDuration(...) method. Wdyt?

Ahh, I always forget about AccessibilityUtils. Thanks for the reminder. I'll update that in a subsequent pull request since this one has been merged.

malinajirka · 2018-07-10T12:12:35Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostPreviewActivity.java

                    mPost = mPostStore.getPostByLocalPostId(mPost.getId());
                    refreshPreview();
+
+                    if (mMessageView != null) {
+                        Snackbar.make(mMessageView, getString(R.string.local_changes_discarded), Snackbar.LENGTH_LONG)


Same comment as in EditPostActivity.java

Same comment as in EditPostActivity.java. I'll update that in a subsequent pull request since this one has been merged.

malinajirka · 2018-07-10T12:16:37Z

WordPress/src/main/java/org/wordpress/android/ui/posts/PostPreviewActivity.java

@@ -326,20 +359,69 @@ private void hideProgress() {
        }
    }

+    private void showDialogError() {
+        new AlertDialog.Builder(new ContextThemeWrapper(this, R.style.Calypso_Dialog_Alert))


Same comment as in EditPostActivity.java

Same comment as in EditPostActivity.java. I'll update that in a subsequent pull request since this one has been merged.

theck13 · 2018-07-10T16:07:57Z

Design-wise this looks great. Are we able to add more space between the text string and the buttons in the 2 button Snackbar as shown here.

@SylvesterWilmott, are you referring to the 18dp between the message text and action button in this snackbar? We can do that. If it's something else, let me know. If you would like, I can create an issue and address that separately since this pull request has already been merged.

@mzorz, thanks for the review and filing the issues!

@malinajirka, thanks for the tips for improvements! I'll make those updates in a subsequent pull request.

SylvesterWilmott · 2018-07-10T16:10:26Z

@theck13 Yes, it's the 18dp space. Thanks 🙂

theck13 added 21 commits July 3, 2018 18:14

Move publish to primary action in post preview snackbar

029f711

Update revert to discard changes in post preview snackbar

744ba4e

Update color of changes icon and text to alert yellow dark in post list

22efff9

Add discard changes item to edit post menu

b07d416

Update post preview snackbar message layout parameters for API 17+

ec37198

Update account settings snackbar action from revert to discard

a6c6456

Add missing punctuation to deleting site error message

db3fff1

Remove custom snackbar background and action color from account settings

4c2d01f

Add discard changes action to edit post

e593ef9

Add discard changes action to preview post

aa61e5c

Add undo discard changes action to edit post

4de14ec

Add undo discard changes action to preview post

ffa3e6c

Add progress dialog for edit post discard changes action

bcb8181

Add error dialog for edit post discard changes action

e994c59

Add help origin to error dialog action for edit post discard changes

c3ef800

Add analytics tracking for discard changes action to edit post

61c1e6d

Add analytics tracking for undo discard changes action to edit post a…

01dedd3

…nd preview post

Add error dialog for preview post discard changes action

1a7221f

Remove redundant view casts from post preview activity class

d1f40a9

Remove redundant view casts from account settings fragment class

d0b3116

Remove redundant view casts from post list adapter class

d4059dd

theck13 added [Type] Enhancement Mobile Request labels Jul 7, 2018

theck13 added this to the 10.5 milestone Jul 7, 2018

theck13 requested review from malinajirka, mzorz and aforcier July 7, 2018 15:43

mzorz self-assigned this Jul 9, 2018

mzorz approved these changes Jul 9, 2018

View reviewed changes

mzorz mentioned this pull request Jul 10, 2018

Aztec / Discard changes functionality: don't show discard local changes if no changes to undo #8010

Closed

mzorz merged commit 9c77ae2 into develop Jul 10, 2018

mzorz deleted the issue/5681-discard-changes-action branch July 10, 2018 14:18

malinajirka reviewed Jul 10, 2018

View reviewed changes

mzorz mentioned this pull request Jul 10, 2018

Aztec: match undo and discard local changes options #8011

Merged

theck13 mentioned this pull request Jul 10, 2018

Discard Local Changes Additions #8012

Closed

theck13 mentioned this pull request Jul 22, 2018

Revert local changes #5681

Closed

rachelmcr mentioned this pull request Jul 24, 2020

Feature Request: discard local changes #12505

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue/5681 discard changes action #8005

Issue/5681 discard changes action #8005

theck13 commented Jul 7, 2018

mzorz commented Jul 9, 2018

mzorz left a comment

theck13 commented Jul 10, 2018

SylvesterWilmott commented Jul 10, 2018 •

edited

Loading

mzorz commented Jul 10, 2018

mzorz commented Jul 10, 2018

malinajirka left a comment

malinajirka Jul 10, 2018

theck13 Jul 10, 2018

malinajirka Jul 10, 2018

theck13 Jul 10, 2018

malinajirka Jul 10, 2018

theck13 Jul 10, 2018

malinajirka Jul 10, 2018

theck13 Jul 10, 2018

theck13 commented Jul 10, 2018

SylvesterWilmott commented Jul 10, 2018

Issue/5681 discard changes action #8005

Issue/5681 discard changes action #8005

Conversation

theck13 commented Jul 7, 2018

Fix

Test

mzorz commented Jul 9, 2018

SITUATION A: inconsistencies between undo/discard changes in Aztec

SITUATION B: "Discarding local changes" progress dialog sitting there forever on rotation

SITUATION C: "The post has local changes that haven't been published"

mzorz left a comment

Choose a reason for hiding this comment

theck13 commented Jul 10, 2018

SylvesterWilmott commented Jul 10, 2018 • edited Loading

mzorz commented Jul 10, 2018

1. Discard Changes main case

2. When does the local changes status apply?

3. Nothing to be undone / still Local changes detected situation (posts list)

4. Nothing to be undone / still Local changes detected situation (Editor)

mzorz commented Jul 10, 2018

malinajirka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theck13 commented Jul 10, 2018

SylvesterWilmott commented Jul 10, 2018

SylvesterWilmott commented Jul 10, 2018 •

edited

Loading

2. When does the `local changes` status apply?