Closed Bug 952484 Opened 11 years ago Closed 10 years ago

[Messages][Drafts] We should not show the "replace draft" screen if the user has not changed the draft

Categories

(Firefox OS Graveyard :: Gaia::SMS, defect)

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: julienw, Assigned: evhan55)

Details

Attachments

(1 file)

https://github.com/mozilla-b2g/gaia/pull/15563
46 bytes, text/plain
rwaldron
: review+
julienw
: feedback+
Details 
STR:
1. open an existing thread
2. enter some text in the composer
3. press back, save draft
4. enter the thread again
5. press back

Expected:
* we don't have a confirmation panel, because this is essentially like we leave an empty composer

Actual:
* we have the "replace draft" panel

That said, I worry because the panel has a also a "discard" option, so if we don't display the panel we will lose that option as well. Not sure this is worth keeping the panel though. I got this when I tested and I felt it was weird.

Ayman, what's your advice?
Flags: needinfo?(aymanmaat)
blocking-b2g: --- → 1.4?
Assignee: nobody → evelyn
Are we still waiting on Ayman for this?
I think that we still are, yes.

Last week was filled with meetings to prepare 1.4 but he's aware he has some questions to answer.
(In reply to Julien Wajsberg [:julienw] from comment #2)
> I think that we still are, yes.
> 
> Last week was filled with meetings to prepare 1.4 but he's aware he has some
> questions to answer.

Ok, thanks!
FWIW, this is an awful DX (I made that up, it's "Developer Experience"). When testing Drafts, we're regularly going back and forth between thread list view and a thread or a threadless draft; if I've changed nothing about the draft's state (recipients, subject, content), it feels like the app is trolling* me when it presents the "Replace existing Draft" confirmation. 



* http://knowyourmeme.com/memes/subcultures/trolling
(In reply to Julien Wajsberg [:julienw] from comment #0)
> STR:
> 1. open an existing thread
> 2. enter some text in the composer
> 3. press back, save draft
> 4. enter the thread again
> 5. press back
> 
> Expected:
> * we don't have a confirmation panel, because this is essentially like we
> leave an empty composer
> 
> Actual:
> * we have the "replace draft" panel
> 
> That said, I worry because the panel has a also a "discard" option, so if we
> don't display the panel we will lose that option as well. Not sure this is
> worth keeping the panel though. I got this when I tested and I felt it was
> weird.
> 
> Ayman, what's your advice?

Yep its annoying as hell so the screen in this situation can be dropped. The user can always delete the draft by manually removing the content
Flags: needinfo?(aymanmaat)
(In reply to ayman maat :maat from comment #5)
> (In reply to Julien Wajsberg [:julienw] from comment #0)
> > STR:
> > 1. open an existing thread
> > 2. enter some text in the composer
> > 3. press back, save draft
> > 4. enter the thread again
> > 5. press back
> > 
> > Expected:
> > * we don't have a confirmation panel, because this is essentially like we
> > leave an empty composer
> > 
> > Actual:
> > * we have the "replace draft" panel
> > 
> > That said, I worry because the panel has a also a "discard" option, so if we
> > don't display the panel we will lose that option as well. Not sure this is
> > worth keeping the panel though. I got this when I tested and I felt it was
> > weird.
> > 
> > Ayman, what's your advice?
> 
> Yep its annoying as hell so the screen in this situation can be dropped. The
> user can always delete the draft by manually removing the content

Thanks for the follow up, we'll get this fixed immediately :)
I can see two ways to approach this:

1) Test that MessageManager.draft.content and Composer.getContent() are equal
2) Set a 'dirty' flag whenever the user has started editing the subject, recipients or content fields

In order to test for equality of the current MessageManager.draft content array and whatever is in Composer.getContent(), we may need to bring in an implementation of 'isEqual'.  I believe this is the only way to do a deep equality check of these two content arrays.  There is currently no 'isEqual' implementation in all of gaia and I would probably bring in an external implementation and cite the source.

A 'dirty' flag is easier to implement, but still results in possible 'Replace existing draft?' dialog if the recipients, subject and content don't actually change even if the user started editing them.

Thoughts?
I may be missing other options.
Flags: needinfo?(felash)
Flags: needinfo?(aymanmaat)
I think it's ok to have the 'replace draft' panel as soon as the user edited the draft, even if the content is not changed in the end (eg: add a character, remove the character; add an attachment, remove the attachment).

Simpler is better :)
Flags: needinfo?(felash)
fine by me
Flags: needinfo?(aymanmaat)
Cases:

- edit message
  - add/remove character
- edit subject
  - add/remove character
- edit attachment
  - add/remove attachment
- edit recipients
  - add/remove recipient
(In reply to Evelyn Eastmond [:evhan55] from comment #10)
> Cases:
> 
> - edit message
>   - add/remove character
> - edit subject
>   - add/remove character
> - edit attachment
>   - add/remove attachment
> - edit recipients
>   - add/remove recipient

keyup for message, subject and recipients?
(In reply to Evelyn Eastmond [:evhan55] from comment #11)
> (In reply to Evelyn Eastmond [:evhan55] from comment #10)
> > Cases:
> > 
> > - edit message
> >   - add/remove character
> > - edit subject
> >   - add/remove character
> > - edit attachment
> >   - add/remove attachment
> > - edit recipients
> >   - add/remove recipient
> 
> keyup for message, subject and recipients?

Compose.onContentChanged
If this looks ok to you, Julien, then I will work on the unit tests.

FWIW, Rick was in favor of using isEqual instead of this approach. He asked me to make the decision, but this implementation seemed simpler overall.
Attachment #8362592 - Flags: feedback?(felash)
Comment on attachment 8362592 [details]
https://github.com/mozilla-b2g/gaia/pull/15563

Gave the feedback over IRC yesterday.
Attachment #8362592 - Flags: feedback?(felash)
Comment on attachment 8362592 [details]
https://github.com/mozilla-b2g/gaia/pull/15563

- `compose.js`
    - Every time content (message, subject, attachment) is changed, set draft to edited
- `drafts.js`
    - Set draft to not edited when it's first created
- `message_manager.js`
    - After a threadless draft is loaded in #new panel, set draft to not edited
    - After a threaded draft is loaded in #thread-## panel, set draft to not edited
- `thread_ui.js`
    - Do not prompt for replacement if the draft has not been marked as edited
- Tests
    - Unit tests
        - `compose_test.js`
            - Check that draft is marked as edited when content changes
        - `drafts_test.js`
            - Check that draft is not marked as edited upon creation
        - `message_manager.js`
            - Check that draft is not marked as edited after thread-less draft loads or after thread-bound draft loads
        - `thread_ui_test.js`
            - Check that replacement dialog is not called if draft was not edited
    - Manual tests
        - Thread-bound draft thread
            - Load thread, make no changes, press back => no dialog
            - Load thread, make subject change, press back => dialog
            - Load thread, make attachment change, press back => dialog
            - Load thread, make message change, press back => dialog
        - Thread-less draft thread
            - Load thread, make no changes, press back => no dialog
            - Load thread, make subject change, press back => dialog
            - Load thread, make attachment change, press back => dialog
            - Load thread, make message change, press back => dialog
            - Load thread, make recipient change, press back => dialog
- Notes
    - Dialog comes back even if the content is 'edited' but not actually changed
    - Dialog comes back if **empty** subject is added (this triggers a `Compose.onContentChanged`)
    - Since thread-less drafts are orphaned and resaved every time, they will move up in the thread list even if their content didn't change (because they are resaved)
Attachment #8362592 - Attachment description: https://github.com/evhan55/gaia/compare/952484 → https://github.com/mozilla-b2g/gaia/pull/15563
Comment on attachment 8362592 [details]
https://github.com/mozilla-b2g/gaia/pull/15563

some minor comments but looks good
I haven't actually tried the code on the device so be sure to test :)
Attachment #8362592 - Flags: feedback?(felash) → feedback+
Comment on attachment 8362592 [details]
https://github.com/mozilla-b2g/gaia/pull/15563

Works perfectly, make jshint happy and then this is r=me—thanks!
Attachment #8362592 - Flags: review?(waldron.rick) → review+
Landed and closing: https://github.com/mozilla-b2g/gaia/commit/211239734791af96321cacbca193a1069da37b34
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
blocking-b2g: 1.4? → ---
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: