Skip to main content

Code review for new code submission from Davy Preuveneers (4/24/2008)

28 replies [Last post]
Anonymous

This is a call for external code review of a new code submission from
Davy Preuveneers (4/24/2008).

His changes include changes to MIDP, Personal Basis/Personal Profile,
and CLDC:

* The Alert patch has been extended a bit with a new method to play custom
sounds (path is harded coded though), but the current behavior is to still
play the default system sounds. This should provide some audio feedback if
JSR 135 is not there.

* The AwtPPC and AwtPPCStub patches convert some path definitions with
POSIX2HOST and add new missing method (clearBackground). It also provides
a dummy PPCRobotHelper and related classes and sources to get the Personal
Profile compilation working.

* The CLDC and MIDP patches currently only add configuration support for
WM2003 (to compile with eVC4).

Here are the code diffs (vs. rev. 11040 of the phoneME SVN repo):

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/10/Alert.re...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.r...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCSt...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/15/Suite.re...

Davy, please feel free to add any additional comments to this thread in the forums.

Everyone else, please take a look at the above diffs and send in your comments and/or questions on how they look for committing to our repository.

This code review will be open until Fri. 5/23/2008.

Thanks,

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Reply viewing options

Select your preferred way to display the comments and click "Save settings" to activate your changes.
davyp
Offline
Joined: 2007-01-03

Kaih,

I can confirm I have the same issues. I worked around the boolean issue
with a pragma where I override the include of "jmorecfg.h" with another
version where I first test if HAVE_BOOLEAN is already defined, and if
not define it as declared earlier (unsigned char) before including the
original header again. I am not quite sure if this is the proper way to fix
this problem.

The key strokes do not work either for me (I cannot play the Jonix game),
and I have been debugging it for a while but haven't had the time to
look into it in more detail. What I do know already is that the native
Window receives the key stroke events.(I also have this problem
with the Gtk 1.2 port). The remaining issue is the hard coded
QtRobotHelper dependency.

One question: did you compile a static build, or a dynamic build? I have
been running into issues with the static build lately. A static build is causing
a OutOfMemoryError/invalid peer exception when creating a
PPCFramePeer instance, while a dynamic build does not have this issue.

Davy

cjplummer
Offline
Joined: 2006-10-16

I just glanced at the diffs, and below is clearly the problem. WCECOMPAT_LIB_SRC_DIR cannot be in HOST form since it is used in a rule dependency. Davy is probably not seeing problems because some older versions of make support this, but not the newer versions. The other variables should also be cleaned up because I'm not certain where they are used.

Chris

diff -Naur orig/cdc/build/win32/defs_personal_pocketpc.mk patched/cdc/build/win32/defs_personal_pocketpc.mk
--- orig/cdc/build/win32/defs_personal_pocketpc.mk 2008-02-18 22:19:38.000000000 +0100
+++ patched/cdc/build/win32/defs_personal_pocketpc.mk 2008-05-23 00:00:59.000000000 +0200
@@ -46,9 +49,9 @@
WCECOMPAT_LINK_PATHNAME = $(CVM_BINDIR)/$(LIB_PREFIX)$(WCECOMPAT_LIB_NAME)$(LIB_LINK_SUFFIX)

WCECOMPAT_LIB_LIBS =
-WCECOMPAT_LIB_SRC_DIR = $(CVM_TOP)/src/win32/personal/native/compat
+WCECOMPAT_LIB_SRC_DIR = $(call POSIX2HOST, $(CVM_TOP)/src/win32/personal/native/compat)
WCECOMPAT_LIB_OBJS_FILES = $(patsubst %.o,$(CVM_OBJDIR)/%.o,$(WCECOMPAT_LIB_OBJS))
-WCECOMPAT_LIB_RESOURCES_FILES = $(patsubst %.RES,$(CVM_OBJDIR)/%.RES,$(WCECOMPAT_LIB_RESOURCES))
+WCECOMPAT_LIB_RESOURCES_FILES = $(call POSIX2HOST, (patsubst %.RES,$(CVM_OBJDIR)/%.RES,$(WCECOMPAT_LIB_RESOURCES)))
WCECOMPAT_LIB_CONTENTS = $(WCECOMPAT_LIB_OBJS_FILES) $(WCECOMPAT_LIB_RESOURCES_FILES)

#

davyp
Offline
Joined: 2007-01-03

Chris,

You might be right. I am using make 3.80 on cygwin and not the latest
version 3.81. I downgraded to this version a while back after facing
build problems like those mentioned here:

http://forums.java.net/jive/message.jspa?messageID=203407

Davy

hinkmond
Offline
Joined: 2003-12-01

I had some time to catch up on the code submission from Davy Preuveneers from earlier this year. We have now completed the code review and we are ready to merge it back to the trunk.

For those of you following this code submission, please check the latest code diffs from the following branches and try building and testing:

cdc:
https://phoneme.dev.java.net/svn/phoneme/components/cdc/branches/code-su...

cldc:
https://phoneme.dev.java.net/svn/phoneme/components/cldc/branches/code-s...

midp:
https://phoneme.dev.java.net/svn/phoneme/components/midp/branches/code-s...

pcsl:
https://phoneme.dev.java.net/svn/phoneme/components/pcsl/branches/code-s...

