|
Replies:
28
-
Last Post:
Aug 22, 2008 10:07 PM
by: hinkmond
|
Threads:
[
Previous
|
Next
]
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
Apr 25, 2008 11:14 AM
in response to: Hinkmond Wong
|
|
|
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.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCStubs.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.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.
>
>
> 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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Apr 25, 2008 11:14 AM
in response to: Danila Sinopaln...
|
|
|
|
|
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]
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
Apr 25, 2008 11:14 AM
in response to: Davy Preuveneers
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
Apr 25, 2008 11:14 AM
in response to: Hinkmond Wong
|
|
|
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.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCStubs.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev11040.diff
>
> https://phoneme.dev.java.net/nonav/issues/showattachment.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.
>
>
> 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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
May 5, 2008 2:49 AM
in response to: Jiangli Zhou
|
|
|
|
|
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]
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
May 12, 2008 11:44 AM
in response to: Davy Preuveneers
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
May 8, 2008 7:42 PM
in response to: Jiangli Zhou
|
|
|
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.rev11040.diff
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/11/AwtPPC.rev11040.diff
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/12/AwtPPCStubs.rev11040.diff
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/13/CLDC.rev11040.diff
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.cgi/14/MIDP.rev11040.diff
>>
>> https://phoneme.dev.java.net/nonav/issues/showattachment.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.
>>
>>
>> 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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
May 13, 2008 1:01 AM
in response to: Stephen Flores
|
|
|
|
|
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]
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
May 13, 2008 9:25 AM
in response to: Davy Preuveneers
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
May 13, 2008 12:06 PM
in response to: Jiangli Zhou
|
|
|
|
|
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
<snip>
> > 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]
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
May 13, 2008 6:48 PM
in response to: Davy Preuveneers
|
|
|
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
>
> <snip>
>
>>> 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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
May 23, 2008 6:17 PM
in response to: Hinkmond Wong
|
|
|
> 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
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 14, 2008 1:13 PM
in response to: hinkmond
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 14, 2008 9:18 PM
in response to: kaih
|
|
|
> 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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
Jun 15, 2008 1:42 AM
in response to: kaih
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 15, 2008 3:16 AM
in response to: kaih
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 16, 2008 12:47 PM
in response to: davyp
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 16, 2008 1:56 PM
in response to: 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.
>
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 16, 2008 3:37 PM
in response to: cjplummer
|
|
|
> > 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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 16, 2008 3:55 PM
in response to: hinkmond
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
Jun 16, 2008 3:57 PM
in response to: kaih
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers
(4/24/2008)
Posted:
Jun 16, 2008 4:38 PM
in response to: Hinkmond Wong
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 16, 2008 4:24 PM
in response to: kaih
|
|
|
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
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 16, 2008 3:57 PM
in response to: hinkmond
|
|
|
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)
#
|
|
|
|
|
|
|
|
|
Re: Code review for new code submission from Davy Preuveneers (4/24/2008)
Posted:
Jun 16, 2008 4:33 PM
in response to: cjplummer
|
|
|
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
|
|
|
|
|
java.net RSS
