Rockbox Technical Forums

Support and General Use => User Interface and Voice => Topic started by: AM on August 17, 2006, 11:23:25 AM

Title: New patch: Settings Display Within Menus. DONE!
Post by: AM on August 17, 2006, 11:23:25 AM
It's nice to see that there's been some extra focus on usability the last couple of days. This patch is my contribution.

It 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.
Now done converting all the menus!

I'd love it if you guys gave it a try and help me with testing/suggestions.

So far only tested on H100 sim, H300 sim and iPod4G sim.and a few more

screenie:
(http://web.telia.com/~u16106552/menusettings.png)

link: http://www.rockbox.org/tracker/task/5833
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: lowlight on August 17, 2006, 11:54:23 AM
Interesting...I would suggest modeling it after the "Show ID3 Info" screen (it's a list were each item is 2 lines). It might look cleaner and scale better to other targets (particularly those with smaller screens).
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 17, 2006, 12:03:37 PM
Interesting...I would suggest modeling it after the "Show ID3 Info" screen (it's a list were each item is 2 lines). It might look cleaner and scale better to other targets (particularly those with smaller screens).
AFAIK with the current list code you cant have different amount of lines for list different entries, which could be a problem since on many of the settings screens there are a combination of actual settings and submenus (would mean empty lines after each submenu?). Also, I'd like to be able to keep the option of displaying the value for the selected item only, which would mean one entry with two lines, the rest just one - again not possible.

Personally I don't feel like redoing all the list code to allow for a differing number of lines for different entries and I really doubt it'd be possible to get it to work well on really small screens or with huge fonts so I think it might be better if those users just had this option switched off, using the current method of editing settings on a separate page.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: LinusN on August 17, 2006, 12:09:20 PM
I wouldn't want different line heights for sub-categories. Instead, I can imagine something like this:
Code: [Select]
Volume
                 -11dB
Sub-setting
       -------------->

or something like that.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 17, 2006, 12:17:22 PM
I wouldn't want different line heights for sub-categories. Instead, I can imagine something like this:
Code: [Select]
Volume
                 -11dB
Sub-setting
       -------------->

or something like that.
Hmm... Shouldn't be all that hard to add that as an option I guess (except perhaps if you want a really fancy graphical arrow). Maybe I'll give it a shot later on. Currently I'm a little fatigued from coding many hours straight so right now I won't touch the code for a while unless I find any small but glaringly obvious bugs.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 17, 2006, 04:44:18 PM
Now added :

If you've tried an earlier version of this patch save your settings before upgrading, then reset and reload.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: bluebrother on August 17, 2006, 05:06:06 PM
I like it. Especially when configured like the last screenshot (that configuration should be default IMO). But: the right-aligned text looks a bit too "right" on the target -- the left-aligned text has a 1 pixel distance (which is clearly to see when highlighted) while the right aligned text doesn't -- giving it a 1px distance would look even better.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: Llorean on August 17, 2006, 05:10:48 PM
I agree with everything bluebrother just said.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 17, 2006, 05:12:59 PM
I like it. Especially when configured like the last screenshot (that configuration should be default IMO). But: the right-aligned text looks a bit too "right" on the target -- the left-aligned text has a 1 pixel distance (which is clearly to see when highlighted) while the right aligned text doesn't -- giving it a 1px distance would look even better.
Hmm... It looks like that 1px left margin is there only when the scrollbar is visible, am I correct (just going by sim now)? Do you propose adding the right margin only when the scrollbar is visible, or add it regardless of whether the margin is there to the left or not? Either way seems kinda weird to me... :-\
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: ilikedirt on August 17, 2006, 05:14:31 PM
I like it. Always thought that having a seperate page for each setting is kind of confusing (almost empty pages where you just can select yes/no).
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 17, 2006, 05:56:52 PM
Ok it seems that the margin or no margin issue is font dependant. For now I hard coded an additional 1px right margin for aligned text, meaning for some fonts there'll be a 2px margin to the right. Perhaps it's ok to leave it like this but I'd prefer if there where some better way to do this...
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: Yotto on August 17, 2006, 07:30:36 PM
That looks pretty cool.  I can't wait to try this "in the field."

As to the margins, I imagine that you can't do much about the font.  I mean, how are you, as the program, going to know if the font has a border of whitespace around the letters?  I think it'll be far more noticable (and far worse looking) to have it be sometimes 1px and sometimes 0px, than for it to be sometimes 2px and sometimes 1px.

IOW, my vote is to force a 1px margin.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: mightybrick on August 17, 2006, 10:28:14 PM
I like it.  I can't test it on my Nano, but I like the idea and the screenshots.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 18, 2006, 06:36:30 AM
Actually, the more I think about it, the less I like the current fix with a forced 1px margin to the right. Some fonts have no margin to the left, so then you end up having 0px to the left and 1px to the right. Worst case scenario: font has no margin to the left, but 1px to the right - then you've got 2 empty pixels to the right and none to the left!  :(

And working around this by just forcing an additional 1px margin to the left will screw up the scrolling. When the text starts scrolling it wont disappear behind the left edge of the screen or behind the scrollbar, it'll disappear 1 pixel away from it. So which would be worse; text that only has margins sometimes, or text that magically disappears 1px away when scrolling?
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: Yotto on August 18, 2006, 06:47:15 AM
I'd have to see it, but I'd guess the 1px margin that magically stays through scrolling would be better.
If you have room for them, margins can only be a good thing.
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: fatherfork on August 18, 2006, 10:08:18 AM
Worst case scenario: font has no margin to the left, but 1px to the right - then you've got 2 empty pixels to the right and none to the left!  :(

Why not force a 1px margin to the left as well?

FF
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 18, 2006, 10:13:41 AM
Worst case scenario: font has no margin to the left, but 1px to the right - then you've got 2 empty pixels to the right and none to the left!  :(

Why not force a 1px margin to the left as well?
FF

Heh, if you had continued reading the second paragraph of that post you'd see that the problem with this is it sort of screws up scrolling:
And working around this by just forcing an additional 1px margin to the left will screw up the scrolling. When the text starts scrolling it wont disappear behind the left edge of the screen or behind the scrollbar, it'll disappear 1 pixel away from it.
Apparently some feel this is ok but I think others (including myself) would be bothered by it...
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: fatherfork on August 18, 2006, 10:18:26 AM
oops, I did read that too. I personally wouldn't have a problem with the forced left margin, but would be satisfied either way. Now If I can only figure out how to compile patches (not a question).

FF
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: fml2 on August 18, 2006, 10:43:54 AM
Interesting...I would suggest modeling it after the "Show ID3 Info" screen (it's a list were each item is 2 lines). It might look cleaner and scale better to other targets (particularly those with smaller screens).

That looks nice at the first glance. But there are so many menu items in RB that I'd rather had one line per item. This way I'll find the desired item faster.

Another possibility: make menu display mode an option (with/without current value).
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 18, 2006, 07:04:43 PM
Latest update:

TODO now: Lots of testing on different targets...



Another possibility: make menu display mode an option (with/without current value).
Well, that IS the way it's done for now, you have the following options:
Title: Re: New patch: Settings Display within menus. Testing/Suggestions Wanted!
Post by: AM on August 19, 2006, 11:28:26 AM
Converted the rest of the main menu, plus the playlist viewer context menu.
Menu conversion is now complete! :D yay!
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: ryran on August 19, 2006, 02:36:56 PM
This looks really nice AM. I definitely look forward to checking it out the next time I update my build.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: psycho_maniac on August 19, 2006, 04:03:25 PM
This looks really nice AM. I definitely look forward to checking it out the next time I update my build.
yes this is truly amazing !!! keep up the good work
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: ww2- on August 19, 2006, 07:26:42 PM
Anyone tested this on the iPod 5G?
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: AM on August 20, 2006, 03:40:34 AM
Anyone tested this on the iPod 5G?
Should work. Works just fine in the simulator.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on August 20, 2006, 09:32:15 AM
I get a compile error with your patch.

*** No rule to make target 'select.c'

You added 'select.c' to apps/SOURCES, but this file is located in apps/gui/
I changed the line to 'gui/select.c'. This worked, but I get a lot of compiler warnings and some more errors.
Do you get them too or is there a problem with my build?

I only applied your patch and tried to compile a H120 Simulator
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: AM on August 20, 2006, 10:07:42 AM
I get a compile error with your patch.

*** No rule to make target 'select.c'

You added 'select.c' to apps/SOURCES, but this file is located in apps/gui/
I changed the line to 'gui/select.c'. This worked, but I get a lot of compiler warnings and some more errors.
Do you get them too or is there a problem with my build?

I only applied your patch and tried to compile a H120 Simulator
Damn. Actually no, the problem was that in the latest patch I forgot to include select.c and select.h  :-[, these are new files that should be in apps. The select.c and select.h in apps/gui/ on the other hand have been obsolete since the setting selection list was included in CVS and should be deleted. I've now uploaded the (hopefully this time) corrected patch.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on August 20, 2006, 11:04:01 AM
It is working now.

One small suggestion: How about adding the submenu indicator ( > ) in the "always" and "for selected only" modes, too?
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: AM on August 20, 2006, 11:20:38 AM
It is working now.

One small suggestion: How about adding the submenu indicator ( > ) in the "always" and "for selected only" modes, too?
I've done some thinking about that, but I'm still undecided. Theres already the risk of the screen feeling cluttered with the single line setting as it is, and turning it on for those modes too increases that further imo.

While on the subject, currently the ">" doesn't actually tell you it's a submenu, just that it's not a setting. Do we want that to be handled differently for menu items that actually open submenus vs ones that perform actions? And should there in that case be a different indicator for menu items that perform actions or just no indicator at all (personally I don't like the look of empty lines in a menu)?

If we opt for any of this I'd be happy to implement support for this, but I don't feel like going through all the menus once again to designate which options are actions vs. submenus. It would be dead easy to (anyone could do it), just tiresome and time consuming...
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on August 20, 2006, 03:31:50 PM
differentiating between actions and submenu items would be useful.
My first thought for actions was an asterisk * but I don't know if I really like it.
There are not that many actions, i can only think of Radio, Recording Screen, Graphic EQ, Rockbox Info, View Current Playlist.
But there is Browse cfg files, plugins, themes etc, which is not really a submenu. I don't know how we should handle these.

I just don't like it when there is 'something' for some items and nothing for others on the right side of the screen.

btw, a graphic for > would be cool, but this has to be in different font sizes...
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: AM on August 20, 2006, 07:13:08 PM
Ok, the latest patch now displays >> (couldn't think of anything better) for menu items that perform actions rather than open submenus (>) in the multiline mode.

I did however opt to not add these right-aligned indicators to the single-line modes. And I'll tell you why: it's beyond the scope of what this patch was originally meant for. If I added those arrows to the ends of all lines you would expect to see them in all menus, not just settings menus (which is what I made this patch for). The arrows where really only added in order to eliminate gaps between lines so it's obvious that there isn't something missing when in multiline mode. And if this was done to all menus, in which case BTW I really should have redone the code in menu.[ch] rather than wrapping and extending them using new source files, pretty soon people would expect the arrows to be there also for folders in the file tree (after all, it would make sense for them to be there for anything that opens a sublevel of anything, and now we're talking not only redoing the menu code but also the list and tree code).

I also think I'm going to give the hunt for 1px margins a rest for now. Those are a) font dependant and b) not just an issue with my patch but with all lists in rockbox (as there are fonts that lack margin not only to the right but also to the left).

Right now I'd rather limit this patch to what it was originally intended for, ie simplifying display and modification of settings - and getting it ready for (hopefully) CVS inclusion *crosses fingers*. After that, if anyone wants to take things further with this they are welcome to modify it however they see fit, and I've tried to the code such as to make things as easy as possible for whoever wishes to to do so.

Anyhow, I have now built and successfully tested the functionality of this patch in sims for all targets except the following for which the sim didn't build (I believe all due to reasons other than my patch):
Archos Gmini 120, Archos Gmini SP, iPod 3G, Toshiba Gigabeat F, SanDisk Sansa e200 and iriver H10 5/6Gb. edit: scratch one more from the list
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Mr. Brownstone on August 20, 2006, 07:25:44 PM
the latest patch now displays >> (couldn't think of anything better)
How about -> ?  ;D

Great looking patch by the way, can't wait to try it out! 8)
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on August 21, 2006, 12:10:49 PM
words
I agree with you completely.
Tonight I try this patch on my player. I hope it is working as well as in the sim.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: dropandhop on September 01, 2006, 06:56:05 AM
I gotta bring this thread back to life.  Any ideas if we will ever see this patch commited to CVS?

Thanks for all the work!  I think it wonderful.

Thanks,
Aaron
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Llorean on September 01, 2006, 08:50:17 AM
Just as a note: The liveliness or death of a thread us usually not relevant to whether or not a feature gets into Rockbox.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: psycho_maniac on September 01, 2006, 12:46:57 PM
I think AM is on vacation. He said on the patch tracker page that he is going to Dublin and thats the last i heard from him on that patch.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on September 01, 2006, 12:59:25 PM
the patch doesn't work with current cvs anymore, so we just have to wait a little longer until AM is back. This idea isn't dead yet and the chances are quite good, because LinusN likes it  :)
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: AM on September 05, 2006, 04:08:01 AM
Well, I'm back from my mini-vacation now (it sort of got extended, after the Dublin trip I've been partying all over Sweden for a while).

Anyhow, it appears quite a bit has happened to CVS while I was away so updating to CVS is no longer just a no-brainer. So I've been thinking; is it really worth it constantly keeping this patch up to date? I mean last I heard (which was a while ago) JdGordon was working on redoing the entire menu system and in the meanwhile it seems to me that efforts to keep this up to date are pretty much wasted. *shrugs* Opinions?
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on September 06, 2006, 10:00:49 AM
I would like to use this patch in my personal build, so I would like to have some updates. I don't need an update for every conflict, I can fix small ones myself, but if there is major work on the menu code it would be nice if this patch gets updated accordingly.

But if you don't have the time, it is ok, too.

If you wait to long with updating it can get harder to make it work with cvs again than it would be if you work on your patch constantly.

If jdGordon applies his menu rework to cvs I would wait until his code is stable before working on the patch again.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: bluebrother on September 06, 2006, 03:51:06 PM
As far as I understood JdGordon on IRC today the menu rework will still need some time. Also, I really liked the idea of your patch and think it makes the menus way better looking. IMO it would be very sad to loose this piece of work -- maybe this can be integrated with the menu rework?
Anyway, I hope this idea gets implemented in Rockbox sometime.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Kitty on October 14, 2006, 06:43:10 PM
what happend to this patch? no one fixing it to fit the latest build?
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Mr. Brownstone on October 14, 2006, 06:54:42 PM

As far as I understood JdGordon on IRC today the menu rework will still need some time. Also, I really liked the idea of your patch and think it makes the menus way better looking. IMO it would be very sad to loose this piece of work -- maybe this can be integrated with the menu rework?
Anyway, I hope this idea gets implemented in Rockbox sometime.
Well said. Kudos to AM and all who have helped with this patch. 8-)
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Kratos on October 15, 2006, 09:32:03 PM

what happend to this patch? no one fixing it to fit the latest build?


apparently people are working on it
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: JdGordon on November 04, 2006, 05:24:33 AM

 JdGordon was working on redoing the entire menu system

I am, but it will be at least another week because I have exams this week, and for some reason my patch has major issues...

that said tho, (and bearing in mind i havnt looked at your path) I dont tinhk it will be difficult to retrofit this onto my patch if/when it is commited.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: dropandhop on November 04, 2006, 09:46:19 AM
JdGordon, that is great news!

Thanks for working on this helpful patch.

Regarding the other stuff you are working on-

How is the menu re-working comming along.  Do you think we might expect that commited sometime soon?

And how about porting over all the button changes?  (I think we are still in need of the plugins being ported)

Thanks again!
Aaron
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: baobab68 on November 04, 2006, 06:31:21 PM
I was wondering, and not sure if this has been discussed before...

When using the new item lists within menus, would it be possible that you can highlight your selection and just press LEFT and have the selection acted on.

It feels odd to me to have to press NAVI on it to put it into effect. I am forever changing the treble setting and then cancelling it.  :-)

Maybe there's a technical or usability reason why it doesn't work like that?
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on November 09, 2006, 10:47:58 AM
It's not a technical reason, it is just a personal preference from the developer who commited the last major change to the menu system.
It used to be that you could press left to apply a setting, now it has changed to right.

So to save yourself a click on NAVI, you can go into the menu, set your setting and leave it again with right (feels like going through the settings screen and when you come to the other end you are mysteriously at the start again with you setting applied...).
It may not be intuitive but it is faster than pressing NAVI.
I liked it with left better (that was the behavior until a few months ago).

There was quite a long discussion on the dev mailing list about how the buttons on the settings screen should be mapped, but I don't remember if it came to a conclusion.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: JdGordon on November 09, 2006, 04:36:45 PM
iirc, the outcome was that a checkmark would be used for the currently set item, pressing navi/select would move the checkmark. pressing right or left would exit with the current setting, and off would cancel
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on November 10, 2006, 09:58:09 AM

iirc, the outcome was that a checkmark would be used for the currently set item, pressing navi/select would move the checkmark. pressing right or left would exit with the current setting, and off would cancel


That's great, exactly what I like the best  :)
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Febs on November 10, 2006, 10:19:13 AM
Actually, I thought that the favored proposal was the one that David-NY summarized today on the developer list:

Quote
UP/DOWN selects items in dir
LEFT exists dir no change
RIGHT enters dir or applies setting and exits (up one level)
NAVI makes change but does not exit dir

An indicator of the current setting (like the checkmark) in addition to the
inverted line (or pointer) based on where your current selection is would be
beneficial.

The checkmark should 'jump' to the inverted line and pause for a tiny amount
of time when NAVI (or RIGHT) is pressed.


I favor this approach for two reasons:

First, it is more consistent with the way that the File Browser works.  In the File Browser, LEFT exits the current folder without selecting the current file.  For consistency, LEFT should exit a settings menu without applying the setting under the cursor.

Second, some platforms do not have a STOP button (iPod, H10) or OFF button (iPod).  Those functions are achieved by a long press of another button.  I do not like the idea of having to use a long keypress to a settings screen with my current settings intact.

Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: mightybrick on November 10, 2006, 10:38:25 AM

Actually, I thought that the favored proposal was the one that David-NY summarized today on the developer list:

Quote
UP/DOWN selects items in dir
LEFT exists dir no change
RIGHT enters dir or applies setting and exits (up one level)
NAVI makes change but does not exit dir

An indicator of the current setting (like the checkmark) in addition to the
inverted line (or pointer) based on where your current selection is would be
beneficial.

The checkmark should 'jump' to the inverted line and pause for a tiny amount
of time when NAVI (or RIGHT) is pressed.


I favor this approach for two reasons:

First, it is more consistent with the way that the File Browser works.  In the File Browser, LEFT exits the current folder without selecting the current file.  For consistency, LEFT should exit a settings menu without applying the setting under the cursor.

Second, some platforms do not have a STOP button (iPod, H10) or OFF button (iPod).  Those functions are achieved by a long press of another button.  I do not like the idea of having to use a long keypress to a settings screen with my current settings intact.



I agree with your statements above.  With an iPod, and no OFF button, it would be nice to have a button mapping as listed above.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: Rincewind on November 10, 2006, 10:39:12 AM

Second, some platforms do not have a STOP button (iPod, H10) or OFF button (iPod).  Those functions are achieved by a long press of another button.  I do not like the idea of having to use a long keypress to a settings screen with my current settings intact.


On the ipods and H10, maybe the MENU button could be used as cancel?

Anyway, it is not a big deal, as long as there is a short way (be it LEFT or RIGHT, not only SELECT!) to apply a setting imediatly and exit, I'm fine.
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: dropandhop on December 21, 2006, 10:22:14 AM
Hey guys,

Just wanted to follow up on this great patch and see where it was heading.

Any news?

Thanks!
Aaron
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: dropandhop on March 17, 2007, 10:33:11 AM
Just bumping this one up again.  It was a really neat patch and was wondering if anyone was interested in committing it.

Thanks again!
A
Title: Re: New patch: Settings Display Within Menus. DONE!
Post by: midgey on March 17, 2007, 11:06:31 AM
Recently, there was a redesign of how the menus work. This patch won't even apply since it was written for the old menu system. Basically, the author has to completely rewrite the patch to get this working again.