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-9539: BlackBerry: Implement include() #91

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
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 blackberry/tibb/TiMessageStrings.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ namespace Ti
namespace Msg
{

TIMESSAGESTRINGS_CONST_DEF(char*, Include_file_not_found, "Include file not found");
TIMESSAGESTRINGS_CONST_DEF(char*, INTERNAL__Global_String_symbol_is_not_an_object, "INTERNAL: Global String symbol is not an object");
TIMESSAGESTRINGS_CONST_DEF(char*, INTERNAL__args0_is_not_a_number, "args[0] is not a number.");
TIMESSAGESTRINGS_CONST_DEF(char*, Missing_argument, "Missing argument");
Expand Down
70 changes: 68 additions & 2 deletions blackberry/tibb/TiTitaniumObject.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,13 @@
*/

#include "TiTitaniumObject.h"
#include "TiUIObject.h"
#include "TiAPIObject.h"
#include "TiGenericFunctionObject.h"
#include "TiLogger.h"
#include "TiMessageStrings.h"
#include "TiUIObject.h"

#include <fstream>

TiTitaniumObject::TiTitaniumObject()
: TiObject("Titanium")
Expand All @@ -28,9 +33,10 @@ TiObject* TiTitaniumObject::createObject(NativeObjectFactory* objectFactory)

void TiTitaniumObject::onCreateStaticMembers()
{
// TODO: remove hard coded version number

Choose a reason for hiding this comment

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

Too far above. Should be exactly above the line with New(2.0)

Copy link
Author

Choose a reason for hiding this comment

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

Done :)

ADD_STATIC_TI_VALUE("buildDate", String::New(__DATE__), this);
ADD_STATIC_TI_VALUE("version", Number::New(2.0), this);
// TODO: remove hard coded version number
TiGenericFunctionObject::addGenericFunctionToParent(this, "include", this, _include);

Choose a reason for hiding this comment

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

Put this line after the TODO, or better yet move the TODO onto or above the line it's talking about

Copy link
Author

Choose a reason for hiding this comment

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

Done

TiUIObject::addObjectToParent(this, objectFactory_);
TiAPIObject::addObjectToParent(this);
}
Expand All @@ -39,3 +45,63 @@ bool TiTitaniumObject::canAddMembers() const
{
return false;
}

Handle<Value> TiTitaniumObject::_include(void* userContext, TiObject* caller, const Arguments& args)
{
if (args.Length() < 1)
{
return ThrowException(String::New(Ti::Msg::Missing_argument));
}

Local<Value> javaScript = args[0];
if (!javaScript->IsString())
{
javaScript = javaScript->ToString();
}

static string sRelDir = "";

string base = "app/native/assets/";

Choose a reason for hiding this comment

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

This can be static const

Copy link
Author

Choose a reason for hiding this comment

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

Done

string fullPath = base + sRelDir + *String::Utf8Value(javaScript);

Choose a reason for hiding this comment

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

Is there nothing in userContext, caller, or something in v8 that can tell us which file it is being called from? It could simplify things if we add that functionality.

Copy link
Author

Choose a reason for hiding this comment

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

TODO added.

string filename = *String::Utf8Value(javaScript);

Choose a reason for hiding this comment

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

This can be done before and used in fullPath so there's less conversion

Choose a reason for hiding this comment

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

Why is sRelDir empty? Can it be changed?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

@dcampbell-macadamian: It changed when included filename contains directory. See the "if (slash_pos != std::string::npos)" block.


// Get the directory before slash

Choose a reason for hiding this comment

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

This is an interesting way to approach relative directories, but I don't think it will work for windows with a url or event handlers. Windows with a url don't seem to be used much in KS and the docs say calling from an event handler "does not work consistently across platforms" so maybe we can get by with this for now. What I'd like to see is a TODO saying that this needs to be checked if it will work properly in those scenarios.

Choose a reason for hiding this comment

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

Actually, I'd also like you to take a look at android/runtime/common/src/js/titanium.js where it does Titanium.include = TiInclude; and also look at the TiInclude function/comments/friends itself. They seem to be trying to handle the contexts within JavaScript itself by creating wrapper objects and storing the baseUrl. If we store the baseUrl in TiObject it could help things.
This isn't simple stuff :)

Copy link
Author

Choose a reason for hiding this comment

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

TODO added

std::string::size_type slash_pos = filename.rfind("/");
if (slash_pos != std::string::npos)
{
slash_pos++;
sRelDir += filename.substr(0, slash_pos);
}

ifstream ifs(fullPath.c_str());

if (ifs.bad())

Choose a reason for hiding this comment

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

I just realized this won't be true if the file does not exist. Use fail() instead, which also checks the badbit.

Copy link
Author

Choose a reason for hiding this comment

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

Done with !ifs.

{
return ThrowException(String::New(Ti::Msg::Include_file_not_found));
}

string buffer;
getline(ifs, buffer, string::traits_type::to_char_type(string::traits_type::eof()));
ifs.close();

TryCatch tryCatch;
Handle<Script> compiledScript = Script::Compile(String::New(buffer.c_str()));
if (compiledScript.IsEmpty())
{
String::Utf8Value error(tryCatch.Exception());
TiLogger::getInstance().log(string(*error) + "\n");
return ThrowException(tryCatch.Exception());

Choose a reason for hiding this comment

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

This seems like duplicated effort. Won't ThrowException display the error in the log?

Choose a reason for hiding this comment

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

You should add the name of the file that can't be found to the exception.

Copy link
Author

Choose a reason for hiding this comment

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

Remove 1st logging.

}
Handle<Value>result = compiledScript->Run();
if (result.IsEmpty())
{
String::Utf8Value error(tryCatch.Exception());
TiLogger::getInstance().log(string(*error) + "\n");
return ThrowException(tryCatch.Exception());
}

// Reset relative path
sRelDir = "";

return Undefined();
}
5 changes: 4 additions & 1 deletion blackberry/tibb/TiTitaniumObject.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,10 @@ class TiTitaniumObject : public TiObject
virtual bool canAddMembers() const;

private:
TiTitaniumObject();
explicit TiTitaniumObject();
TiTitaniumObject(const TiTitaniumObject&);
TiTitaniumObject& operator=(const TiTitaniumObject&);
static Handle<Value> _include(void* userContext, TiObject* caller, const Arguments& args);
NativeObjectFactory* objectFactory_;
};

Expand Down