Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#10283 - Simplify Xobox menu and fix a issue

Attached to Project: Rockbox
Opened by Teruaki Kawashima (teru) - Saturday, 06 June 2009, 06:45 GMT+2
Last edited by Teruaki Kawashima (teru) - Sunday, 01 November 2009, 15:45 GMT+2
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

*Use same menu for both in-game and not in game.
-"Restart Level" and "Start Game" are combined as "Start New Game".
"Restart Level" sounds like to restart the level I stoped for me. but it starts game from level 1.
-Show message "No game to resume" if "Resume Game" is selecetd while not in game. this is similar to chopper.
*fix typo "Difficult" to "Difficulty".
*Fix the following issue.
pause game and enter menu. then resume game. the menu will stay shown untill game is unpaused. it is confusable.
With this patch, redraw the board and show "Paused".
another way to fix it would be to force unpausing. i.e. somthing like "pause = false;".
or do not allow to enter menu while the game is pausing.
I'm not sure which is the best.
   xobox.patch (4.5 KiB) 
 apps/plugins/xobox.c |  109 +++++++++++++++++++--------------------------------
 1 file changed, 41 insertions(+), 68 deletions(-)

This task depends upon

Closed by  Teruaki Kawashima (teru)
Sunday, 01 November 2009, 15:45 GMT+2
Reason for closing:  Out of Date
Comment by Johannes Schwarz (Ubuntuxer) - Saturday, 06 June 2009, 16:57 GMT+2
Hi, I doesn't like the idea to use the same menu for both in-game and not in game, because resume game shouldn't be displayed, if you can't select it. In my opinion the different menus increase the usability.
I think it would be make sense, that you can't enter the menu while the game is paused.
I wrote a patch, which fixes the typos and illogical entrys in the menu.
   xobox.diff (2.8 KiB) 
 apps/plugins/xobox.c |   26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

Comment by Teruaki Kawashima (teru) - Sunday, 07 June 2009, 18:19 GMT+2
it's not good things to have duplicated settings and so on.
and i dont like something like
>if(ingame)
> MENUITEM_STRINGLIST(...)
>else
> MENUITEM_STRINGLIST(...)
another approach is using MAKE_MENU, MENUITEM_RETURNVALUE, and menu_callback function.
   xobox.2.patch (5.8 KiB) 
 apps/plugins/xobox.c |  130 +++++++++++++++++++++------------------------------
 1 file changed, 54 insertions(+), 76 deletions(-)

Comment by Johannes Schwarz (Ubuntuxer) - Monday, 08 June 2009, 17:59 GMT+2
I think the usage of MENUITEM_RETURNVALUE and a menu_callback function is unnecessary complicated, because it's just code style. But your patch works fine for me.
Comment by Teruaki Kawashima (teru) - Wednesday, 10 June 2009, 16:15 GMT+2
hmm... then, how about this one? use MENUITEM_STRINGLIST with a menu_callback.
   xobox.3.patch (5.8 KiB) 
 apps/menu.c          |    3 -
 apps/plugins/xobox.c |  122 +++++++++++++++++++--------------------------------
 2 files changed, 48 insertions(+), 77 deletions(-)

Comment by Johannes Schwarz (Ubuntuxer) - Wednesday, 10 June 2009, 19:46 GMT+2
It's simply great; a very neat solution. Are you sure the change in menu.c doesn't affect any other plugin?
Comment by Teruaki Kawashima (teru) - Friday, 12 June 2009, 16:26 GMT+2
I made a similar change for menus of some other plugins.

At least, the change in apps/menu.c won't affect existing code.
It only affects MT_RETURN_ID with menu_callback function. because if menu_callback is not set, current_subitems[n] is n.
And as far as i checked, MT_RETURN_ID is only used by MENUITEM_STRINGLIST and none of MENUITEM_STRINGLIST is used with callback.
I think it's almost ok for MENUITEM_STRINGLIST with menu_callback but I'm not sure.
   simplify_menus_by_using_stringlist_with_callback.... (13.6 KiB) 
 apps/plugins/mazezam.c  |   52 +++++++-------------
 apps/plugins/xobox.c    |  122 ++++++++++++++++++------------------------------
 apps/plugins/keybox.c   |   26 ++--------
 apps/plugins/calendar.c |   27 ++--------
 apps/plugins/chopper.c  |   12 ++++
 apps/menu.c             |    3 -
 6 files changed, 90 insertions(+), 152 deletions(-)

Comment by Johannes Schwarz (Ubuntuxer) - Sunday, 14 June 2009, 19:53 GMT+2
I wrote a patch, which fix the warnings and adapt the behaviour of chooper to xobox.
   simplify_menus_by_using_stringlist_with_callback2... (14.5 KiB) 
 apps/plugins/mazezam.c  |   52 +++++++-------------
 apps/plugins/xobox.c    |  122 ++++++++++++++++++------------------------------
 apps/plugins/keybox.c   |   26 ++--------
 apps/plugins/calendar.c |   26 ++--------
 apps/plugins/chopper.c  |   29 ++++++-----
 apps/menu.c             |    3 -
 6 files changed, 95 insertions(+), 163 deletions(-)

Comment by Teruaki Kawashima (teru) - Monday, 15 June 2009, 05:13 GMT+2
Please dont change the behavior of calendar. "Edit" shouldn't be displayed if the day dose't have memos.
Could you write the warnings you got if you remember? i haven't got any warning while compiling.
Comment by Johannes Schwarz (Ubuntuxer) - Monday, 15 June 2009, 07:55 GMT+2
warnings:
/home/johannes/Rockbox/rockbox/apps/plugins/chopper.c: In function ‘chopMenuCb’:
/home/johannes/Rockbox/rockbox/apps/plugins/chopper.c:707: warning: comparison between pointer and integer"
/home/johannes/Rockbox/rockbox/apps/plugins/calendar.c: In function ‘edit_menu_cb’:
/home/johannes/Rockbox/rockbox/apps/plugins/calendar.c:630: warning: initialization makes integer from pointer without a cast

Comment by Teruaki Kawashima (teru) - Monday, 15 June 2009, 14:52 GMT+2
small fix for mazezam.
   simplify_menus_by_using_stringlist_with_callback.... (14.6 KiB) 
 apps/plugins/mazezam.c  |   52 +++++++-------------
 apps/plugins/xobox.c    |  122 ++++++++++++++++++------------------------------
 apps/plugins/keybox.c   |   26 ++--------
 apps/plugins/calendar.c |   27 ++--------
 apps/plugins/chopper.c  |   28 ++++++-----
 apps/menu.c             |    3 -
 6 files changed, 96 insertions(+), 162 deletions(-)

Comment by Teruaki Kawashima (teru) - Thursday, 25 June 2009, 16:38 GMT+2
Update patch.
*apply same chage to solitaire.
*divide fix for xobox and simplifying menu.
   fix_xobox.patch (1.8 KiB) 
 apps/plugins/xobox.c |   19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

   simplify_menus_by_using_stringlist_with_callback_... (9.8 KiB) 
 apps/menu.c            |    3 -
 apps/plugins/mazezam.c |   52 ++++++++----------------
 apps/plugins/xobox.c   |  105 +++++++++++++++++--------------------------------
 apps/plugins/chopper.c |   30 ++++++++------
 4 files changed, 76 insertions(+), 114 deletions(-)

   simplify_menus_by_using_stringlist_with_callback_... (3.8 KiB) 
 apps/plugins/keybox.c   |   26 ++++++--------------------
 apps/plugins/calendar.c |   27 +++++++--------------------
 2 files changed, 13 insertions(+), 40 deletions(-)

Comment by Teruaki Kawashima (teru) - Thursday, 25 June 2009, 16:44 GMT+2
Oops, diff of solitaire was not included..
apply the attached patch with the 2nd patch of previous comment.
   simplify_menus_for_solitaire.5.patch (3 KiB) 
 apps/plugins/solitaire.c |   57 +++++++++++++++++++++++++----------------------
 1 file changed, 31 insertions(+), 26 deletions(-)

Comment by Rafaël Carré (funman) - Monday, 29 June 2009, 00:15 GMT+2
I notice something weird:

For example in simplify_menus_by_using_stringlist_with_callback_b.5.patch :

- if (action == ACTION_REQUEST_MENUITEM && memos_in_shown_memory <= 0)
+ int i = (intptr_t)this_item;
+ if (action == ACTION_REQUEST_MENUITEM
+ && memos_in_shown_memory <= 0 && (i==0 || i==1))

You cast this_item (which is a pointer) to an integer (intptr_t) and compare it to 0 or 1.

What are you trying to do exactly ?

The commit r21523 contains other uses of intptr_t and I think they all are illegal here.

If you want to check if a pointer is not null, you should do if(pointer != NULL) or even if(!pointer) but not cast it to intptr_t.
Comment by Teruaki Kawashima (teru) - Monday, 29 June 2009, 16:51 GMT+2
I tried to compare this_item as an interger.
In this context, i, 0, and 1 are indexes of the list of text.
I don't know the reason but index is passed as a pointer casted like "(void*)(intptr_t)i" in apps/menu.c at line around 192 if the menu is from MENUITEM_STRINGLIST.
I thought that maybe I can use this value to simplify menu and I need to cast this_item to an integer to compare.
Comment by Thomas Martitz (kugel.) - Monday, 29 June 2009, 17:48 GMT+2
I think this is a special case.

The names you pass to MENUITEM_STRINGLIST don't have an address anyway as they're literal constants (so it can't actually be a pointer). Hence their ID is used. In this case, casting to int_ptr seems weird but correct.

Loading...