To review the changes, see the latest diffs at:

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/31/Alert.re...

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/32/AwtPPC.r...

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/33/AwtPPCSt...

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/34/CLDC.rev...

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/35/PCSL.rev...

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/36/Suite.re...

https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/37/Relative...

Please let me know if anyone has any comments or concerns before I merge the above back to the respective trunks by next week Fri. 29Aug2008.

Thanks,

Hinkmond

xyzzy (Dean)

This error is usually a result of the ':' in Windows paths.
These paths need to be in POSIX format.

Dean

phonemeadvanced@mobileandembedded.org wrote:
> Hinkmond and Davy,
>
> I tried to compile Foundation (works) and Personal (does not work).
>
> On Personal I get this error:
> /programs/java/PhoneME/Trunk/cdc/build/win32/rules_personal_pocketpc.mk:29: ***
> target pattern contains no `%'. Stop.
>
> However, the lines in that file reads:
> $(CVM_OBJDIR)/%.o: $(WCECOMPAT_LIB_SRC_DIR)/%.c
> $(AT)echo "...Compiling wcecompat objs: $@"
> $(WCECOMPAT_LIB_CC_RULE)
>
> It must be one of the patches that added a POSIX2HOST command to the variables and messed things up with backslashes.
>
> How can I print out a variable value at any time during the scan phase of make?
>
> Thanks,
> Kai
> [Message sent by forum member 'kaih' (kaih)]
>
> http://forums.java.net/jive/thread.jspa?messageID=280354
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

davyp
Offline
Joined: 2007-01-03

Hi Kaih,

I added those POSIX2HOST calls, because otherwise eVC4 gets to compile
source files with a path given in UNIX style with forward slashes. If I leave
them out, I get my first error when compiling wcecomp:

...Compiling wcecompat objs: obj/wceCompat.o
clarm.exe /nologo /c /W2 /MC ... /Foobj/wceCompat.o /phoneme_advanced_mr2/cdc/src/win32/personal/native/compat/wceCompat.c
Command line warning D4002 : ignoring unknown
option '/phoneme_advanced_mr2/cdc/src/win32/personal/native/compat/wceCompat.c'
Command line error D2003 : missing source filename

I left out many of the irrelevant compiler options, but none of the paths I
have uses backslashes.

Davy

xyzzy (Dean)

You might have better lucky using abs2rel instead of POSIX2HOST,
or calling POSIX2HOST later, in the %.o rule when clarm is invoked,
rather than in the make variable.

dl

phonemeadvanced@mobileandembedded.org wrote:
> Hi Kaih,
>
> I added those POSIX2HOST calls, because otherwise eVC4 gets to compile
> source files with a path given in UNIX style with forward slashes. If I leave
> them out, I get my first error when compiling wcecomp:
>
> ...Compiling wcecompat objs: obj/wceCompat.o
> clarm.exe /nologo /c /W2 /MC ... /Foobj/wceCompat.o /phoneme_advanced_mr2/cdc/src/win32/personal/native/compat/wceCompat.c
> Command line warning D4002 : ignoring unknown
> option '/phoneme_advanced_mr2/cdc/src/win32/personal/native/compat/wceCompat.c'
> Command line error D2003 : missing source filename
>
> I left out many of the irrelevant compiler options, but none of the paths I
> have uses backslashes.
>
> Davy
> [Message sent by forum member 'davyp' (davyp)]
>
> http://forums.java.net/jive/thread.jspa?messageID=280387
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

cjplummer
Offline
Joined: 2006-10-16

> You might have better lucky using abs2rel instead of
> POSIX2HOST,
> or calling POSIX2HOST later, in the %.o rule when
> clarm is invoked,
> rather than in the make variable.
>

Our policy is to only use POSIX2HOST on variables you know are only destined to be used as command line arguments. This usually means deferring the POSIX2HOST until either the command line itself, or when creating a variable to be used on the command line. Do it any sooner and you risk having problems like using the variable in a rule target or dependency.

Chris

hinkmond
Offline
Joined: 2003-12-01

> > You might have better lucky using abs2rel instead
> of
> > POSIX2HOST,
> > or calling POSIX2HOST later, in the %.o rule when
> > clarm is invoked,
> > rather than in the make variable.
> >
>
> Our policy is to only use POSIX2HOST on variables you
> know are only destined to be used as command line
> arguments. This usually means deferring the
> POSIX2HOST until either the command line itself, or
> when creating a variable to be used on the command
> line. Do it any sooner and you risk having problems
> like using the variable in a rule target or
> dependency.

Hi Davy,

Do you have a way in your defs_personal_pocketpc.mk changes to move the call to POSIX2HOST to later on the command line?

If there's a way to refactor to defer that call like Chris suggests, send me the new diff and I'll update your code submission. If you need help with that or need suggestions how to move it, let us know.

Thanks,

Hinkmond

kaih
Offline
Joined: 2008-06-14

I found a solution over the weekend. I just have no time currently to describe it in depth. My bread and butter jobs has prio.
There were a couple other dings in the Personal profile build, even after applying the latest patch files here, e.g. the jpeg boolean definition :-( and some other oddities in the build system.
But in essence I could build the Personal system out of the SVN Trunk with the patch set here, and a couple minor tweaks.

It seemed to work fine with the start class personal.DemoFrame out of democlasses.jar.
The only thing that never seems to work (also not with much older test builds) are key strokes in the Events tab, mouse movement and clicks work. On my PC with Java6 key strokes are recognized in the Events tab.

Kai

Hinkmond Wong

phonemeadvanced@mobileandembedded.org wrote:
> I found a solution over the weekend. I just have no time currently to describe it in depth. My bread and butter jobs has prio.
> There were a couple other dings in the Personal profile build, even after applying the latest patch files here, e.g. the jpeg boolean definition :-( and some other oddities in the build system.
> But in essence I could build the Personal system out of the SVN Trunk with the patch set here, and a couple minor tweaks.
>
> It seemed to work fine with the start class personal.DemoFrame out of democlasses.jar.
> The only thing that never seems to work (also not with much older test builds) are key strokes in the Events tab, mouse movement and clicks work. On my PC with Java6 key strokes are recognized in the Events tab.
>

Hi Kai,

When you have a chance (apart from your higher priority jobs :-)), can
you describe the keys on your device? Is it a regular QWERTY keyboard
on your device, or some type of special (WinMobile 5.0/6.0 Standard)
numeric or alpha-numeric keypad where you don't get key events? Or, is
it the virtual WinMobile keyboard that you're using?

Your device is probably generating different underlying native key codes
that are different from the default Windows Mobile keycodes that we have
currently in the Personal Profile build. The way to fix it is to debug
in the WinMobile porting layer code where you can output the WinMobile
key code to see if it matches what we have (see PPCComponentPeer.cpp
WindowsKeyToJavaKey()).

Hinkmond

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

davyp
Offline
Joined: 2007-01-03

I never got the key stroke events to work, from more than a year ago
when I got Personal Profile compiling again up to now. And it does not
work on any of the devices I tested it with.

Davy

Stephen Flores

I don't think that CdcMIDletSuiteLoader should call methods in MIDPConfig, that
would be circular dependency. I think the calling setProperties before calling
main is better, however having 2 methods that must be called in the correct
order and are not named for what is really being done can lead to them being
called out of order, I would like to see just method called instead of main in
the case where uninstalled suites are being run with properties such as
launchUninstalledSuite(args, props) which would call a *private* setProperties
method and then main.

Steve.

Jiangli Zhou wrote:
> Hi Davy,
>
> For the changes in MIDPLauncher.java and CdcMIDletSuiteLoader.java,
> using reflection to invoke the new CdcMIDletSuiteLoader.setProperties()
> is not really necessary. Since the CdcMIDletSuiteLoader class is only
> used for MIDP/CDC stack, it is safe to access sun.misc classes from
> there. I think you can add a new method (something like
> readSuiteProperties()) in sun.misc.MIDPConfig to read the the manifest
> attributes. That way you don't need to duplicate the while loop in two
> places. You can add another method in MIDPConfig (such as
> getSuiteProperties(), which returns the properties gathered from the
> manifest. You can call that method from CdcMIDletSuiteLoader to obtain
> the properties. With those, you don't need the new setProperties()
> method and the 'properties' filed in CdcMIDletSuiteLoader.
>
> Regards,
>
> Jiangli
>
> Hinkmond Wong wrote:
>> This is a call for external code review of a new code submission from
>> Davy Preuveneers (4/24/2008).
>>
>> His changes include changes to MIDP, Personal Basis/Personal Profile,
>> and CLDC:
>>
>> * The Alert patch has been extended a bit with a new method to play
>> custom sounds (path is harded coded though), but the current behavior
>> is to still play the default system sounds. This should provide some
>> audio feedback if JSR 135 is not there.
>>
>> * The AwtPPC and AwtPPCStub patches convert some path definitions with
>> POSIX2HOST and add new missing method (clearBackground). It also provides
>> a dummy PPCRobotHelper and related classes and sources to get the
>> Personal Profile compilation working.
>>
>> * The CLDC and MIDP patches currently only add configuration support
>> for WM2003 (to compile with eVC4).
>>
>>
>> Here are the code diffs (vs. rev. 11040 of the phoneME SVN repo):
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/10/Alert.re...
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.r...
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCSt...
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev...
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev...
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/15/Suite.re...
>>
>>
>>
>> Davy, please feel free to add any additional comments to this thread
>> in the forums.
>>
>> Everyone else, please take a look at the above diffs and send in your
>> comments and/or questions on how they look for committing to our
>> repository.
>>
>>
>> This code review will be open until Fri. 5/23/2008.
>>
>>
>> Thanks,
>>
>> Hinkmond
>>
>>
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
>> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Davy Preuveneers

Hi Steve, Jiangli,

Thank you both for your feedback. Is there any preference for which approach
to pursue? Steve seemed to prefer my first patch with a few modifications
without using MIDPConfig, while Jiangli was for the MIDPConfig approach.
Personally, I would go for the first patch with the changes proposed by
Steve, but I am open to other suggestions.

Thanks,
Davy

On Monday 12 May 2008 20:44:55 Jiangli Zhou wrote:
> Hi Davy,
>
> Sorry for the delay. Your new diff looks okay, except there are some
> lines exceeding 80 characters. Please make sure all lines do not exceed
> the 80-character limit.
>
> Thanks,
> Jiangli

On Friday 09 May 2008 04:42:10 Stephen Flores wrote:
> I don't think that CdcMIDletSuiteLoader should call methods in MIDPConfig,
> that would be circular dependency. I think the calling setProperties before
> calling main is better, however having 2 methods that must be called in the
> correct order and are not named for what is really being done can lead to
> them being called out of order, I would like to see just method called
> instead of main in the case where uninstalled suites are being run with
> properties such as launchUninstalledSuite(args, props) which would call a
> *private* setProperties method and then main.
>
> Steve.

--
Davy Preuveneers
K.U.Leuven - http://www.kuleuven.be
Department of Computer Science - http://www.cs.kuleuven.be
DistriNet - http://www.cs.kuleuven.be/~distrinet
Celestijnenlaan 200A, B-3001 Heverlee (Leuven), Belgium
Room: 02.152
Phone: (+32) (0)16 327853
E-Mail: Davy.Preuveneers@cs.kuleuven.be
Web: http://www.cs.kuleuven.be/~davy
[signature.asc]

Jiangli Zhou

Hi Davy,

I don't have problem with Steve's suggestion.

Regards,
Jiangli

Davy Preuveneers wrote:
> Hi Steve, Jiangli,
>
> Thank you both for your feedback. Is there any preference for which approach
> to pursue? Steve seemed to prefer my first patch with a few modifications
> without using MIDPConfig, while Jiangli was for the MIDPConfig approach.
> Personally, I would go for the first patch with the changes proposed by
> Steve, but I am open to other suggestions.
>
> Thanks,
> Davy
>
> On Monday 12 May 2008 20:44:55 Jiangli Zhou wrote:
>
>> Hi Davy,
>>
>> Sorry for the delay. Your new diff looks okay, except there are some
>> lines exceeding 80 characters. Please make sure all lines do not exceed
>> the 80-character limit.
>>
>> Thanks,
>> Jiangli
>>
>
> On Friday 09 May 2008 04:42:10 Stephen Flores wrote:
>
>> I don't think that CdcMIDletSuiteLoader should call methods in MIDPConfig,
>> that would be circular dependency. I think the calling setProperties before
>> calling main is better, however having 2 methods that must be called in the
>> correct order and are not named for what is really being done can lead to
>> them being called out of order, I would like to see just method called
>> instead of main in the case where uninstalled suites are being run with
>> properties such as launchUninstalledSuite(args, props) which would call a
>> *private* setProperties method and then main.
>>
>> Steve.
>>
>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Davy Preuveneers

Hi Steve,

I followed your suggestions for the new patch in attach and created a
launchUninstalledSuite(args, props) method in which I encapsulated the
code that was previously in the setProperties method.

I replaced the reflective call to main() with a call to the new
launchUninstalledSuite(args, props) method, because the property list can
never be empty. Even if no Jad file is specified, an uninstalled suite always
has a manifest with mandatory properties.

Regards,
Davy

On Tuesday 13 May 2008, Jiangli Zhou wrote:
> Hi Davy,
>
> I don't have problem with Steve's suggestion.
>
> Regards,
> Jiangli

> > On Friday 09 May 2008 04:42:10 Stephen Flores wrote:
> >> I don't think that CdcMIDletSuiteLoader should call methods in
> >> MIDPConfig, that would be circular dependency. I think the calling
> >> setProperties before calling main is better, however having 2 methods
> >> that must be called in the correct order and are not named for what is
> >> really being done can lead to them being called out of order, I would
> >> like to see just method called instead of main in the case where
> >> uninstalled suites are being run with properties such as
> >> launchUninstalledSuite(args, props) which would call a *private*
> >> setProperties method and then main.
> >>
> >> Steve.

--
Davy Preuveneers
K.U.Leuven - http://www.kuleuven.be
Department of Computer Science - http://www.cs.kuleuven.be
DistriNet - http://www.cs.kuleuven.be/~distrinet
Celestijnenlaan 200A, B-3001 Heverlee (Leuven), Belgium
Room: 02.152
Phone: (+32) (0)16 327853
E-Mail: Davy.Preuveneers@cs.kuleuven.be
Web: http://www.cs.kuleuven.be/~davy
[Suite.rev11206.diff]
[signature.asc]

Stephen Flores

Davy,

The changes look fine.

Steve.

Davy Preuveneers wrote:
> Hi Steve,
>
> I followed your suggestions for the new patch in attach and created a
> launchUninstalledSuite(args, props) method in which I encapsulated the
> code that was previously in the setProperties method.
>
> I replaced the reflective call to main() with a call to the new
> launchUninstalledSuite(args, props) method, because the property list can
> never be empty. Even if no Jad file is specified, an uninstalled suite always
> has a manifest with mandatory properties.
>
> Regards,
> Davy
>
>
> On Tuesday 13 May 2008, Jiangli Zhou wrote:
>> Hi Davy,
>>
>> I don't have problem with Steve's suggestion.
>>
>> Regards,
>> Jiangli
>
>
>
>>> On Friday 09 May 2008 04:42:10 Stephen Flores wrote:
>>>> I don't think that CdcMIDletSuiteLoader should call methods in
>>>> MIDPConfig, that would be circular dependency. I think the calling
>>>> setProperties before calling main is better, however having 2 methods
>>>> that must be called in the correct order and are not named for what is
>>>> really being done can lead to them being called out of order, I would
>>>> like to see just method called instead of main in the case where
>>>> uninstalled suites are being run with properties such as
>>>> launchUninstalledSuite(args, props) which would call a *private*
>>>> setProperties method and then main.
>>>>
>>>> Steve.
>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

hinkmond
Offline
Joined: 2003-12-01

> This is a call for external code review of a new code
> submission from
> Davy Preuveneers (4/24/2008).
>
> His changes include changes to MIDP, Personal
> Basis/Personal Profile,
> and CLDC:
>
> * The Alert patch has been extended a bit with a new
> method to play custom
> sounds (path is harded coded though), but the current
> behavior is to still
> play the default system sounds. This should provide
> some audio feedback if
> JSR 135 is not there.
>
> * The AwtPPC and AwtPPCStub patches convert some path
> definitions with
> POSIX2HOST and add new missing method
> (clearBackground). It also provides
> a dummy PPCRobotHelper and related classes and
> sources to get the Personal
> Profile compilation working.
>
> * The CLDC and MIDP patches currently only add
> configuration support for
> WM2003 (to compile with eVC4).
>
>
> Here are the code diffs (vs. rev. 11040 of the
> phoneME SVN repo):
>
> https://phoneme.dev.java.net/nonav/issues/showattachme
> nt.cgi/10/Alert.rev11040.diff
> https://phoneme.dev.java.net/nonav/issues/showattachme
> nt.cgi/11/AwtPPC.rev11040.diff
> https://phoneme.dev.java.net/nonav/issues/showattachme
> nt.cgi/12/AwtPPCStubs.rev11040.diff
> https://phoneme.dev.java.net/nonav/issues/showattachme
> nt.cgi/13/CLDC.rev11040.diff
> https://phoneme.dev.java.net/nonav/issues/showattachme
> nt.cgi/14/MIDP.rev11040.diff
> https://phoneme.dev.java.net/nonav/issues/showattachme
> nt.cgi/15/Suite.rev11040.diff
>
>
> Davy, please feel free to add any additional comments
> to this thread in the forums.
>
> Everyone else, please take a look at the above diffs
> and send in your comments and/or questions on how
> they look for committing to our repository.
>
>
> This code review will be open until Fri. 5/23/2008.

Last call for code review comments. Please send them in if you have not commented already.

Thanks,

Hinkmond

hinkmond
Offline
Joined: 2003-12-01

Davy Preuveneers sent his updated diffs for his latest code submission for phoneME Advanced. I've updated the IssueTracker Issue #36 with attachments of his latest diffs. Please, review these latest changes:

From Davy P.
-----
Based on the comments I received thus far, I updated the patches against the
latest development build (b72, rev 11420) and included them in attach.

Have changed:

-Alert: Clean up the code by using macros instead of numbers
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/18/Alert.re...

-AwtPPC and AwtPPCStubs: replaced empty clearBackground() method in
PPCComponentPeer.java with one with an implementation. Changed offset of AWT
frame so that the top window bar does not hide part of the frame.
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/19/AwtPPC.r...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/20/AwtPPCSt...

-CLDC: removed redundant VS2005 test so that flag also works with eVC4
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/21/CLDC.rev...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/22/MIDP.rev...

-Suite: Restructured original patch
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/23/Suite.re...

Best regards,
Davy
-----

hinkmond
Offline
Joined: 2003-12-01

Davy sent me an update to his Alert.diff. Please code review this:

-Alert: Clean up the code by using macros instead of numbers
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/24/davyp-20...

For reference, here are the other diffs from Davy that along with the above diff I will be putting on a branch for build and runtime testing:

-AwtPPC and AwtPPCStubs: replaced empty clearBackground() method in
PPCComponentPeer.java with one with an implementation. Changed offset of AWT
frame so that the top window bar does not hide part of the frame.
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/25/davyp-20...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/26/davyp-20...

-CLDC: removed redundant VS2005 test so that flag also works with eVC4
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/27/davyp-20...
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/28/davyp-20...

-Suite: Restructured original patch
https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/29/davyp-20...

Please let me or Davy know if you have any questions or issues with the above planned code diffs for committing to the phoneME repository.

Thanks,
Hinkmond

kaih
Offline
Joined: 2008-06-14

Hinkmond and Davy,

I tried to compile Foundation (works) and Personal (does not work)

On Personal I get this error:
/programs/java/PhoneME/Trunk/cdc/build/win32/rules_personal_pocketpc.mk:29: ***
target pattern contains no `%'. Stop.

