messaging-app

Merge lp://staging/~pat-mcgowan/messaging-app/fix-1278790 into lp://staging/messaging-app

  1. fix-1278790
  2. Merge into trunk
Proposed by Pat McGowan
Status: Merged
Approved by: Tiago Salem Herrmann
Approved revision: 587
Merged at revision: 586
Proposed branch: lp://staging/~pat-mcgowan/messaging-app/fix-1278790
Merge into: lp://staging/messaging-app
Diff against target: 192 lines (+81/-30)
4 files modified
src/qml/ComposeBar.qml (+67/-28)
src/qml/Messages.qml (+1/-0)
src/qml/SettingsPage.qml (+7/-2)
src/qml/messaging-app.qml (+6/-0)
To merge this branch: bzr merge lp://staging/~pat-mcgowan/messaging-app/fix-1278790
Reviewer Review Type Date Requested Status
Tiago Salem Herrmann (community) Approve
Review via email: mp+301952@code.staging.launchpad.net

Commit message

This adds support for showing the current character count and number of messages to be sent via SMS. It includes a new label on top of the text area showing number of characters / 160 (number of messages). It is shown once the message is over 1 line in length, per other systems.
A new settings option is added to enable the feature which is off by default.
Emojis are counted as two characters. I did not test with multibyte character sets but expect that QML TextArea does the correct thing.
See lp:1278790

Description of the change

This adds support for showing the current character count and number of messages to be sent via SMS. It includes a new label on top of the text area showing number of characters / 160 (number of messages). It is shown once the message is over 1 line in length, per other systems.
A new settings option is added to enable the feature which is off by default.
Emojis are counted as two characters. I did not test with multibyte character sets but expect that QML TextArea does the correct thing.
See lp:1278790

To post a comment you must log in.
Revision history for this message
Bill Filler (bfiller) wrote :

few comments
1) drop the (1) suffix if the count is < 160
2) hide the count label if it's an MMS message (either MMS group chat or single recipient with an attachment) as the rules are different I suppose

Revision history for this message
Tiago Salem Herrmann (tiagosh) wrote :

Thanks for the patch.
I left some comments.

You can check if the conversation is mms with the following code in ComposeBar.qml:

// more than one participant and mms group chat enabled or only one participant and any attachment
property var isMms = ((messages.participants.length > 1 && telepathyHelper.mmsGroupChat) || (messages.participants.length == 1 && attachments.count > 0))

review: Needs Fixing
lp://staging/~pat-mcgowan/messaging-app/fix-1278790 updated
583. By Pat McGowan

Indicate when using MMS, show partial composition with italics, only show message count when more than 1

584. By Pat McGowan

cleanup

585. By Pat McGowan

MMS needs to be translated

Revision history for this message
Pat McGowan (pat-mcgowan) wrote :

Ready for another pass

Revision history for this message
Pat McGowan (pat-mcgowan) wrote :

Waiting for final design inputs

lp://staging/~pat-mcgowan/messaging-app/fix-1278790 updated
586. By Pat McGowan

Match UX design specs, don't show the 160 limit, move to lower right, always show the message count

587. By Pat McGowan

add a space per mockup

Revision history for this message
Tiago Salem Herrmann (tiagosh) wrote :

Just tested and it works fine.
Code looks good too.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches