Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TIMOB-8821: BlackBerry: Implement UI.Label #61

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions apidoc/Titanium/UI/Label.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ properties:
summary: Key identifying a string from the locale file to use for the label text.
description: Only one of `text` or `textid` should be specified.
type: String
platforms: [android,iphone,ipad,mobileweb]
- name: wordWrap
summary: Enable or disable word wrapping in the label.
type: Boolean
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to update the Font apidoc as well

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot found documentation for it

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apidoc/Titanium/UI/Font.yml

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't meant that. For example for Android it says: "On Android, use the font file name, minus the .otf or .ttf extension.
For example, if you are using the Chantelli Antiqua font and the file is
named Chantelli_Antiqua.ttf, specify fontFamily: 'Chantelli_Antiqua'
on Android."
Cannot find appropriate documentation for BB.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you're looking at is for custom fonts. Can you install custom fonts on the BB? If you can't find the answer through investigation or documentation, then ask suavek.

The Font doc still needs to be updated to reflect the work of this PR.

  • fontFamily: Do you know which fonts are supported currently?
  • fontSize: what unit does the BB use? should the unit be appended to the number?
  • fontStyle: does italic work? currently it says only supported for iphone ipad
  • fontWeight: do bold and semibold work?

If you've tested your work you should already know most of these answers

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there are still things that need to be updated but need to wait for R6 to find the answers then add them to the JIra Issue for reworking label/font after R6

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you confirm that the jira issue has been updated with the information above?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still awaiting confirmation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expand Down
149 changes: 149 additions & 0 deletions blackberry/tibb/NativeAbstractTextControlObject.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,149 @@
/**
* Appcelerator Titanium Mobile
* Copyright (c) 2009-2012 by Appcelerator, Inc. All Rights Reserved.
* Licensed under the terms of the Apache Public License
* Please see the LICENSE included with this distribution for details.
*/

#include "NativeAbstractTextControlObject.h"
#include <bb/cascades/controls/abstracttextcontrol.h>
#include <bb/cascades/controls/textalignment.h>
#include <qt4/QtCore/qmap.h>
#include <qt4/QtCore/qstring.h>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only need to include the class name for Qt classes, like so:

#include <QMap>
#include <QString>

I don't think there's an equivalent for cascades classes though

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


NativeAbstractTextControlObject::NativeAbstractTextControlObject()
{
textControl_ = NULL;
}

NativeAbstractTextControlObject::~NativeAbstractTextControlObject()
{
}

bb::cascades::AbstractTextControl* NativeAbstractTextControlObject::getTextControl() const
{
return textControl_;
}

void NativeAbstractTextControlObject::setTextControl(bb::cascades::AbstractTextControl* textControl)
{
textControl_ = textControl;
setControl((bb::cascades::Control*)textControl_);
}

int NativeAbstractTextControlObject::setText(TiObject* obj)
{
QString str;
int error = NativeControlObject::getString(obj, str);
if (error != NATIVE_ERROR_OK)
{
return error;
}
textControl_->setText(str);
return NATIVE_ERROR_OK;
}

int NativeAbstractTextControlObject::setColor(TiObject* obj)
{
float r;
float g;
float b;
float a;

int error = NativeControlObject::getColorComponents(obj, &r, &g, &b, &a);
if (error != NATIVE_ERROR_OK)
{
return error;
}
bb::cascades::Color cscolor = bb::cascades::Color::fromRGBA(r, g, b, a);
textControl_->textStyle()->setColor(cscolor);
return NATIVE_ERROR_OK;
}

int NativeAbstractTextControlObject::setTextAlign(TiObject* obj)
{
int value;
int error = NativeControlObject::getInteger(obj, &value);
if (error != NATIVE_ERROR_OK)
{
return error;
}

switch (value)
{
case 0: // TEXT_ALIGNMENT_LEFT
textControl_->textStyle()->setAlignment(bb::cascades::TextAlignment::ForceLeft);
break;
case 1: // TEXT_ALIGNMENT_CENTER
textControl_->textStyle()->setAlignment(bb::cascades::TextAlignment::Center);
break;
case 2: // TEXT_ALIGNMENT_RIGHT
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do the TEXT_ALIGNMENT_* values come from?

I don't see them defined anywhere, they are part of Ti.UI in the API.

Please add them and update the apidoc for them as well. The actual defined values should then be used here in the switch instead of bare integers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added N_TEXT_ALIGNMENT enum

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be actually defined as Javascript constants, so users of the Ti API can use them. Also update apidoc

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

textControl_->textStyle()->setAlignment(bb::cascades::TextAlignment::ForceRight);
break;
default:
textControl_->textStyle()->setAlignment(bb::cascades::TextAlignment::Default);
break;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the event of an unrecognized alignment value, is this the intended behaviour? Or should the alignment not change instead?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In these cases where we silently ignore unknown values, it would be good to log a line saying that we received unknown value and log what the received value was.

Same goes for setFont

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

}

return NATIVE_ERROR_OK;
}

int NativeAbstractTextControlObject::setFont(TiObject* obj)
{
QMap<QString, QString> font;
int error = NativeControlObject::getFontObject(obj, font);
if (error != NATIVE_ERROR_OK)
{
return error;
}

QMap<QString, QString>::const_iterator it = font.begin();
for (; it != font.end(); ++it)
{
if (it.key().compare("fontFamily") == 0)
{
textControl_->textStyle()->setFontFamily(it.value());
}
else if (it.key().compare("fontSize") == 0)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can declare const variables, instead of using string value (e.g const string fontSize = "fontSize")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address the same comment, for other hard coded values as well (e.g. fontFamily, normal, intalic, fontWeithg, etc.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constats defined

{
bool bSucceed;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

variable name, please use either the past tense "bSucceeded" or the name "bSuccess"

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

float size = it.value().toFloat(&bSucceed);
if (bSucceed)
{
textControl_->textStyle()->setSize(size);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if bSucceeded is false, that should be logged too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

}
else if (it.key().compare("fontStyle") == 0)
{
if (it.value().compare("normal") == 0)
{
textControl_->textStyle()->setFontStyle(bb::cascades::FontStyle::Normal);
}
else if (it.value().compare("italic") == 0)
{
textControl_->textStyle()->setFontStyle(bb::cascades::FontStyle::Italic);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the else clause should be logged too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

else
{
textControl_->textStyle()->setFontStyle(bb::cascades::FontStyle::Default);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the correct behaviour. if fontstyle is default then set to default (don't think this is a valid use case though), but if it's anything else then it may be better to just not change it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

}
}
else if (it.key().compare("fontWeight") == 0)
{
if (it.value().compare("normal") == 0)
{
textControl_->textStyle()->setFontWeight(bb::cascades::FontWeight::Normal);
}
else if (it.value().compare("bold") == 0)
{
textControl_->textStyle()->setFontWeight(bb::cascades::FontWeight::Bold);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the else clause should be logged too

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

else
{
textControl_->textStyle()->setFontWeight(bb::cascades::FontWeight::Default);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above comment

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

}
}
}

return NATIVE_ERROR_OK;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a more general behaviour note, please verify how badly formatted fonts behave on other platforms. are errors/exceptions raised or are they silently ignored? This goes for both the key string and the value string

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In cases something went wrong default style will be used.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not answer the question. is you're answer relating to Android or Iphone or BB? Please specify. Have you tried it on another platform?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found no documentation on that

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not asking for documentation, i'm asking you to try it out and report your findings.

}
47 changes: 47 additions & 0 deletions blackberry/tibb/NativeAbstractTextControlObject.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
/**
* Appcelerator Titanium Mobile
* Copyright (c) 2009-2012 by Appcelerator, Inc. All Rights Reserved.
* Licensed under the terms of the Apache Public License
* Please see the LICENSE included with this distribution for details.
*/

#ifndef NATIVEABSTRACTTEXTCONTROLOBJECT_H_
#define NATIVEABSTRACTTEXTCONTROLOBJECT_H_

#include "NativeControlObject.h"

//forward declaration
namespace bb
{
namespace cascades
{
class AbstractTextControl;
}
}

/*
* NativeAbstractTextControlObject
*
* UI: Abstract Text Control
*/

class NativeAbstractTextControlObject : public NativeControlObject
{
public:
virtual int setText(TiObject* obj);
virtual int setFont(TiObject* obj);
virtual int setColor(TiObject* obj);
virtual int setTextAlign(TiObject* obj);

protected:
explicit NativeAbstractTextControlObject();
virtual ~NativeAbstractTextControlObject();
virtual bb::cascades::AbstractTextControl* getTextControl() const;
virtual void setTextControl(bb::cascades::AbstractTextControl* textControl);

private:
bb::cascades::AbstractTextControl* textControl_;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also disable the copy ctor and assignment operator

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done



#endif /* NATIVEABSTRACTTEXTCONTROLOBJECT_H_ */
35 changes: 34 additions & 1 deletion blackberry/tibb/NativeControlObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,12 @@ int NativeControlObject::setImage(TiObject* obj)
return NATIVE_ERROR_NOTSUPPORTED;
}

PROP_SETTER(setFont)
int NativeControlObject::setFont(TiObject* obj)
{
return NATIVE_ERROR_NOTSUPPORTED;
}

// PROP_SETTING_FUNCTION resolves the static name of the function, e.g.,
// PROP_SETTING_FUNCTION(setBackgroundColor) resolves to "prop_setBackgroundColor"

Expand Down Expand Up @@ -195,7 +201,7 @@ static vector<NATIVE_PROPSET_CALLBACK> initFunctionMap()
vect[N_PROP_COLOR] = PROP_SETTING_FUNCTION(setColor);
vect[N_PROP_ELLIPSIZE] = NULL;
vect[N_PROP_FOCUSABLE] = NULL;
vect[N_PROP_FONT] = NULL;
vect[N_PROP_FONT] = PROP_SETTING_FUNCTION(setFont);
vect[N_PROP_HEIGHT] = NULL;
vect[N_PROP_HIGHLIGHTED_COLOR] = NULL;
vect[N_PROP_HINT_TEXT] = NULL;
Expand Down Expand Up @@ -337,3 +343,30 @@ int NativeControlObject::getStringArray(TiObject* obj, QVector<QString>& value)
}
return NATIVE_ERROR_OK;
}

int NativeControlObject::getFontObject(TiObject* obj, QMap<QString, QString>& props)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since we don't do any specific font related parsing, we can rename this function as e.g. getArrayObject and use it whenever we want to parse Array

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed

{
Handle<Value> v8value = obj->getValue();
if (v8value.IsEmpty() || !v8value->IsObject())
{
return NATIVE_ERROR_INVALID_ARG;
}
Handle<Array> array = Handle<Array>::Cast(v8value);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also test that v8value is indeed an array before casting it. So the above would test for !IsArray instead of !IsObject

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that is not array. Changed casting into object instead of array.

Handle<Array> keys = array->GetPropertyNames();

for (unsigned int i = 0; i < keys->Length(); i++)
{
v8::Handle<v8::String> key = keys->Get(v8::Integer::New(i))->ToString();
v8::String::Utf8Value keyStr(key);
v8::Handle<v8::String> value = array->Get(key)->ToString();
v8::String::Utf8Value valueStr(value);
const char* cstrKey = *keyStr;
QString strKey = cstrKey;
const char* cstrValue = *valueStr;
QString strValue = cstrValue;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's be explicit about the string conversion to qstring: Do this" QString strValue = QString::fromUtf8(cstrValue); same for strKey

FYI: cstr* pointers are not necessary steps

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


props.insert(strKey, strValue);
}

return NATIVE_ERROR_OK;
}
2 changes: 2 additions & 0 deletions blackberry/tibb/NativeControlObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ class NativeControlObject : public NativeObject
virtual int setOptions(TiObject* obj);
virtual int setSelectedIndex(TiObject* obj);
virtual int setImage(TiObject* obj);
virtual int setFont(TiObject* obj);
static int getColorComponents(TiObject* obj, float* r, float* g, float* b, float* a);
static int getBoolean(TiObject* obj, bool* value);
static int getString(TiObject* obj, QString& str);
static int getFloat(TiObject* obj, float* value);
static int getInteger(TiObject* obj, int* value);
static int getStringArray(TiObject* obj, QVector<QString>& value);
static int getFontObject(TiObject* obj, QMap<QString, QString>& props);
protected:
NativeControlObject();
virtual ~NativeControlObject();
Expand Down
39 changes: 1 addition & 38 deletions blackberry/tibb/NativeLabelObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -29,47 +29,10 @@ int NativeLabelObject::getObjectType() const
return N_TYPE_LABEL;
}

int NativeLabelObject::setText(TiObject* obj)
{
QString str;
int error = NativeControlObject::getString(obj, str);
if (error != NATIVE_ERROR_OK)
{
return error;
}
label_->setText(str);
return NATIVE_ERROR_OK;
}

int NativeLabelObject::setColor(TiObject* obj)
{
float r;
float g;
float b;
float a;

int error = NativeControlObject::getColorComponents(obj, &r, &g, &b, &a);
if (error != NATIVE_ERROR_OK)
{
return error;
}
bb::cascades::Color cscolor = bb::cascades::Color::fromRGBA(r, g, b, a);
// TODO: setTextColor is not yet supported by Cascades. When it becomes
// available, un-comment out the next line.
//label_->setTextColor(cscolor);
return NATIVE_ERROR_OK;
}

int NativeLabelObject::setTextAlign(TiObject* obj)
{
// TODO: finish
return NATIVE_ERROR_OK;
}

int NativeLabelObject::initialize(TiEventContainerFactory*)
{
label_ = bb::cascades::Label::create();
setControl(label_);
setTextControl(label_);
// TODO: Set label layout here
return NATIVE_ERROR_OK;
}
Expand Down
7 changes: 2 additions & 5 deletions blackberry/tibb/NativeLabelObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
#ifndef NATIVELABELOBJECT_H_
#define NATIVELABELOBJECT_H_

#include "NativeControlObject.h"
#include "NativeAbstractTextControlObject.h"
#include <bb/cascades/Label>

/*
Expand All @@ -17,14 +17,11 @@
* UI: Label control
*/

class NativeLabelObject : public NativeControlObject
class NativeLabelObject : public NativeAbstractTextControlObject
{
public:
static NativeLabelObject* createLabel();
virtual int getObjectType() const;
virtual int setText(TiObject* obj);
virtual int setColor(TiObject* obj);
virtual int setTextAlign(TiObject* obj);
virtual int initialize(TiEventContainerFactory* containerFactory);
virtual NAHANDLE getNativeHandle() const;

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do these changes (all changes to NativeLabelObject) for NativeTextFieldObject

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to derive from NativeAbstractTextControl class.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that this class still has a bunch of old style setters. some rendered obsolete by your addition of setHeight and setWidth in the NativeControlObject and others that need to be updated to take in a TiObject* and also be added to NativeControlObject.

Can you make these changes or make sure that a Jira issue exists to do that cleanup task. You could also check with DavidC as he might be going over and fixing these for the UI.View work.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

Expand Down
5 changes: 5 additions & 0 deletions blackberry/tibb/TiUIBase.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ const static TiProperty g_tiProperties[] =
{
"image", TI_PROP_PERMISSION_READ | TI_PROP_PERMISSION_WRITE,
N_PROP_IMAGE
},

{
"font", TI_PROP_PERMISSION_READ | TI_PROP_PERMISSION_WRITE,
N_PROP_FONT
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should be in order, can you please order the ones that were added to the end?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

};

Expand Down