Rockbox

Tasklist

FS#5833 - Display/Edit Settings Within Menus

Attached to Project: Rockbox
Opened by Andreas Mattsson (AM) - Thursday, 17 August 2006, 16:18 GMT
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 01 August 2007, 14:09 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Allows values for settings to be viewed and modified from directly within the menus (you don't have to select them to display them on a separate page).
Activated via General Settings -> Display -> Settings Display.
So far only the aforementioned menu, plus Playback Settings and its submenus, have been changed to allow for this. I thought I'd await comments/suggestions/testing before I go ahead and convert all settings menus.

Apply patch with -p0
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 01 August 2007, 14:09 GMT
Reason for closing:  Out of Date
Comment by Linus Nielsen Feltzing (linusnielsen) - Thursday, 17 August 2006, 16:29 GMT
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.
Comment by Andreas Mattsson (AM) - Thursday, 17 August 2006, 16:39 GMT
oops, small bug spotted, fixed
Comment by Alistair Marshall (amar) - Thursday, 17 August 2006, 16:41 GMT
I like this idea, I will test it on my h300 later tonight and add any comment/ suggestions later
Comment by Andreas Mattsson (AM) - Thursday, 17 August 2006, 16:55 GMT
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.
Comment by Andreas Mattsson (AM) - Thursday, 17 August 2006, 17:48 GMT
Added option to enable/disable right-aligning of setting values.
Comment by Andreas Mattsson (AM) - Thursday, 17 August 2006, 21:30 GMT
Added multiline support.
Comment by Andreas Mattsson (AM) - Thursday, 17 August 2006, 22:00 GMT
Fixed bug that caused values to always be displayed right-aligned in multiline mode.
Comment by Dominik Riebeling (bluebrother) - Thursday, 17 August 2006, 22:20 GMT
- 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.
Comment by Andreas Mattsson (AM) - Thursday, 17 August 2006, 22:50 GMT
added 1px margin to right-aligned text for now, we'll see if I leave it in or not
Comment by Andreas Mattsson (AM) - Thursday, 17 August 2006, 22:54 GMT
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...?
Comment by Dominik Riebeling (bluebrother) - Friday, 18 August 2006, 05:06 GMT
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.
Comment by Andreas Mattsson (AM) - Friday, 18 August 2006, 23:56 GMT
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...
Comment by Andreas Mattsson (AM) - Saturday, 19 August 2006, 07:47 GMT
bluebrother: forgot to mention, in the latest update up/down are switched in inline integer selection.
Comment by Andreas Mattsson (AM) - Saturday, 19 August 2006, 09:40 GMT
killed one small bug
Comment by Andreas Mattsson (AM) - Saturday, 19 August 2006, 16:25 GMT
Converted the rest of the main menu, plus the playlist viewer context menu.
Menu conversion is now complete.
Comment by Andreas Mattsson (AM) - Saturday, 19 August 2006, 16:49 GMT
Ooops, wouldn't compile on non-bitmap-LCD targets. Fixed.
Comment by Andreas Mattsson (AM) - Sunday, 20 August 2006, 15:05 GMT
Argh, forgot to include select.[ch] in last upload... :(
Comment by Andreas Mattsson (AM) - Sunday, 20 August 2006, 23:27 GMT
* 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*).
Comment by Dominik Riebeling (bluebrother) - Monday, 21 August 2006, 06:53 GMT
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.
Comment by Andreas Mattsson (AM) - Monday, 21 August 2006, 10:32 GMT
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).
Comment by Max Weninger (maxwen) - Monday, 21 August 2006, 18:16 GMT
Should it work also for a x5 build?
I tried it with the simulator and it works
but not on my x5 player
Comment by Andreas Mattsson (AM) - Monday, 21 August 2006, 20:21 GMT
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?
Comment by Max Weninger (maxwen) - Monday, 21 August 2006, 20:56 GMT
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


Comment by Andreas Mattsson (AM) - Monday, 21 August 2006, 21:53 GMT
Ok, got make to complete for X5 with this. Wanna give it a try?
Comment by Max Weninger (maxwen) - Monday, 21 August 2006, 22:04 GMT
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
Comment by Max Weninger (maxwen) - Monday, 21 August 2006, 22:17 GMT
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)
Comment by Andreas Mattsson (AM) - Monday, 21 August 2006, 22:25 GMT
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.
Comment by Andreas Mattsson (AM) - Monday, 21 August 2006, 22:58 GMT
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).
Comment by Max Weninger (maxwen) - Tuesday, 22 August 2006, 10:43 GMT
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 :-(
Comment by Andreas Mattsson (AM) - Tuesday, 22 August 2006, 12:14 GMT
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.
Comment by Max Weninger (maxwen) - Tuesday, 22 August 2006, 12:19 GMT
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
Comment by Andreas Mattsson (AM) - Tuesday, 22 August 2006, 12:59 GMT
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.
Comment by Max Weninger (maxwen) - Tuesday, 22 August 2006, 13:09 GMT
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
Comment by Andreas Mattsson (AM) - Tuesday, 22 August 2006, 14:56 GMT
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.
Comment by Max Weninger (maxwen) - Tuesday, 22 August 2006, 16:06 GMT
Yes now it works also on the real target :-)
And now I also see the differences between the options
that I asked before
Comment by Simon Menzel (Rincewind) - Wednesday, 23 August 2006, 17:32 GMT
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.
Comment by Alistair Marshall (amar) - Wednesday, 23 August 2006, 17:38 GMT
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)
Comment by Simon Menzel (Rincewind) - Wednesday, 23 August 2006, 18:06 GMT
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.
Comment by Max Weninger (maxwen) - Wednesday, 23 August 2006, 19:24 GMT
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 :-)
Comment by Max Weninger (maxwen) - Wednesday, 23 August 2006, 19:25 GMT
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 :-)
Comment by Max Weninger (maxwen) - Wednesday, 23 August 2006, 19:31 GMT
oops sorry for the double post :-(
Comment by Andreas Mattsson (AM) - Wednesday, 23 August 2006, 20:28 GMT
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.
Comment by Jeroen Klomp (madcow) - Friday, 25 August 2006, 10:59 GMT
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!
Comment by Max Weninger (maxwen) - Saturday, 26 August 2006, 11:05 GMT
Andreas: I have feedback from 6 different people testing this patch
during the last days on x5

short answer: no problems so far! :-)
Comment by Simon Menzel (Rincewind) - Saturday, 26 August 2006, 12:37 GMT
your patch is not working with current cvs. Gives me conflicts in tree.c when I want to apply it.

Loading...