Re: [PATCH 4/8] stk: Add SimApplication interface.

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1289 bytes --]

Hi Andrew,

> ---
>  include/dbus.h |    1 +
>  src/stk.c      |  585
>  +++++++++++++++++++++++++++++++++++++++++++++++++++++++- 2 files 
changed,
>  585 insertions(+), 1 deletions(-)
> 

Just a general comment: In the previous version of the patch you included an 
api document in the commit description.  It might be a good idea to formalize 
it into a proper doc/stk-api.txt document and include it along with the patch.  
This way people have a much easier time understanding what is going on, and we 
do want feedback on the API from many people for this one.

Another suggestion I have is to split out the Select Item proactive command 
handling from the regular Menu.  Perhaps moving Select Item handling to an 
Agent interface along with Display Text, Play Tone, Get Input, Get Inkey, etc 
might be a good idea.

Also, I really encourage you to split this patch apart some more.  E.g. one 
patch for foundation of the stk atom, one part handling the Setup Menu / Menu 
Selection envelope, another for Select Item, etc.  This makes it easier to 
review the code.  Smaller patches are always good, and higher chance that less 
controversial chunks will get accepted, leading to even smaller patches in the 
future.

Regards,
-Denis

Regards,
-Denis