Qreator

Merge lp://staging/~stefan-schwarzburg/qreator/qreator_sms_mms_tel into lp://staging/qreator

  1. qreator_sms_mms_tel
  2. Merge into trunk
Proposed by Schwarzburg
Status: Merged
Approved by: David Planella
Approved revision: 150
Merged at revision: 151
Proposed branch: lp://staging/~stefan-schwarzburg/qreator/qreator_sms_mms_tel
Merge into: lp://staging/qreator
Diff against target: 0 lines
To merge this branch: bzr merge lp://staging/~stefan-schwarzburg/qreator/qreator_sms_mms_tel
Reviewer Review Type Date Requested Status
David Planella Approve
Review via email: mp+133805@code.staging.launchpad.net

This proposal supersedes a proposal from 2012-10-30.

Description of the change

This branch implements the three data formats: SMS, MMS and tel (call).

The data format is not standardized (see https://code.google.com/p/zxing/wiki/BarcodeContents), but it seems that the chosen format: SMSTO:<number>:<message> is the most used format.

I'm open to all suggestions (including a different icon or separating the "tel" type from the "SMS" and "MMS".

A minor bug fix is included in this branch: adding QR Code types increases the main window height, but this can be fixed a simple line that hides the individual windows before adding them to the main window. I can of cause separate this little fix in a different branch.

When you have reviewed the branches, you might want to ask me to get the branches up to date with trunk after each merge...

To post a comment you must log in.
Revision history for this message
David Planella (dpm) wrote : Posted in a previous version of this proposal

Thanks again for this feature!

I'm planning to do the thorough review that it deserves, but for now, after having tested if from a user point of view (i.e. not having looked at the code yet), here are a few things that I've noticed:

- The Call/SMS/MMS dropdown needs a label on the right, to keep consistency with the other code type layouts (e.g. VCard, Wifi, etc) and not leave an empty space. It could simply be "Type:" for example.
- The extra space for the exclamation sign next to the 'Message:' text box in the 'SMS:' or 'MMS:' types should be taken into account in the 'Call:' type, so that when switching from one type to another we don't have widget sizes moving around. In fact, it should either be put inside the textbox, or next to it on the right, as we do with the VCard code
- The exclamation mark icon should be the same one as we use for the VCard code
- I could not manage to make the exclamation mark go away in the 'SMS:' type, even after having entered a correct phone number and message
- [Optional] we should probably only allow numbers and the '+' sign in the Number text box
- [Optional, discussion] shall we have a multi-line textbox to enter the message in SMS?
- [Optional, discussion] I could not see any noticeable difference between the SMS and MMS types. Do you think we really need MMS?, do people still use them?

review: Needs Fixing
Revision history for this message
David Planella (dpm) wrote : Posted in a previous version of this proposal

Also, if it's not too much trouble, would you mind, as you're suggesting, separating the window height bug into a dedicated branch?

I don't mind minor unrelated fixes to be put in feature branches, but this particular one, as it's got to do with window sizing, which I remember to be a bit of a nightmare for Qreator in GTK, I'd like to keep track of.

Thanks!

Revision history for this message
Schwarzburg (stefan-schwarzburg) wrote : Posted in a previous version of this proposal

I'll add the "Type:" label.

The space is good as well.

Explamation mark icon will be changed.

Very good point with the MMS :-) I never used them. So I'll remove them.

the number box should allow all signs that locale phone numbers have. I have looked at a great library https://code.google.com/p/libphonenumber/ which is sadly not available for Ubuntu... But the goal would be: if someone from Germany enters a typical German number: "07473 / 123456", this should be allowed and converted into an international format (+497473123456). This could be done by the library I mentioned, but getting it into Ubuntu is a lot of work.
So maybe until this can be done, we should restrict to numbers and +

Multiline message box is fine, do you think a numbers counter would be imporant (e.g. "137 [3 letters left]")?

The exclamation mark: I think it necessary to inform the user that if a message is entered, many QR Code scanners will behave badly. On many, it will not work at all (even the phonenumber will be useless then).
SO my question to you would be: how is this best communicated to the user and if by a message dialog, should the sign disappear after the user has read the warning?

Revision history for this message
David Planella (dpm) wrote : Posted in a previous version of this proposal

Al 09/11/12 14:18, En/na Schwarzburg ha escrit:
> I'll add the "Type:" label.
>
> The space is good as well.
>
> Explamation mark icon will be changed.
>
> Very good point with the MMS :-) I never used them. So I'll remove them.
>

Ack, thanks.

> the number box should allow all signs that locale phone numbers have. I have looked at a great library https://code.google.com/p/libphonenumber/ which is sadly not available for Ubuntu... But the goal would be: if someone from Germany enters a typical German number: "07473 / 123456", this should be allowed and converted into an international format (+497473123456). This could be done by the library I mentioned, but getting it into Ubuntu is a lot of work.
> So maybe until this can be done, we should restrict to numbers and +
>

I agree, let's keep the scope down for now.

> Multiline message box is fine, do you think a numbers counter would be imporant (e.g. "137 [3 letters left]")?
>

Ah, yeah, good idea, this might be a nice feature.

> The exclamation mark: I think it necessary to inform the user that if a message is entered, many QR Code scanners will behave badly. On many, it will not work at all (even the phonenumber will be useless then).
> SO my question to you would be: how is this best communicated to the user and if by a message dialog, should the sign disappear after the user has read the warning?
>

Ah, gotcha, I understand it now. A quick question first: assuming we're
restricting the target QR scanners to the most popular ones (e.g.
QrDroid and whichever popular app they use on the iPhone) - if we find
out that they support the non-standard SMS code, do you still think that
we should show a message?

If you still think so, then my suggestion would be to show the message
in a manner that it's visible but does not get in the way. For example:

1. Inside the text box: [Message. WARNING not all code readers might
support it]
2. Additionally, an exclamation mark icon inside the textbox that goes
away as soon as you type. I know single-line text boxes support it, not
sure about the multi-line ones, though.

I'd be up for keeping it lightweight, as users tend to freak out when
they see warning signs and exclamation marks :)

Let me know what you think.

Revision history for this message
Schwarzburg (stefan-schwarzburg) wrote : Posted in a previous version of this proposal

As for the message:
- I have a N900 (with Debian based Maemo OS)
- I have access to an iPhone, where I installed 12 free QR scanners for testing purposes

If you have access to an android phone, we might be able to determine how often the format

SMSTO:<number>:<message>

fails.

- I have also tested 20 freely available online QR Code creaters, which allow the message type. All of them use this format. But I forgot to count the number of creators I found which only support sms:<number>. Probably about 5.

What do you think?

Revision history for this message
David Planella (dpm) wrote : Posted in a previous version of this proposal

Sounds good, I'll add the results of my experiments with Android too.

In the meantime, and as with the other MP, would you mind resubmitting it against the qreator-hackers branch? Thanks!

Revision history for this message
David Planella (dpm) :
review: Needs Fixing
Revision history for this message
Schwarzburg (stefan-schwarzburg) wrote :

HI David,

I have tried to update the code to include most comments.

I have not found an easy or nice way to show the letter counter nor for setting an icon in the textview.
Instead I have added a tooltip to show the warning.

What do you think?

Revision history for this message
Schwarzburg (stefan-schwarzburg) wrote :

Ok, from my point of view, this is ready for merging.

Since the textviews do not support placeholder text nor icons, the appearance is not as nice as I would like it to be, but it is close enough. If you happen to know a way to get the text color of an entry-placeholder text (dynamically, for every theme...), then I'm happy to update the code. Otherwise, I would just leave it like this.

Revision history for this message
David Planella (dpm) wrote :

In terms of functionality, it looks good to me. I've got a few recommendations in terms of layout and wording. Rather than just listing them, I thought I'd put them on a branch for you to review and if you're happy with them to accept them:

bzr push lp:~dpm/qreator/qreator_sms_mms_tel

## Rationale

- Layout: I've just added some horizontal and vertical spacing to the widgets to be consistent with the rest of the code's UI
- Wording: I've modified some texts with the intention to make them (hopefully!) more user-friendly. I've avoided using SMS other than in the combobox and I've suggested "Messaging", as as far as I know, at least Brits tend not to use the acronym (they use "text messaging"), and looking at my Android phone there is no reference to SMS at all (only to "Messaging")
- Other: I've removed the extra tooltip with the warning regarding some code readers not suporting the SMS format, as we're already clearly stating that in the placeholder text

Finally, this is not a requirement to pass this review, but on future merge proposals, and again for the sake of consistency with the rest of the code, if you could look into wrapping the code at 80 lines, that'd be great, thanks!

I'm setting this as Needs Fixing for now, as I think at the very least the layout changes should be applied.

Let me know what you think.

review: Needs Fixing
Revision history for this message
David Planella (dpm) wrote :

The bzr command should have been, of course 'bzr pull lp:~dpm/qreator/qreator_sms_mms_tel' (or 'bzr merge'), not 'push'

lp://staging/~stefan-schwarzburg/qreator/qreator_sms_mms_tel updated
147. By Schwarzburg

merged davids changes

148. By Schwarzburg

replaced the last occurence of SMS with 'Text Message'

149. By Schwarzburg

made qrcode/QRCodeSMS*.py pep8 complient and removed pylint errors and warnings

150. By Schwarzburg

merged with latest trunk

Revision history for this message
Schwarzburg (stefan-schwarzburg) wrote :

> In terms of functionality, it looks good to me. I've got a few recommendations
> in terms of layout and wording. Rather than just listing them, I thought I'd
> put them on a branch for you to review and if you're happy with them to accept
> them:
>
> bzr push lp:~dpm/qreator/qreator_sms_mms_tel
>
> ## Rationale
>
> - Layout: I've just added some horizontal and vertical spacing to the widgets
> to be consistent with the rest of the code's UI

Thanks. Maybe you could add a short HACKING file or README file, where we can add the typical layout properties?
Something like:
- Entry: margin xxx, align xxx, ...,

I have of cause accepted your changes!

> - Wording: I've modified some texts with the intention to make them
> (hopefully!) more user-friendly. I've avoided using SMS other than in the
> combobox and I've suggested "Messaging", as as far as I know, at least Brits
> tend not to use the acronym (they use "text messaging"), and looking at my
> Android phone there is no reference to SMS at all (only to "Messaging")

You are right to go for consistency. I just hope the German translation will be SMS and not "Nachricht" or "Text Nachricht"...

> - Other: I've removed the extra tooltip with the warning regarding some code
> readers not suporting the SMS format, as we're already clearly stating that in
> the placeholder text
>

Sure.

> Finally, this is not a requirement to pass this review, but on future merge
> proposals, and again for the sake of consistency with the rest of the code, if
> you could look into wrapping the code at 80 lines, that'd be great, thanks!
>

Actually, this should be a requirement :-)
I have now changed this, as well as all other pep8 violations and pylint warnings/errors.

> I'm setting this as Needs Fixing for now, as I think at the very least the
> layout changes should be applied.
>
> Let me know what you think.
I think qreator is really getting forward :-)

Revision history for this message
David Planella (dpm) wrote :

Al 22/11/12 09:53, En/na Schwarzburg ha escrit:
>> In terms of functionality, it looks good to me. I've got a few recommendations
>> in terms of layout and wording. Rather than just listing them, I thought I'd
>> put them on a branch for you to review and if you're happy with them to accept
>> them:
>>
>> bzr push lp:~dpm/qreator/qreator_sms_mms_tel
>>
>> ## Rationale
>>
>> - Layout: I've just added some horizontal and vertical spacing to the widgets
>> to be consistent with the rest of the code's UI
>
> Thanks. Maybe you could add a short HACKING file or README file, where we can add the typical layout properties?
> Something like:
> - Entry: margin xxx, align xxx, ...,
>

Indeed, that sounds like a very good idea.

> I have of cause accepted your changes!
>

Cool, thanks!

>> - Wording: I've modified some texts with the intention to make them
>> (hopefully!) more user-friendly. I've avoided using SMS other than in the
>> combobox and I've suggested "Messaging", as as far as I know, at least Brits
>> tend not to use the acronym (they use "text messaging"), and looking at my
>> Android phone there is no reference to SMS at all (only to "Messaging")
>
> You are right to go for consistency. I just hope the German translation will be SMS and not "Nachricht" or "Text Nachricht"...
>

This is easily solvable by adding a translator comment on the line above
where the translatable message appears. E.g.

# TRANSLATORS: this refers to an SMS message
sometext = _('Messaging')

>> - Other: I've removed the extra tooltip with the warning regarding some code
>> readers not suporting the SMS format, as we're already clearly stating that in
>> the placeholder text
>>
>
> Sure.
>
>> Finally, this is not a requirement to pass this review, but on future merge
>> proposals, and again for the sake of consistency with the rest of the code, if
>> you could look into wrapping the code at 80 lines, that'd be great, thanks!
>>
>
> Actually, this should be a requirement :-)
> I have now changed this, as well as all other pep8 violations and pylint warnings/errors.
>

Excellent.

>> I'm setting this as Needs Fixing for now, as I think at the very least the
>> layout changes should be applied.
>>
>> Let me know what you think.
> I think qreator is really getting forward :-)
>

Rock on.

 review approve

review: Approve
lp://staging/~stefan-schwarzburg/qreator/qreator_sms_mms_tel updated
151. By Schwarzburg

added short translator guides

Preview Diff

Empty

Subscribers

People subscribed via source and target branches

to all changes: