TIMOB-9390: Blackberry: implement rest of String.formatXXX methods (other than format) #76

alexandergalstyan · 2012-06-11T12:35:11Z

Reviewers: HarutM, JP

Review the 99130f8

ChangeLog:

Added new TiDateObject class.
Added new message constants.
Implemented formatDate, formatTime, formatCurrency, formatDecimal methods

Test Cases:

Using sample tibbtest project create var with the value of Date
Use formatDate(date, [format]), formatTime(date, [format]) in app.js
Make sure string correctly formatted.
Try the same with previously changed language.
Create var with number value.
Use formatCurrency(number) in app.js
Make sure string correctly formatted.
Try the same with previously changed language.
Create var with number value.
Use formatDecimal(number, [locale], [pattern]) in app.js
Make sure string correctly formatted.
Try the same with previously changed language.

NOTE: locale & pattern not yet implemented for formatDecimal

Test snippet from app.js:
//
// create root window
//
var win1 = Titanium.UI.createWindow({
backgroundColor:'#F00'
});

var date = new Date();
var format = String.formatDate(date, 'short');

//var date = new Date();
//var format = String.formatDate(date, 'medium');

//var date = new Date();
//var format = String.formatDate(date, 'long');

//var date = new Date();
//var format = String.formatTime(date, 'short');

//var date = new Date();
//var format = String.formatTime(date, 'medium');

//var date = new Date();
//var format = String.formatTime(date, 'long');

var label1 = Ti.UI.createLabel({
textAlign : 'center',
text:'Hello, world!',
color:'green',
top: 200
});

label1.text = format;

win1.add(label1);

// open window
win1.open();

…88' into formatReview #Some more info....

…rry' into formatReview #Some more info....

Reviewers: DavidC, JP ChangeLog: - Implement decode/encode URIComponent Test Cases: - Using sample tibbtest project create var with the value of: 'http://www.example.com/List%20of%20holidays.xml' - Use decodeURIComponent to decode the uri. - Make sure it decodes successfully. - Using sample tibbtest project create var with the value of: 'http://www.example.com/List of holidays.xml' - Use encodeURIComponent to encode the uri. - Make sure it encodes successfully.

…ther than format) Reviewers: HarutM, JP ChangeLog: - Added new TiDateObject class. - Added new message constants. - Implemented formatDate, formatTime, formatCurrency, formatDecimal methods Test Cases: - Using sample tibbtest project create var with the value of Date - Use formatDate(date, [format]), formatTime(date, [format]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatCurrency(number) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatDecimal(number, [locale], [pattern]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. NOTE: locale & pattern not yet implemented for formatDecimal

jpl-mac · 2012-06-12T18:43:48Z

Can you be more explicit on the test cases? If i were to try and run these tests on my own, i wouldn't know what to do or look for.

what's a "var with value of Date"?
what's the [format] in "Use formatDate(date, [format]), formatTime(date, [format])"
etc.

The value behind the Tests section in the patch description is so a QA or someone else with no knowledge of the patch can look at the tests and run them.
Then the tests could be as simple as build this app.js snippet for BB and compare with the same built for Android or Iphone..
Maybe it's time we start providing the app.js file we used for the development/testing of a patch. the app.js snippet could be added here as a comment.

jpl-mac · 2012-06-12T19:30:14Z

blackberry/tibb/TiConstants.h

+    DATE_FORMAT_LONG,
+    DATE_FORMAT_MEDIUM,
+    DATE_FORMAT_SHORT
+};


This doesn't seem to be used anywhere

jpl-mac · 2012-06-12T19:44:41Z

apidoc needs to be updated

jpl-mac · 2012-06-12T20:27:19Z

blackberry/tibb/TiStringObject.cpp

+
+    Local<Object> obj = Object::Cast(*value);
+    // Get year property
+    Local<Value> getYear_prop = (Object::Cast(*value)->Get(String::New("getFullYear")));


Can't you just use obj instead of casting *value everytime?

jpl-mac · 2012-06-12T21:38:46Z

Reviewed

Harutyun · 2012-06-13T05:48:22Z

blackberry/tibb/TiMessageStrings.cpp

Do we need this cpp file?

yes, the constants need to be defined in a cpp.

it's macro dark magic :)

…ther than format) ver2 ChangeLog: - Address comments. - Update String.yml apidoc. - Change the QDateTime to QTime for formatTime method. Test Cases: - Using sample tibbtest project create var with the value of Date - Use formatDate(date, [format]), formatTime(date, [format]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatCurrency(number) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatDecimal(number, [locale], [pattern]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. NOTE: locale & pattern not yet implemented for formatDecimal NOTE: test snippet available at discussion page.

alexandergalstyan · 2012-06-13T10:13:00Z

Updated. Added test cases snippets

Harutyun · 2012-06-13T11:59:16Z

Reviewed

Conflicts: blackberry/tibb/TiRootObject.cpp

…rry' into formatBranch #Some more info....

…ther than format) ver3 Reviewers: HarutM, JP Review the 99130f8 ChangeLog: - Remove TiDateObject class. - Address comments. Test Cases: - Using sample tibbtest project create var with the value of Date - Use formatDate(date, [format]), formatTime(date, [format]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatCurrency(number) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatDecimal(number, [locale], [pattern]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language.

alexandergalstyan · 2012-06-18T09:48:42Z

Updated behalf on #82

jpl-mac · 2012-06-18T15:29:20Z

Now that you also have the patch for built-in JS keywords, it would be good to validate the use of StringObjects and then remove the TODOs

…rry' into formatBranch #Some more info....

…89' into formatBranch Conflicts: blackberry/tibb/TiRootObject.cpp blackberry/tibb/TiStringObject.cpp

jpl-mac · 2012-06-21T14:58:05Z

blackberry/tibb/TiStringObject.cpp

+    // Try to parse optional format argument
+    if (args.Length() > 1 && (args[1]->IsString() || args[1]->IsStringObject()))
+    {
+        const String::Utf8Value utf8(args[1]->ToString());


Actually some care is needed around using ToString().

So we want to convert non string litteral (IsString() == true) args using ToString() when a string is expected, but we don't want to call ToString on strings.

This is the behaviour that we see from the chrome js console. So far i've had ToString() return a null pointer on string literals so that stopped me from using ToString on all args. If it works for you though you could do that.

This comment also applies to the other use further down

jpl-mac · 2012-06-22T17:04:38Z

blackberry/tibb/NativeStringInterface.cpp

+    if (args.Length() > 1)
+    {
+        Local<Value> format = args[1];
+        if (!(format->IsString() || format->IsStringObject()))


This condition only needs to check for IsString. The ToString() methods should be applied to StringObjects also. This is how JS behaves in Chrome.

jpl-mac · 2012-06-22T17:21:16Z

Rereviewed

…rry' into formatBranch Conflicts: blackberry/tibbtest/app.js

…ther than format) ver3 Reviewers: HarutM, JP Review the 99130f8 ChangeLog: - Remove TiDateObject class. - Address comments. Test Cases: - Using sample tibbtest project create var with the value of Date - Use formatDate(date, [format]), formatTime(date, [format]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatCurrency(number) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatDecimal(number, [locale], [pattern]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language.

alexandergalstyan · 2012-06-25T09:50:44Z

Updated.

jpl-mac · 2012-06-26T14:54:44Z

blackberry/tibb/NativeStringInterface.cpp

+static QString formatString(QString s, Local<Value> arg);
+static QString formatPointer(QString s, Local<Value> arg);
+
+// Helper function


prototypes above is for helper functions, this one can be bunched with the others at merge time

jpl-mac · 2012-06-26T15:05:36Z

Approved with trivial comment, can be done at merge time

I don't think Harut has approved yet. Also this PR needs to wait for #84 to be merged first.

Harutyun · 2012-06-28T06:07:52Z

Approved

…rry' into formatBranch Conflicts: blackberry/tibb/NativeStringInterface.cpp blackberry/tibb/TiMessageStrings.h blackberry/tibb/TiRootObject.cpp

…ther than format) ver4 Reviewers: HarutM, JP ChangeLog: - Address comments. Test Cases: - Using sample tibbtest project create var with the value of Date - Use formatDate(date, [format]), formatTime(date, [format]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatCurrency(number) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatDecimal(number, [locale], [pattern]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. NOTE: locale & pattern not yet implemented for formatDecimal Test snippet from app.js: // // create root window // var win1 = Titanium.UI.createWindow({ backgroundColor:'#F00' }); var date = new Date(); var format = String.formatDate(date, 'short'); //var date = new Date(); //var format = String.formatDate(date, 'medium'); //var date = new Date(); //var format = String.formatDate(date, 'long'); //var date = new Date(); //var format = String.formatTime(date, 'short'); //var date = new Date(); //var format = String.formatTime(date, 'medium'); //var date = new Date(); //var format = String.formatTime(date, 'long'); var label1 = Ti.UI.createLabel({ textAlign : 'center', text:'Hello, world!', color:'green', top: 200 }); label1.text = format; win1.add(label1); // open window win1.open();

jpl-mac · 2012-07-06T15:37:10Z

blackberry/tibb/NativeMessageStrings.h

@@ -22,6 +22,7 @@
 {

 N_MESSAGESTRINGS_CONST_DEF(char*, An_error_occurred_converting_to_int, "An error occurred converting to int");
+N_MESSAGESTRINGS_CONST_DEF(char*, Expected_argument_of_type_date, "Expected argument of type date");


Trivial: Date type in js is with a capital D so the message should reflect that, can be done at merge time

…rry' into formatBranch #Some more info....

…ther than format) Reviewers: HarutM, JP ChangeLog: - Move the date conversion logics into the NativeControlObject. - Address comments. Test Cases: - Using sample tibbtest project create var with the value of Date - Use formatDate(date, [format]), formatTime(date, [format]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatCurrency(number) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatDecimal(number, [locale], [pattern]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. NOTE: locale & pattern not yet implemented for formatDecimal Test snippet from app.js: // // create root window // var win1 = Titanium.UI.createWindow({ backgroundColor:'#F00' }); var date = new Date(); var format = String.formatDate(date, 'short'); //var date = new Date(); //var format = String.formatDate(date, 'medium'); //var date = new Date(); //var format = String.formatDate(date, 'long'); //var date = new Date(); //var format = String.formatTime(date, 'short'); //var date = new Date(); //var format = String.formatTime(date, 'medium'); //var date = new Date(); //var format = String.formatTime(date, 'long'); var label1 = Ti.UI.createLabel({ textAlign : 'center', text:'Hello, world!', color:'green', top: 200 }); label1.text = format; win1.add(label1); // open window win1.open();

alexandergalstyan · 2012-07-09T13:01:58Z

Updated.

jpl-mac · 2012-07-09T14:23:33Z

blackberry/tibb/NativeDateTimePickerObject.cpp

@@ -7,7 +7,7 @@

 #include "NativeDateTimePickerObject.h"

-#include "ConversionHelper.h"
+#include "NativeControlObject.h"


Shouldn't be necessary as it's already a parent class of NDTP

…rry' into formatBranch Conflicts: blackberry/tibb/NativeControlObject.cpp

…ther than format) ver6 Reviewers: HarutM, JP ChangeLog: - Address comments. - Remove ConversionHelper class. Test Cases: - Using sample tibbtest project create var with the value of Date - Use formatDate(date, [format]), formatTime(date, [format]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatCurrency(number) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. - Create var with number value. - Use formatDecimal(number, [locale], [pattern]) in app.js - Make sure string correctly formatted. - Try the same with previously changed language. NOTE: locale & pattern not yet implemented for formatDecimal Test snippet from app.js: // // create root window // var win1 = Titanium.UI.createWindow({ backgroundColor:'#F00' }); var date = new Date(); var format = String.formatDate(date, 'short'); //var date = new Date(); //var format = String.formatDate(date, 'medium'); //var date = new Date(); //var format = String.formatDate(date, 'long'); //var date = new Date(); //var format = String.formatTime(date, 'short'); //var date = new Date(); //var format = String.formatTime(date, 'medium'); //var date = new Date(); //var format = String.formatTime(date, 'long'); var label1 = Ti.UI.createLabel({ textAlign : 'center', text:'Hello, world!', color:'green', top: 200 }); label1.text = format; win1.add(label1); // open window win1.open();

alexandergalstyan · 2012-07-11T08:15:37Z

Updated

jpl-mac · 2012-07-11T13:07:11Z

Approved with comment

Harutyun · 2012-07-11T13:29:44Z

Approved

TIMOB-9390: Blackberry: implement rest of String.formatXXX methods (other than format)

unknown added 4 commits June 9, 2012 15:28

Merge branch 'formatReview', remote-tracking branch 'jpl-mac/timob-93…

959eecc

…88' into formatReview #Some more info....

Merge branch 'formatReview', remote-tracking branch 'upstream/blackbe…

4c3ab30

…rry' into formatReview #Some more info....

jpl-mac reviewed Jun 12, 2012
View reviewed changes

Harutyun reviewed Jun 13, 2012
View reviewed changes

unknown added 3 commits June 18, 2012 13:06

Merge branch 'formatBranch'; commit '3c93b467' into formatBranch

8c9c499

Conflicts: blackberry/tibb/TiRootObject.cpp

Merge branch 'formatBranch', remote-tracking branch 'upstream/blackbe…

ba505e6

…rry' into formatBranch #Some more info....

unknown added 2 commits June 21, 2012 18:14

Merge branch 'formatBranch', remote-tracking branch 'upstream/blackbe…

4510b27

…rry' into formatBranch #Some more info....

Merge branch 'formatBranch', remote-tracking branch 'jpl-mac/timob-93…

014c5fd

…89' into formatBranch Conflicts: blackberry/tibb/TiRootObject.cpp blackberry/tibb/TiStringObject.cpp

jpl-mac reviewed Jun 21, 2012
View reviewed changes

jpl-mac reviewed Jun 22, 2012
View reviewed changes

unknown added 2 commits June 25, 2012 13:44

Merge branch 'formatBranch', remote-tracking branch 'upstream/blackbe…

05bcf56

…rry' into formatBranch Conflicts: blackberry/tibbtest/app.js

jpl-mac reviewed Jun 26, 2012
View reviewed changes

alexandergalstyan added 2 commits July 6, 2012 18:59

Merge branch 'formatBranch', remote-tracking branch 'upstream/blackbe…

ba500af

…rry' into formatBranch Conflicts: blackberry/tibb/NativeStringInterface.cpp blackberry/tibb/TiMessageStrings.h blackberry/tibb/TiRootObject.cpp

jpl-mac reviewed Jul 6, 2012
View reviewed changes

alexandergalstyan added 2 commits July 9, 2012 13:19

Merge branch 'formatBranch', remote-tracking branch 'upstream/blackbe…

e9d0fa3

…rry' into formatBranch #Some more info....

jpl-mac reviewed Jul 9, 2012
View reviewed changes

alexandergalstyan added 2 commits July 11, 2012 12:02

Merge branch 'formatBranch', remote-tracking branch 'upstream/blackbe…

16eb9a4

…rry' into formatBranch Conflicts: blackberry/tibb/NativeControlObject.cpp

alexandergalstyan closed this Jul 11, 2012

alexandergalstyan reopened this Jul 11, 2012

alexandergalstyan added a commit that referenced this pull request Jul 11, 2012

Merge pull request #76 from alexandergalstyan/formatBranch

fddf0b1

TIMOB-9390: Blackberry: implement rest of String.formatXXX methods (other than format)

alexandergalstyan merged commit fddf0b1 into Macadamian:blackberry Jul 11, 2012

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TIMOB-9390: Blackberry: implement rest of String.formatXXX methods (other than format) #76

TIMOB-9390: Blackberry: implement rest of String.formatXXX methods (other than format) #76

alexandergalstyan commented Jun 11, 2012

jpl-mac commented Jun 12, 2012

jpl-mac Jun 12, 2012

alexandergalstyan Jun 13, 2012

jpl-mac commented Jun 12, 2012

jpl-mac Jun 12, 2012

alexandergalstyan Jun 13, 2012

jpl-mac commented Jun 12, 2012

Harutyun Jun 13, 2012

jpl-mac Jun 13, 2012

alexandergalstyan commented Jun 13, 2012

Harutyun commented Jun 13, 2012

alexandergalstyan commented Jun 18, 2012

jpl-mac commented Jun 18, 2012

jpl-mac Jun 21, 2012

jpl-mac Jun 22, 2012

alexandergalstyan Jun 25, 2012

jpl-mac commented Jun 22, 2012

alexandergalstyan commented Jun 25, 2012

jpl-mac Jun 26, 2012

jpl-mac commented Jun 26, 2012

Harutyun commented Jun 28, 2012

jpl-mac Jul 6, 2012

alexandergalstyan commented Jul 9, 2012

jpl-mac Jul 9, 2012

alexandergalstyan Jul 11, 2012

alexandergalstyan commented Jul 11, 2012

jpl-mac commented Jul 11, 2012

Harutyun commented Jul 11, 2012

TIMOB-9390: Blackberry: implement rest of String.formatXXX methods (other than format) #76

TIMOB-9390: Blackberry: implement rest of String.formatXXX methods (other than format) #76

Conversation

alexandergalstyan commented Jun 11, 2012

jpl-mac commented Jun 12, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpl-mac commented Jun 12, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpl-mac commented Jun 12, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandergalstyan commented Jun 13, 2012

Harutyun commented Jun 13, 2012

alexandergalstyan commented Jun 18, 2012

jpl-mac commented Jun 18, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpl-mac commented Jun 22, 2012

alexandergalstyan commented Jun 25, 2012

Choose a reason for hiding this comment

jpl-mac commented Jun 26, 2012

Harutyun commented Jun 28, 2012

Choose a reason for hiding this comment

alexandergalstyan commented Jul 9, 2012

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexandergalstyan commented Jul 11, 2012

jpl-mac commented Jul 11, 2012

Harutyun commented Jul 11, 2012