However, the lines in that file reads:
$(CVM_OBJDIR)/%.o: $(WCECOMPAT_LIB_SRC_DIR)/%.c
$(AT)echo "...Compiling wcecompat objs: $@"
$(WCECOMPAT_LIB_CC_RULE)

It must be one of the patches that added a POSIX2HOST command to the variables and messed things up with backslashes.

How can I print out a variable value at any time during the scan phase of make?

Thanks,
Kai

I built from cdc/build/win32-x86-ppc03 to run and test stuff on the emulator. But the makefile and defs.mk in win32-arm-ppc03 are virtually identical at least from a makefile scan perspective, so I would rule that out.

When I try the same with an un-patched version is proceeds beyond the scan phase and starts compiling the Java stuff (and throws the expected errors).

Message was edited by: kaih

cjplummer
Offline
Joined: 2006-10-16

> How can I print out a variable value at any time
> during the scan phase of make?
>

I usually use $(warning ...)

$(warning WCECOMPAT_LIB_SRC_DIR = "$(WCECOMPAT_LIB_SRC_DIR)")

Chris

Jiangli Zhou

Hi Davy,

For the changes in MIDPLauncher.java and CdcMIDletSuiteLoader.java,
using reflection to invoke the new CdcMIDletSuiteLoader.setProperties()
is not really necessary. Since the CdcMIDletSuiteLoader class is only
used for MIDP/CDC stack, it is safe to access sun.misc classes from
there. I think you can add a new method (something like
readSuiteProperties()) in sun.misc.MIDPConfig to read the the manifest
attributes. That way you don't need to duplicate the while loop in two
places. You can add another method in MIDPConfig (such as
getSuiteProperties(), which returns the properties gathered from the
manifest. You can call that method from CdcMIDletSuiteLoader to obtain
the properties. With those, you don't need the new setProperties()
method and the 'properties' filed in CdcMIDletSuiteLoader.

Regards,

Jiangli

Hinkmond Wong wrote:
> This is a call for external code review of a new code submission from
> Davy Preuveneers (4/24/2008).
>
> His changes include changes to MIDP, Personal Basis/Personal Profile,
> and CLDC:
>
> * The Alert patch has been extended a bit with a new method to play
> custom sounds (path is harded coded though), but the current behavior
> is to still play the default system sounds. This should provide some
> audio feedback if JSR 135 is not there.
>
> * The AwtPPC and AwtPPCStub patches convert some path definitions with
> POSIX2HOST and add new missing method (clearBackground). It also provides
> a dummy PPCRobotHelper and related classes and sources to get the
> Personal Profile compilation working.
>
> * The CLDC and MIDP patches currently only add configuration support
> for WM2003 (to compile with eVC4).
>
>
> Here are the code diffs (vs. rev. 11040 of the phoneME SVN repo):
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/10/Alert.re...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.r...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCSt...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/15/Suite.re...
>
>
>
> Davy, please feel free to add any additional comments to this thread
> in the forums.
>
> Everyone else, please take a look at the above diffs and send in your
> comments and/or questions on how they look for committing to our
> repository.
>
>
> This code review will be open until Fri. 5/23/2008.
>
>
> Thanks,
>
> Hinkmond
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: advanced-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Davy Preuveneers

On Friday 25 April 2008 18:55:28 Jiangli Zhou wrote:
> Hi Davy,
>
> For the changes in MIDPLauncher.java and CdcMIDletSuiteLoader.java,
> using reflection to invoke the new CdcMIDletSuiteLoader.setProperties()
> is not really necessary. Since the CdcMIDletSuiteLoader class is only
> used for MIDP/CDC stack, it is safe to access sun.misc classes from
> there. I think you can add a new method (something like
> readSuiteProperties()) in sun.misc.MIDPConfig to read the the manifest
> attributes. That way you don't need to duplicate the while loop in two
> places. You can add another method in MIDPConfig (such as
> getSuiteProperties(), which returns the properties gathered from the
> manifest. You can call that method from CdcMIDletSuiteLoader to obtain
> the properties. With those, you don't need the new setProperties()
> method and the 'properties' filed in CdcMIDletSuiteLoader.
>
> Regards,
>
> Jiangli

Hi Jiangli,

In the attached patch, I have moved some code to MIDPConfig and avoid the
reflection to invoke CdcMIDletSuiteLoader.setProperties(). I have not found a
way to eliminate the second while loop in CdcMIDletSuiteLoader, because
sun.misc.MIDPConfig cannot return a com.sun.midp.util.Properties structure.
Importing that package in MIDPConfig results in a compilation error. I
therefore return a HashMap and convert that datastructure to Properties in
the CdcMIDletSuiteLoader.

Is the new patch more or less what you had in mind?

Regards,
Davy

--
Davy Preuveneers
K.U.Leuven - http://www.kuleuven.be
Department of Computer Science - http://www.cs.kuleuven.be
DistriNet - http://www.cs.kuleuven.be/~distrinet
Celestijnenlaan 200A, B-3001 Heverlee (Leuven), Belgium
Room: 02.152
Phone: (+32) (0)16 327853
E-Mail: Davy.Preuveneers@cs.kuleuven.be
Web: http://www.cs.kuleuven.be/~davy
[Suite.rev11161.diff]
[signature.asc]

Jiangli Zhou

Hi Davy,

Sorry for the delay. Your new diff looks okay, except there are some
lines exceeding 80 characters. Please make sure all lines do not exceed
the 80-character limit.

Thanks,
Jiangli

Davy Preuveneers wrote:
> On Friday 25 April 2008 18:55:28 Jiangli Zhou wrote:
>
>> Hi Davy,
>>
>> For the changes in MIDPLauncher.java and CdcMIDletSuiteLoader.java,
>> using reflection to invoke the new CdcMIDletSuiteLoader.setProperties()
>> is not really necessary. Since the CdcMIDletSuiteLoader class is only
>> used for MIDP/CDC stack, it is safe to access sun.misc classes from
>> there. I think you can add a new method (something like
>> readSuiteProperties()) in sun.misc.MIDPConfig to read the the manifest
>> attributes. That way you don't need to duplicate the while loop in two
>> places. You can add another method in MIDPConfig (such as
>> getSuiteProperties(), which returns the properties gathered from the
>> manifest. You can call that method from CdcMIDletSuiteLoader to obtain
>> the properties. With those, you don't need the new setProperties()
>> method and the 'properties' filed in CdcMIDletSuiteLoader.
>>
>> Regards,
>>
>> Jiangli
>>
>
> Hi Jiangli,
>
> In the attached patch, I have moved some code to MIDPConfig and avoid the
> reflection to invoke CdcMIDletSuiteLoader.setProperties(). I have not found a
> way to eliminate the second while loop in CdcMIDletSuiteLoader, because
> sun.misc.MIDPConfig cannot return a com.sun.midp.util.Properties structure.
> Importing that package in MIDPConfig results in a compilation error. I
> therefore return a HashMap and convert that datastructure to Properties in
> the CdcMIDletSuiteLoader.
>
> Is the new patch more or less what you had in mind?
>
> Regards,
> Davy
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Danila Sinopalnikov

Hi Davy,

regarding the second chunk of CLDC patch:

@@ -851,7 +855,10 @@
CPP_DEF_FLAGS_release =
CPP_DEF_FLAGS_product = -DPRODUCT

-ifeq ($(USE_VS2005), true)
+ifeq ($(compiler), visCPP)
+CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE
+endif
+ifeq ($(compiler), evc)
CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE
endif

This code is in visCPP compiler section, under ifeq ($(compiler),
visCPP), so it seems the first condition will always be true and the
second will be false.

Danila

Hinkmond Wong wrote:
> This is a call for external code review of a new code submission from
> Davy Preuveneers (4/24/2008).
>
> His changes include changes to MIDP, Personal Basis/Personal Profile,
> and CLDC:
>
> * The Alert patch has been extended a bit with a new method to play
> custom sounds (path is harded coded though), but the current behavior
> is to still play the default system sounds. This should provide some
> audio feedback if JSR 135 is not there.
>
> * The AwtPPC and AwtPPCStub patches convert some path definitions with
> POSIX2HOST and add new missing method (clearBackground). It also provides
> a dummy PPCRobotHelper and related classes and sources to get the
> Personal Profile compilation working.
>
> * The CLDC and MIDP patches currently only add configuration support
> for WM2003 (to compile with eVC4).
>
>
> Here are the code diffs (vs. rev. 11040 of the phoneME SVN repo):
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/10/Alert.re...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.r...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCSt...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev...
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/15/Suite.re...
>
>
>
> Davy, please feel free to add any additional comments to this thread
> in the forums.
>
> Everyone else, please take a look at the above diffs and send in your
> comments and/or questions on how they look for committing to our
> repository.
>
>
> This code review will be open until Fri. 5/23/2008.
>
>
> Thanks,
>
> Hinkmond
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: feature-unsubscribe@phoneme.dev.java.net
> For additional commands, e-mail: feature-help@phoneme.dev.java.net
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net

Davy Preuveneers

Hi Danila,

Indeed you are quite right, I made this modification a while ago to avoid the
warnings when compiling with eVC4 to target WM2003. I did not notice the top
visCPP test and assumed my patch worked because I had USE_VS2005 set to false
but still needed the -D_CRT_SECURE_NO_DEPRECATE flag.

I think I was also a bit confused, because the documentation for the compiler
section says "Visual Studio 6++". I found it rather weird to have a
test "ifeq ($(USE_VS2005), true)" test in that section. Even below, you have
another compiler section "Embedded Visual C++ 3.0" with another USE_VS2005
test.

Is there a reason why the -D_CRT_SECURE_NO_DEPRECATE flag can only be set when
USE_VS2005 is true in the visCPP section? If there is such a case, would it
make sense to also add and use another variable (say USE_EVC4) instead? If
not, perhaps the "ifeq ($(USE_VS2005), true)" test can be removed?

Best regards,
Davy

On Friday 25 April 2008 10:41:46 Danila Sinopalnikov wrote:
> Hi Davy,
>
> regarding the second chunk of CLDC patch:
>
> @@ -851,7 +855,10 @@
> CPP_DEF_FLAGS_release =
> CPP_DEF_FLAGS_product = -DPRODUCT
>
> -ifeq ($(USE_VS2005), true)
> +ifeq ($(compiler), visCPP)
> +CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE
> +endif
> +ifeq ($(compiler), evc)
> CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE
> endif
>
>
> This code is in visCPP compiler section, under ifeq ($(compiler),
> visCPP), so it seems the first condition will always be true and the
> second will be false.
>
> Danila
>
> Hinkmond Wong wrote:
> > This is a call for external code review of a new code submission from
> > Davy Preuveneers (4/24/2008).
> >
> > His changes include changes to MIDP, Personal Basis/Personal Profile,
> > and CLDC:
> >
> > * The Alert patch has been extended a bit with a new method to play
> > custom sounds (path is harded coded though), but the current behavior
> > is to still play the default system sounds. This should provide some
> > audio feedback if JSR 135 is not there.
> >
> > * The AwtPPC and AwtPPCStub patches convert some path definitions with
> > POSIX2HOST and add new missing method (clearBackground). It also provides
> > a dummy PPCRobotHelper and related classes and sources to get the
> > Personal Profile compilation working.
> >
> > * The CLDC and MIDP patches currently only add configuration support
> > for WM2003 (to compile with eVC4).
> >
> >
> > Here are the code diffs (vs. rev. 11040 of the phoneME SVN repo):
> >
> > https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/10/Alert.rev
> >11040.diff
> >
> > https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.re
> >v11040.diff
> >
> > https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCStu
> >bs.rev11040.diff
> >
> > https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev1
> >1040.diff
> >
> > https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev1
> >1040.diff
> >
> > https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/15/Suite.rev
> >11040.diff
> >
> >
> >
> > Davy, please feel free to add any additional comments to this thread
> > in the forums.
> >
> > Everyone else, please take a look at the above diffs and send in your
> > comments and/or questions on how they look for committing to our
> > repository.
> >
> >
> > This code review will be open until Fri. 5/23/2008.
> >
> >
> > Thanks,
> >
> > Hinkmond
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: feature-unsubscribe@phoneme.dev.java.net
> > For additional commands, e-mail: feature-help@phoneme.dev.java.net

--
Davy Preuveneers
K.U.Leuven - http://www.kuleuven.be
Department of Computer Science - http://www.cs.kuleuven.be
DistriNet - http://www.cs.kuleuven.be/~distrinet
Celestijnenlaan 200A, B-3001 Heverlee (Leuven), Belgium
Room: 02.152
Phone: (+32) (0)16 327853
E-Mail: Davy.Preuveneers@cs.kuleuven.be
Web: http://www.cs.kuleuven.be/~davy
[signature.asc]

Danila Sinopalnikov

Hi Davy,

I think it is okay to just remove "ifeq ($(USE_VS2005), true)" test at
that place.

Thanks,
Danila

Davy Preuveneers wrote:
> Hi Danila,
>
> Indeed you are quite right, I made this modification a while ago to avoid the
> warnings when compiling with eVC4 to target WM2003. I did not notice the top
> visCPP test and assumed my patch worked because I had USE_VS2005 set to false
> but still needed the -D_CRT_SECURE_NO_DEPRECATE flag.
>
> I think I was also a bit confused, because the documentation for the compiler
> section says "Visual Studio 6++". I found it rather weird to have a
> test "ifeq ($(USE_VS2005), true)" test in that section. Even below, you have
> another compiler section "Embedded Visual C++ 3.0" with another USE_VS2005
> test.
>
> Is there a reason why the -D_CRT_SECURE_NO_DEPRECATE flag can only be set when
> USE_VS2005 is true in the visCPP section? If there is such a case, would it
> make sense to also add and use another variable (say USE_EVC4) instead? If
> not, perhaps the "ifeq ($(USE_VS2005), true)" test can be removed?
>
> Best regards,
> Davy
>
>
> On Friday 25 April 2008 10:41:46 Danila Sinopalnikov wrote:
>
>> Hi Davy,
>>
>> regarding the second chunk of CLDC patch:
>>
>> @@ -851,7 +855,10 @@
>> CPP_DEF_FLAGS_release =
>> CPP_DEF_FLAGS_product = -DPRODUCT
>>
>> -ifeq ($(USE_VS2005), true)
>> +ifeq ($(compiler), visCPP)
>> +CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE
>> +endif
>> +ifeq ($(compiler), evc)
>> CPP_DEF_FLAGS += -D_CRT_SECURE_NO_DEPRECATE
>> endif
>>
>>
>> This code is in visCPP compiler section, under ifeq ($(compiler),
>> visCPP), so it seems the first condition will always be true and the
>> second will be false.
>>
>> Danila
>>
>> Hinkmond Wong wrote:
>>
>>> This is a call for external code review of a new code submission from
>>> Davy Preuveneers (4/24/2008).
>>>
>>> His changes include changes to MIDP, Personal Basis/Personal Profile,
>>> and CLDC:
>>>
>>> * The Alert patch has been extended a bit with a new method to play
>>> custom sounds (path is harded coded though), but the current behavior
>>> is to still play the default system sounds. This should provide some
>>> audio feedback if JSR 135 is not there.
>>>
>>> * The AwtPPC and AwtPPCStub patches convert some path definitions with
>>> POSIX2HOST and add new missing method (clearBackground). It also provides
>>> a dummy PPCRobotHelper and related classes and sources to get the
>>> Personal Profile compilation working.
>>>
>>> * The CLDC and MIDP patches currently only add configuration support
>>> for WM2003 (to compile with eVC4).
>>>
>>>
>>> Here are the code diffs (vs. rev. 11040 of the phoneME SVN repo):
>>>
>>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/10/Alert.rev
>>> 11040.diff
>>>
>>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.re
>>> v11040.diff
>>>
>>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCStu
>>> bs.rev11040.diff
>>>
>>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev1
>>> 1040.diff
>>>
>>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev1
>>> 1040.diff
>>>
>>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/15/Suite.rev
>>> 11040.diff
>>>
>>>
>>>
>>> Davy, please feel free to add any additional comments to this thread
>>> in the forums.
>>>
>>> Everyone else, please take a look at the above diffs and send in your
>>> comments and/or questions on how they look for committing to our
>>> repository.
>>>
>>>
>>> This code review will be open until Fri. 5/23/2008.
>>>
>>>
>>> Thanks,
>>>
>>> Hinkmond
>>>
>>>
>>>
>>> ---------------------------------------------------------------------
>>> To unsubscribe, e-mail: feature-unsubscribe@phoneme.dev.java.net
>>> For additional commands, e-mail: feature-help@phoneme.dev.java.net
>>>
>
>
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: advanced-unsubscribe@phoneme.dev.java.net
For additional commands, e-mail: advanced-help@phoneme.dev.java.net