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

Already on GitHub? Sign in to your account

FileUploader extension for Smartphone #18

Merged
merged 10 commits into from Nov 29, 2011

Conversation

Projects
None yet
5 participants
Member

jcarty commented Sep 26, 2011

Hello,

I'd like to contribute this extension to the Community APIs.

Thanks.

Member

tneil commented Sep 27, 2011

Hi Jerome,

Can you add the Apache license headers to your XML and Java files

Member

jcarty commented Sep 27, 2011

Same as below?

/*

  • Copyright 2010-2011 Research In Motion Limited.
    *
  • Licensed under the Apache License, Version 2.0 (the "License");
  • you may not use this file except in compliance with the License.
  • You may obtain a copy of the License at
    *
  • http://www.apache.org/licenses/LICENSE-2.0
    *
  • Unless required by applicable law or agreed to in writing, software
  • distributed under the License is distributed on an "AS IS" BASIS,
  • WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  • See the License for the specific language governing permissions and
  • limitations under the License.
    */

Jerome Carty
Twitter: @jcarty | @KisaiLabs
http://www.kisailabs.com

-----Original Message-----
From: Tim Neil [mailto:reply@reply.github.com]
Sent: Tuesday, September 27, 2011 10:12 AM
To: Jerome Carty
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

Hi Jerome,

Can you add the Apache license headers to your XML and Java files

Reply to this email directly or view it on GitHub:
#18 (comment)

Member

tneil commented Sep 27, 2011

Yup.. Perfect..

-----Original Message-----
From: Jerome Carty [mailto:reply@reply.github.com]
Sent: Tuesday, September 27, 2011 11:21 AM
To: Tim Neil
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

Same as below?

/*

  • Copyright 2010-2011 Research In Motion Limited.
    *
  • Licensed under the Apache License, Version 2.0 (the "License");
  • you may not use this file except in compliance with the License.
  • You may obtain a copy of the License at
    *
  • http://www.apache.org/licenses/LICENSE-2.0
    *
  • Unless required by applicable law or agreed to in writing, software
  • distributed under the License is distributed on an "AS IS" BASIS,
  • WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  • See the License for the specific language governing permissions and
  • limitations under the License.
    */

Jerome Carty
Twitter: @jcarty | @KisaiLabs
http://www.kisailabs.com

-----Original Message-----
From: Tim Neil [mailto:reply@reply.github.com]
Sent: Tuesday, September 27, 2011 10:12 AM
To: Jerome Carty
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

Hi Jerome,

Can you add the Apache license headers to your XML and Java files

Reply to this email directly or view it on GitHub:
#18 (comment)

Reply to this email directly or view it on GitHub:
#18 (comment)


This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

Member

jcarty commented Sep 27, 2011

Notice anything else that needs fixing? Done.

Jerome Carty
Twitter: @jcarty | @KisaiLabs
http://www.kisailabs.com

-----Original Message-----
From: Tim Neil [mailto:reply@reply.github.com]
Sent: Tuesday, September 27, 2011 11:22 AM
To: Jerome Carty
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

Yup.. Perfect..

-----Original Message-----
From: Jerome Carty [mailto:reply@reply.github.com]
Sent: Tuesday, September 27, 2011 11:21 AM
To: Tim Neil
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

Same as below?

/*

  • Copyright 2010-2011 Research In Motion Limited.
    *
  • Licensed under the Apache License, Version 2.0 (the "License");
  • you may not use this file except in compliance with the License.
  • You may obtain a copy of the License at
    *
  • http://www.apache.org/licenses/LICENSE-2.0
    *
  • Unless required by applicable law or agreed to in writing, software
  • distributed under the License is distributed on an "AS IS" BASIS,
  • WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  • See the License for the specific language governing permissions and
  • limitations under the License.
    */

Jerome Carty
Twitter: @jcarty | @KisaiLabs
http://www.kisailabs.com

-----Original Message-----
From: Tim Neil [mailto:reply@reply.github.com]
Sent: Tuesday, September 27, 2011 10:12 AM
To: Jerome Carty
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

Hi Jerome,

Can you add the Apache license headers to your XML and Java files

Reply to this email directly or view it on GitHub:
#18 (comment)

Reply to this email directly or view it on GitHub:
#18 (comment)


This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

Reply to this email directly or view it on GitHub:
#18 (comment)

Member

tneil commented Sep 27, 2011

I'll ask Jeff or Alex to give it a review...

isWhitelist=true is no longer being used, please remove it.

I really like this objectToHashtable helper, it keeps the invoke() method focused!

This run() method is very busy because it goes through the low level details of setting up a connection, its headers, data boundaries and actual data. I'm wondering if moving this code to a helper class that takes the file and its MIME type as constructor params and has a .upload() function would make this easier to read. It would also go a long way toward helping other reuse the HTTP connection code without thinking about the details too much.

Owner

jcarty replied Oct 5, 2011

After reviewing this again, the only thing I'd be able to do is move UploadRunnable to a new class file. Every parameter passed in is required for the most flexible upload (think Facebook, Twitter, etc.).

I've changed everything else except this. Let me know if this makes sense.

ababut commented on 7f26e42 Oct 4, 2011

Jerome, this looks great! I made a few comments, but overall the extension is very clean. Let me know what you think.

Owner

jcarty replied Oct 4, 2011

I'll get those changes made. Everything you suggested is either required or makes sense.

ababut commented Oct 6, 2011

Moving the runnable to another class is perfect. It breaks that long class up. Have you pushed the latest changes to Github because I don't see them in the pull request.

Member

jcarty commented Oct 6, 2011

Not yet. I was waiting for response to the UploadRunnable. Once I extract it, I will commit the changes.
------Original Message------
From: Alex Babut
To: Jerome Carty
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)
Sent: Oct 6, 2011 10:11 AM

Moving the runnable to another class is perfect. It breaks that long class up. Have you pushed the latest changes to Github because I don't see them in the pull request.

Reply to this email directly or view it on GitHub:
#18 (comment)

Jerome Carty

Owner

kwallis commented Nov 8, 2011

Anything outstanding on this pull request at this point?

Member

jcarty commented Nov 9, 2011

Providing a few more options to make it as flexible as possible.

- Extension now reads <rim:connection> for transport order and options
- Timeout option added (defaults to 30000 milliseconds)
Contributor

jeffheifetz commented Nov 15, 2011

Everything is great from a code perspective

Member

jcarty commented Nov 15, 2011

Just fixed the following issue:

jcarty#1

It seems it wasn't linked properly in the pull request. Let me know if anything else needs to be done. Once this is in, I will submit more extensions.

@kwallis kwallis and 1 other commented on an outdated diff Nov 28, 2011

...hone/FileUploader/src/webworks/io/UploadRunnable.java
+
+ Logger.log("Setting mime type...");
+
+ if (_mimeType == null) {
+ _mimeType = MIMETypeAssociations.getMIMEType(_filePath);
+ if (_mimeType == null) {
+ _mimeType = HttpProtocolConstants.CONTENT_TYPE_IMAGE_JPEG;
+ }
+ }
+
+ if (!fc.exists()) {
+ Logger.error("File not found");
+ callErrorCallback(new String[] { _filePath + " not found" });
+ }
+
+ if (_url.indexOf("https") != -1) {
@kwallis

kwallis Nov 28, 2011

Owner

This will actually return true for any URL that contains 'https' anywhere in the URL. Safer would be to check for "https://", or at least check if https are the very first characters (think that would be indexof == 0?)

@jcarty

jcarty Nov 28, 2011

Member

Good point. Will make the change.

@kwallis

kwallis Nov 29, 2011

Owner

Thanks! I'll try to merge this tomorrow morning.

----- Original Message -----
From: Jerome Carty [mailto:reply@reply.github.com]
Sent: Monday, November 28, 2011 05:50 PM
To: Ken Wallis
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

  •        Logger.log("Setting mime type...");
    
  •     if (_mimeType == null) {
    
  •            _mimeType = MIMETypeAssociations.getMIMEType(_filePath);
    
  •            if (_mimeType == null) {
    
  •                _mimeType = HttpProtocolConstants.CONTENT_TYPE_IMAGE_JPEG;
    
  •            }  
    
  •        }
    
  •        if (!fc.exists()) {
    
  •         Logger.error("File not found");
    
  •         callErrorCallback(new String[] { _filePath + " not found" });
    
  •        } 
    
  •     if (_url.indexOf("https") != -1) {
    

Fixed

Jerome Carty
Twitter: @jcarty | @KisaiLabs
http://www.kisailabs.com

-----Original Message-----
From: Ken Wallis [mailto:reply@reply.github.com]
Sent: Monday, November 28, 2011 1:19 PM
To: Jerome Carty
Subject: Re: [WebWorks-Community-APIs] FileUploader extension for Smartphone (#18)

  •        Logger.log("Setting mime type...");
    
  •     if (_mimeType == null) {
    
  •            _mimeType = MIMETypeAssociations.getMIMEType(_filePath);
    
  •            if (_mimeType == null) {
    
  •                _mimeType = HttpProtocolConstants.CONTENT_TYPE_IMAGE_JPEG;
    
  •            }  
    
  •        }
    
  •        if (!fc.exists()) {
    
  •         Logger.error("File not found");
    
  •         callErrorCallback(new String[] { _filePath + " not found" });
    
  •        } 
    
  •     if (_url.indexOf("https") != -1) {
    

This will actually return true for any URL that contains 'https' anywhere in the URL. Safer would be to check for "https://", or at least check if https are the very first characters (think that would be indexof == 0?)


Reply to this email directly or view it on GitHub:
https://github.com/blackberry/WebWorks-Community-APIs/pull/18/files#r254950


Reply to this email directly or view it on GitHub:
https://github.com/blackberry/WebWorks-Community-APIs/pull/18/files#r256077


This transmission (including any attachments) may contain confidential information, privileged material (including material protected by the solicitor-client or other applicable privileges), or constitute non-public information. Any use of this information by anyone other than the intended recipient is prohibited. If you have received this transmission in error, please immediately reply to the sender and delete this information from your system. Use, dissemination, distribution, or reproduction of this transmission by unintended recipients is not authorized and may be unlawful.

@jcarty

jcarty Nov 29, 2011

Member

Got two more for ya. Just making sure this one is good.

kwallis pushed a commit that referenced this pull request Nov 29, 2011

Merge pull request #18 from jcarty/master
FileUploader extension for Smartphone

@kwallis kwallis merged commit 4dbcb8a into blackberry:master Nov 29, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment