Le mardi 27 janvier 2009 à 18:30 -0500, Chris Frey a écrit :
> I have a few quibbles with this patch, and if you have time to fix
them,
> that would be great. If not, just let me know, and I can do it.
I can corrige...
> 1) any code that accesses protocol values should be careful about
endian
> issues... I see that you have done this in some places, which is
great.
> The new function:
>
> +unsigned int JLPacket::Size()
> +{
> + Protocol::CheckSize(m_receive, SB_JLRESPONSE_HEADER_SIZE);
> + MAKE_JLPACKET(rpack, m_receive);
> + return rpack->u.response.expect;
> +}
>
> should return btohs(rpack->u.response.expect);
>
> Note that "btohs" stands for "Blackberry to Host, short", and is
supposed
> to be analogous to the standard network functions ntohs() and friends.
Fixed
> 2) JavaLoader::GetScreenshot() is still using hard coded values, such
as
> 0x64, which has a define for it (ACK is probably the right
define)
Fixed
> 3) Note that tabs are 8 characters, so reindenting code in
JLPacket::PacketData
> will just have to be undone again, by one of us. :-)
I'm used to work with tab = 4 (I use VIM)
I have set VIM to use tab = 8 for the Barry project
> 4) I think we should stick to the command line syntax available in the
Windows
> javaloader, similar to Josh Kropf's patches to bjavaloader, to
make it
> easy for users to switch between Linux and Windows and use the
same
> commands. So instead of a new command 'bscreenshot', we should
add a
> 'screenshot' command argument to bjavaloader.
>
> The windows syntax is:
>
> javaloader screenshot <.bmp file>
Ok, I have removed bscreenshot binary and I have add this option into
bjavaloader.
We can make easily an alias :
alias bscreenshot='bjavaloader screenshot'
> 5) I'd prefer to have a struct for the BMP header in bscreenshot.
Endian
> issues should be handled there too, to keep Barry completely
> cross platform. The struct is optional, but the endian issues
> should be addressed.
Of course, I'm going to work with struct to be cleaner :)
I think that I'm going to declare BMP struct into an include file.
(I want to avoid a dependance with other library)
You can commit the patch ; I send you a patch about BMP later.
I have updated the patch on my web site :
http://www.progweb.com/modules/blackberry/bscreenshot/
my git log :
Remove bscreenshot binary
Add screenshot option to bjavaloader binary
Add comments in the code
Use defines beside of integer constant
Use b2hons to be compliant with evrywhat host
I think that I'm going to add a ZSH completion file for Barry command
line. (except if it already exists)
Regards,
--
Nicolas VIVIEN
------------------------------------------------------------------------------
This SF.net email is sponsored by:
SourcForge Community
SourceForge wants to tell your story.
http://p.sf.net/sfu/sf-spreadtheword
_______________________________________________
Barry-devel mailing list
Barry-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/barry-devel
