Closed Bug 944249 Opened 6 years ago Closed 6 years ago

[Messages] move the segment information request and size check to compose.js

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S1 (1aug)

People

(Reporter: julienw, Assigned: julienw)

References

Details

(Whiteboard: [p(2.0S6)=3][p(2.1S1)=2])

Attachments

(1 file)

46 bytes, text/x-github-pull-request
steveck
: review+
Details | Review
Right now, there is a complex code to set the type of the in-progress message and to prevent it when the max size was reached.

Instead, we should do this check in compose.js, and compose.js should expose the result of the segments information request in properties before sending the "type" event.

This is a simple refactoring that will reduce the existing complexity.
Whiteboard: [p=3]
In this bug, we also want to change how the compose type is decided.
Blocks: 1026384
Target Milestone: --- → 2.0 S6 (18july)
Blocks: 1011085
Assignee: nobody → felash
In-progress work: https://github.com/julienw/gaia/compare/944249-move-segment-size-compose-type

(completely broken right now, don't try it :) )
Blocks: 1040144
Depends on: 1009568
Depends on: 1040215
The branch from comment 2 seems to work fine for me. The unit tests are broken though, so I'll work on it tomorrow.

Also I try to make more meaningful commits, possibly in separate bugs (like bug 1009568, bug  1040215) to make it easier to review.
Depends on: 1039585
Whiteboard: [p=3] → [p(2.0S6)=3][p(2.1S1)=2]
Target Milestone: 2.0 S6 (18july) → 2.1 S1 (1aug)
Status: NEW → ASSIGNED
Attached file github PR
Comment on attachment 8463428 [details] [review]
github PR

Hey Steve,

I think it's ready to be reviewed !

I split the work in different commits in a kind of logical way, so that you can look at them separately. But feel free to comment globally if it's easier for you.

I couldn't see how to split them in separate bug because none of the changes make really sense in a standalone way.

The logic of the patch is:
* move a lot of Composer-relative operations from ThreadUI to Compose
* prepare for when we'll split Compose in a Controller and a View parts (that's why I still use the internal 'type' event)
* I didn't use the EventDispatcher for Compose.js but I filed bug 1045058 for this (with a WIP patch)
* I added an operation in MessageManager to get the segment info information.

Tell me what you think!
Attachment #8463428 - Flags: review?(schung)
Blocks: 1045058
It's really awesome to see the compose related logic into compose.js and clean up this ThreadUI! Just give a quick test and it still works fine without any problem. Will take a deep look tomorrow, thanks for another refactoring :)
Your comments have been fixed, new code is pushed to the PR, ready for you review :)
Flags: needinfo?(schung)
Except for the concern in timing for update segmentInfo and conflicts, the rest of part looks great, thanks!
Flags: needinfo?(schung)
Hey Steve, see https://github.com/mozilla-b2g/gaia/pull/22220#discussion_r15638058

There actually is a good reason I still check segment info when we're in MMS mode :) If we're in MMS mode because of a long text, we can't go back from MMS to SMS when removing characters if we don't check segmentinfo.
Flags: needinfo?(schung)
(In reply to Julien Wajsberg [:julienw] from comment #9)
> Hey Steve, see
> https://github.com/mozilla-b2g/gaia/pull/22220#discussion_r15638058
> 
> There actually is a good reason I still check segment info when we're in MMS
> mode :) If we're in MMS mode because of a long text, we can't go back from
> MMS to SMS when removing characters if we don't check segmentinfo.

Do you think it's possible to improve in the future? It seems some call path limitation that cause we unable to trigger updateSegmentInfoThrottled after type changed. I'm fine without this issue address in this patch, just wondering if it's possible to refine in the future.
Flags: needinfo?(schung)
(In reply to Steve Chung [:steveck] from comment #10)
> (In reply to Julien Wajsberg [:julienw] from comment #9)
> > Hey Steve, see
> > https://github.com/mozilla-b2g/gaia/pull/22220#discussion_r15638058
> > 
> > There actually is a good reason I still check segment info when we're in MMS
> > mode :) If we're in MMS mode because of a long text, we can't go back from
> > MMS to SMS when removing characters if we don't check segmentinfo.
> 
> Do you think it's possible to improve in the future? It seems some call path
> limitation that cause we unable to trigger updateSegmentInfoThrottled after
> type changed. I'm fine without this issue address in this patch, just
> wondering if it's possible to refine in the future.

Well, either we call it, or we don't call it :)

The issue is that we need to call it when the user changes the content, otherwise we don't know that the text goes under the limit. I think this can happen in 2 cases:

* the user deletes enough characters.
* the user deletes the character(s) that triggers the Unicode encoding.

So maybe we can call it only when the user removes some text. It should be doable by storing the length in the "input" event handler, and check it's lower.

This is probably quite edgy use case though: I think most people sending SMS send also attachments :)
Comment on attachment 8463428 [details] [review]
github PR

r=me with comment for updateSegmentInfo and conflicts fixed, thanks for the refactoring!
Attachment #8463428 - Flags: review?(schung) → review+
Thanks !
Added the comment, conflicts fixed.

Landed in master:
5a653a7bdedac40eaf1ed82d2e410c3090a5a0b9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.