Rockbox

Looks nice. A few comments:

1) I would like a better way to edit the settings inline, especially when the line gets so long that it has to scroll.

2) I think it would be nicer to have two lines per setting, the first line with the left-adjusted title, and the second line with the right-adjusted value.

oops, small bug spotted, fixed

   setting_display.patch (67.2 KiB)

I like this idea, I will test it on my h300 later tonight and add any comment/ suggestions later

patch update, just some code cleanup

Linus:
1) So would I, but can’t really think of a better way to do it… :) Other than perhaps that it would be nice if the line started scrolling in its rightmost position when in edit mode, but I’m unsure how to do this. (I’m a little wary of messing with the scrolling code, it’s a bit daunting the way it seems to have multiple implementations in separate lcd-*.c files)
2) Might be a good idea, but a problem with that is I believe the way lists work right now yo’u cant have differing amounts of lines for different menu options.

   setting_display.patch (66.3 KiB)

Added option to enable/disable right-aligning of setting values.

   setting_display.patch (66.9 KiB)

Added multiline support.

   setting_display.patch (70.4 KiB)

Fixed bug that caused values to always be displayed right-aligned in multiline mode.

   setting_display.patch (70.6 KiB)

- when changing values inline “up” and “down” seem to be inverted on my h120.
- how about the following idea for inline editing:

o on the highlighted entry "select" or "right" enters edit mode
o when in edit mode, "right" also changes the values. "select" leaves the edit mode, "stop" cancels, possibly also use "left" for changing. Not sure if this is a good idea though ...
(of course this will only work on target with enough buttons)

Apart from that I think this makes the settings menus less confusing.

added 1px margin to right-aligned text for now, we’ll see if I leave it in or not

   setting_display.patch (70.8 KiB)

bluebrother:
The up and down do seem inverted don’t they? At least on integer selection settings. However I’m not really sure it’s a good idea to have separate button handling depending on whether selection is done inline or on a separate page. This doesn’t really feel like an issue specific to this patch, perhaps the order of integers settings should reversed for both inline and list selection…?

after your comment I realized it, it’s mostly the integer values. As the values are sorted ascending when modifying them outside of the menu (which seems “natural” to me) pressing “down” increases the value. But when editing them in the menu I somewhat expect them to increase with “up”, similar as the volume increases when pressing “up” in the wps.
I’m not sure how to handle this best but completely reversing the order in both view will be strange the other way round.

Latest update:
* Removed the 1px right margin for now, probably won’t add it again until I’ve figured out a better way to do this.
* Added a Boolean Quick Toggle option: when enabled it’s enough to hit select or right in the menu to toggle a boolean setting.
* Converted all of General Settings (well almost all, anything still left as ordinary menus was done so intentionally)

TODO now: Lots of testing on different targets…

   setting_display.patch (136.8 KiB)

bluebrother: forgot to mention, in the latest update up/down are switched in inline integer selection.

killed one small bug

   setting_display.patch (136.7 KiB)

Converted the rest of the main menu, plus the playlist viewer context menu.
Menu conversion is now complete.

   setting_display.patch (196.1 KiB)

Ooops, wouldn’t compile on non-bitmap-LCD targets. Fixed.

   setting_display.patch (196.1 KiB)

