Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Plugins
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by teru - 2009-06-06
Last edited by teru - 2009-11-01

FS#10283 - Simplify Xobox menu and fix a issue

*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.

Closed by  teru
2009-11-01 14:45
Reason for closing:  Out of Date

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.

teru commented on 2009-06-07 16:19

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.

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.

teru commented on 2009-06-10 14:15

hmm… then, how about this one? use MENUITEM_STRINGLIST with a menu_callback.

It's simply great; a very neat solution. Are you sure the change in menu.c doesn't affect any other plugin?

teru commented on 2009-06-12 14:26

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.

I wrote a patch, which fix the warnings and adapt the behaviour of chooper to xobox.

teru commented on 2009-06-15 03:13

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.

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

teru commented on 2009-06-15 12:52

small fix for mazezam.

teru commented on 2009-06-25 14:38

Update patch.
*apply same chage to solitaire.
*divide fix for xobox and simplifying menu.

teru commented on 2009-06-25 14:44

Oops, diff of solitaire was not included..
apply the attached patch with the 2nd patch of previous comment.

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.

teru commented on 2009-06-29 14:51

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.

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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing