messaging-app

Merge lp:~sil2100/messaging-app/flush_preedit into lp:messaging-app

flush_preedit
Merge into trunk

Proposed by Łukasz Zemczak on 2014-02-05

Status:

Merged

Approved by:

Bill Filler on 2014-03-12

Approved revision:

Merged at revision:

Proposed branch:

lp:~sil2100/messaging-app/flush_preedit

Merge into:

lp:messaging-app

Diff against target:

19 lines (+7/-1)

1 file modified

src/qml/Messages.qml (+7/-1)

To merge this branch:

bzr merge lp:~sil2100/messaging-app/flush_preedit

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Bill Filler (community)		2014-02-05	Approve on 2014-03-12
PS Jenkins bot	continuous-integration		Approve on 2014-03-06
Review via email: mp+204898@code.launchpad.net

Commit message

Make sure we flush the text that's being held in the OSK pre-edit (if any) before sending the message. Fixes LP: #1271494.

Description of the change

Make sure we flush text that's being held in the OSK pre-edit before sending the message. Fixes LP: #1271494.

* Are there any related MPs required for this MP to build/function as expected? Please list.
-> No

* Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)
-> Yes

* Did you perform an exploratory manual test run of your code change and any related functionality on device or emulator?
-> Yes, both me and Bill

* Did you successfully run all tests found in your component's Test Plan on device or emulator?
-> Yes, on a mako device

* If you changed the UI, was the change specified/approved by design?
-> N/A

* If you changed the packaging (debian), did you subscribe a core-dev to this MP?
-> N/A

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-05:

FAILED: Continuous integration, rev:64
http://jenkins.qa.ubuntu.com/job/messaging-app-ci/137/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty/2912
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-trusty-touch/2654/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/messaging-app-trusty-amd64-ci/52
    SUCCESS: http://jenkins.qa.ubuntu.com/job/messaging-app-trusty-armhf-ci/53
        deb: http://jenkins.qa.ubuntu.com/job/messaging-app-trusty-armhf-ci/53/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/messaging-app-trusty-i386-ci/52
    SUCCESS: http://jenkins.qa.ubuntu.com/job/autopilot-testrunner-otto-trusty/2558
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2914
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-amd64/2914/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2655
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-trusty-armhf/2655/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-runner-mako/5080/console
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/3640

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/messaging-app-ci/137/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/messaging-app-ci/140/rebuild

review: Approve (continuous-integration)

Revision history for this message

Łukasz Zemczak (sil2100) wrote on 2014-02-06:

Bill, you mentioned a specific case when the fix does not work. What was it?

Revision history for this message

Bill Filler (bfiller) wrote on 2014-02-06:

I think there are a couple of issues. Here is how I was testing:

1) I just turn on word prediction (but not auto completion)
2) go to messaging and select an existing conversation
3) type in the word "hello"
4) the send button is not enabled (this is by design I think)
5) pressing send causes the osk to get hidden but message never sent (because send button was disabled)

So, I think we need to have the send button enabled by default, then do the commit on the send. But then you'll need to check that the message is not blank otherwise you shouldn't send it.

The other problem, I think is an OSK bug. Calling Qt.inputMethod.commit() should cause the suggestions on the word ribbon to be cleared, but it doesn't. To produce, type "hello alr" this will cause suggestions to appear, then press send. The message is sent as "hello alr" which is correct but the word ribbon is still populated with the old suggestions when you click again in the input field

review: Needs Fixing

Revision history for this message

Łukasz Zemczak (sil2100) wrote on 2014-02-07:

Thanks for pointing these out.
Ok, so I think the easiest way of fixing it right now is not disabling the Send button when there is no text and just indeed not send a message if there is no message present. I wonder if we need design decision regarding this? In this case I would enable it by default and do all the checks for like the sender being done on button press as well for consistency. I don't like when things work with mixed results, like button enabling/disabling for the case of number and then not doing that for the message body.

On the other hand, why do we have this check in the first place? I remember some cases where there was required to send empty messages - like for certain lotteries and contests per-SMS. Not many people per-take in those, but it's still a possible use case.

I would personally either remove the lock and enable sending empty messages out-of-the-box, or maybe think about a question dialog pop-up with a question "Do you want to send an empty sms?". I'll try the second approach, but I think design needs to comment here.

As for the OSK bug - yes, indeed it seems to be a problem. I will consider it in a separate merge for ubuntu-keyboard.

lp:~sil2100/messaging-app/flush_preedit updated on 2014-02-07

65. By Łukasz Zemczak on 2014-02-07: Allow sending empty messages but only after being prompted as a double-check

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-02-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/messaging-app-ci/145/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Łukasz Zemczak (sil2100) wrote on 2014-02-10:

Will work on those AP tests, and a new one as well.

Revision history for this message

Tiago Salem Herrmann (tiagosh) wrote on 2014-02-14:

Hey guys,

can you check if this patch fixes the issue? http://paste.ubuntu.com/6930758/

Thanks

Revision history for this message

Łukasz Zemczak (sil2100) wrote on 2014-02-14:

Hi Tiago!

So, even though it seems to work, I still find a few small moments where it's a bit inconsitent. If you do the following:
- With preedit enabled, start typing a message like "H"
-> Notice the Send button gets enabled (as expected)
- Next, use backspace to erase that letter
-> Notice the Send button is still enabled

This is a minor nitpick, as the button won't send anything even if pressed - but that's something that I think is not good by design. Also, I would personally prefer to have the ability to send empty messages when needed.

What do you guys think?

Revision history for this message

Tiago Salem Herrmann (tiagosh) wrote on 2014-02-14:

Hey, Thanks for testing.

I believe this is a bug with the osk. Even when you remove the whole text the word prediction is still there, which doesn't make any sense, so probably the osk is reporting the wrong state to the app.
If you tap on any suggestion and then remove the text you will notice that the button is disabled as expected.

Revision history for this message

Łukasz Zemczak (sil2100) wrote on 2014-02-14:

Tests for my new functionality are almost done, it's a bit too late today though.

Revision history for this message

Łukasz Zemczak (sil2100) wrote on 2014-02-17:

Ok, let me look into that then. If it's a problem with the OSK then indeed it might be good to fix it and it might be easier to get your version of the fix landed faster - as my approach probably requires design decision ;) Let me dig in!

lp:~sil2100/messaging-app/flush_preedit updated on 2014-03-06

66. By Łukasz Zemczak on 2014-03-06: Revert back to the first approach + Tiago's proposition
67. By Łukasz Zemczak on 2014-03-06: Merge trunk

Revision history for this message

Łukasz Zemczak (sil2100) wrote on 2014-03-06:

Updated to use Tiago's method, since we have this as well:
https://code.launchpad.net/~sil2100/ubuntu-keyboard/preedit_state_fix/+merge/207878

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/messaging-app-ci/171/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-06:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/messaging-app-ci/172/rebuild

review: Approve (continuous-integration)

Revision history for this message

Tiago Salem Herrmann (tiagosh) wrote on 2014-03-10:

It looks good to me. Let's get the other MR merged and then we can approve this one.
Thank you!

Revision history for this message

Bill Filler (bfiller) wrote on 2014-03-12:

tested, and works well with https://code.launchpad.net/~bfiller/ubuntu-keyboard/fix-preedit/+merge/210527

review: Approve

lp:~sil2100/messaging-app/flush_preedit updated on 2014-03-27

68. By Łukasz Zemczak on 2014-03-27: Merge trunk

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Sam Short

Ubuntu Phablet Team

Łukasz Zemczak

 === modified file 'src/qml/Messages.qml'
 --- src/qml/Messages.qml	2014-03-20 18:39:45 +0000
 +++ src/qml/Messages.qml	2014-03-27 13:54:30 +0000
@@ -503,8 +503,14 @@
              anchors.bottom: parent.bottom
              text: "Send"
              width: units.gu(17)
--            enabled: textEntry.text != "" && telepathyHelper.connected && (participants.length > 0 || multiRecipient.recipientCount > 0 )
++            enabled: (textEntry.text != "" || textEntry.inputMethodComposing) && telepathyHelper.connected && (participants.length > 0 || multiRecipient.recipientCount > 0 )
              onClicked: {
++                // make sure we flush everything we have prepared in the OSK preedit
++                Qt.inputMethod.commit();
++                if (textEntry.text == "") {
++                    return
++                }
++
                  if (participants.length == 0 && multiRecipient.recipientCount > 0) {
                      participants = multiRecipient.recipients
+                 }