Argh, forgot to include select.[ch] in last upload… :(

   setting_display.patch (209.9 KiB)

* Differentiate between menu entries that open submenus and those that perform actions in multiline mode.
* Added code and macros to allow for display of custom (descriptive?) strings on the second line in multiline mode. Nothing I’d want or plan to do myself, just making things easier for anyone who may want to do this in the future - after this has been included in CVS (*crosses fingers*).

   setting_display.patch (211.8 KiB)

Apart from a few issues this gets really great!

I just applied the latest patch and found some menu entries missing (target is h120):
- General Settings → System → Idle Poweroff
- General Settings → System → Sleep Timer
- General Settings → System → Car Adapter Mode
- General Settings → Display → Default Codepage
Also, General Settings and Display doesn’t use the multiline view in the top level – to have it consistent that menus (even if they only contain subentries) should use that multiline view with the extension mark “>” too IMO.

Also, extending this view (I really like the multiline view) up to the main menu level would be nice I guess because this would make it more consistent.

Added the missing settings entries.
However, I have still not changed any menus that do not directly contain any settings - that’s not what this patch was intended for (if it was my intent to replace all menus I’d probably have structured my code in a different manner).

   setting_display.patch (218.4 KiB)

Should it work also for a x5 build?
I tried it with the simulator and it works
but not on my x5 player

Max: I haven’t managed build the latest CVS for X5 (except, like you said for sim) regardless of if I use the patched source. Does yours build properly with the unpatch source?

I just updated to CVS HEAD and it builds for x5 for me

I need to change your patch a little bit to make it compile even for the
simulator.

The problem is the following:
… CC codeclib.c
AR+RANLIB /home/maxl/workspace/rockbox/build-uisimulator/libcodec.a
make[2]: *** No rule to make target `button-target.h’, needed by `/home/maxl/workspace/rockbox/build-uisimulator/apps/codecs/vorbis.o’. Stop.

I think the problem is your #include “select.h” in settings.h
this ends up in including button-target.h but bulding the codecs
does not include the folder where button-target.h is located.

So the dependencies are unresolved

When changing the includes I can make it compile

Ok, got make to complete for X5 with this. Wanna give it a try?

   setting_display.patch (218 KiB)

Yes now it compiles “out-of-the-box”

Another general question:
Whatever I change in “Settings Display” the “result” is always the same
means: I can edit the values for a single setting with the keys without a separate page

What is the difference of the four values of “Display Values In Menus”?
Also the meaning of “Show Edit Indicator” and “Change Settings In Menu” is unclear to me
and I dont see any difference changing the values for any of them

This is for the simulator

Tried it on the “real target” but still no success

I can see all the new settings under “Display” and can change
the values like in the simulator.

But the menu behaviour is not changing and still the
old one (using a separate page)

Indeed it seems in the efforts to get it to compile for the X5 I broke it, none of the settings work properly anymore. :( Sadly I can’t see why (no longer works in ANY sim btw). I’m really too tired for this now, will have a look with fresh eyes tomorrow.

Weird. I really don’t get this. It appears that all the functionality is still there, I just can’t access global_settings from menu_select.c. I replaced all the settings by macros, setting everything ON (so now you should at least be able to see what that is like). You can change these values (at the top of menu_select.c) and see where they should be read from (commented out on the line below each macro).

   setting_display.patch (218.4 KiB)

I noticed such a strange behaviour when trying to get the
scrobbler patch working. There seems to be some problem
of memory corruption.

When adding a new variable at the end of global_settings
it will get corrupt at some point. Adding the variable
“higher” in the struct and everything works.

When debugging I noticed that some variables of the
struct got overwritten and changed e.g. from true to false

Ok, like you said, adding the variables higher up in the struct. Weird, anyhow everything seems to work now, plus it builds properly for X5.

   setting_display.patch (219.3 KiB)

Thanks! I will try it asap
But this kind problems really makes me nervous If it is really some problem of memory corruption
we may end in big trouble sooner or later

The thing that strikes me as odd about the memory corruption theory is that the settings were returning the proper values when accessed from settings_menu.c; only when accessed from my own new file (menu_select.c) did they appear to have the incorrect values. Also, since I moved my settings higher up in the struct there appear to be no problems with the variables that currently are at the end of the struct.

Yes the values get wrong only when you enter a new context e.g. call a function

BTW: I noticed that the location of the new file in the SOURCES file
makes also a difference sometime

You may try that too

Minor update: Removed added trailing space that was occasionally causing unneccessary scrolling in multiline mode.
It seems the earlier fix for the X5 also fixed the Toshiba GigaBeat sim build as well. I now believe this patch no longer breaks anything.

   setting_display.patch (219.4 KiB)

Yes now it works also on the real target And now I also see the differences between the options
that I asked before

I tried your patch before the x5 support. The sim worked well, but on the player it was freezing with an error code that looked like a misplaced pointer. It said PTR and some hex digits. I don’t know if this really was a problem with your patch, because I had the quick menu exit patch applied at the same time and I possibly broke my whole source when I tried things here and there. I will start from scratch now and test one patch at the time.

In theory, adding settings to the end of the settings struct should be the right way to go, otherwise you have to increase the settings version counter. If you apply an updated patch and run the new rockbox.iriver when there was a patched binary before, it is always a good idea to clear the settings at boot (holding rec after bootloader). But if you increase the version counter in your patch there can be serious problems if it gets increased by a cvs update, because then rockbox won’t recognize it (would be the same number) and possibly uses values in wrong places. CVS should show a conflict in this case, though.

Wow i am suprised thatthe two patches are compatable at all, i had decided to wate till this patch was commited before updating the quick exit patch to work together. (or just give up any hopes of it geting into cvs altogether)

apparently they are not… They don’t give conflict directly, I suppose it should be possible to figure out where the problem is. If I have my own patch ready again I try this a second time, because I like both patches very much.

Unfortunatly for us, Jonathan Gordon is going to do a complete rework of the menu code (see the mailing list), this will break all patches again in a few weeks.

Just FYI:
I am also creating builds for the people from the iaudiophile.net forums
So there will be a lot of testers for this patch

Just FYI:
I am also creating builds for the people from the iaudiophile.net forums
including this patch together with some others (e.g. album_art …)

So there will be a lot of testers for this patch

oops sorry for the double post

maxwen: nice

Anyhow, I won’t be able to work on this for a couple of days (off to Dublin over the weekend). Eager to check up on testing evaluations when I get back.

Hey, works great on the Iaudio X5!
One footnote: Don’t know if this could be fixed in your patch or even if the problem lies there; when changing the volume in the menu you can blast you ears out when you reach the bottom and click trough going right to maximal output!

Anyway great patch!

Andreas: I have feedback from 6 different people testing this patch
during the last days on x5

short answer: no problems so far!

your patch is not working with current cvs. Gives me conflicts in tree.c when I want to apply it.