Rockbox

You shouldn't reset settings on .cfg load. Generally speaking, the whole point of being able to leave lines out of .cfg files is that so they can change some settings without changing all of them. This patch basically means my list will reset itself it I load a theme that just changes the iconset, for example. Not good.

Second try...

Paul Louden, you might be right. But if I loaded a theme that's not using custom list, and the custom list coordinates wouldn't reset, this theme would accidentely use custom list.

Anyway, to be honest, I took that solution from an older patch, since I was a bit lazy in the end :). As I said, it's only a first shot, so that can be changed.

Then that theme is "broken". As long as there's a line themes can include to say "the list viewport should be fullscreen" then these themes can have that line, and should be updated to include it. Preserving backward compatibility is, in my opinion, not important enough to remove existing functionality.

why is this filed under "bugs" and not "patches"?!?

Because I was too busy complaining about it to look for errors as well. :-P

ok, i see its corrected now. on topic:

why dont we use the syntax of the "old" list-patch for the cfg-files?
i think it was vplist:|x|y|width|length|font|...
i would prefer anything which is like with the normal viewport definitions in the wps.cfg
propably even the parsing function for the wps could be used for config-entry parsing then?

another thing which propably needs to be pointed out like in the other "old" fs entry:
the wps tags which are shown on other viewports will not be updated if a list is displayed in only a part of the screen, correct?

so this will need to closely work together with the hopefully upcoming "statusbar viewportification" patch to enable updating of trackdata in the background.

offtopic:
another question to viewportifying things:
wouldnt it make sense to make "wps"-files like for the wps for each context?
we would then of course need additional directives for the functions (for example one to display the list content).
an example would be theme.list, theme.menu, theme.browser, theme.radio, theme.statusbar, ... or whatever

Paul Louden. Sorry, but this seemed easier to me. Instead of updating 1 milltion themes to work on a patch (so not even SVN), I'd rather include this function to make it easy.
I don't think functionality is lost, as only these 4 settings are resetted.

Michael Hahn, thanks for the input. To your idea, I've had this idea too actually. But I don't know if's possible with existing settings code (at least from what I've seen). So we need to intruduce a new settingstype. I'd gladly change the syntax to a more %V like.

>another thing which propably needs to be pointed out like in the other "old" fs entry:
>the wps tags which are shown on other viewports will not be updated if a list is displayed in only a part of the screen, correct?
No, it should work fine. My e200 updates the screen properly.

i will try tonight then. either something has changed recently in the svn or whatever, because if you only reduce the viewport for the lists you still see the wps "in the background", and the scrolling of lines, but the actual data (runtime, next, album etc) is not updated during list-display.

this is indeed updated when using the statusbar patch in parallel to reducing viewport size for lists.
but i will try and report tonite.

>i will try tonight then. either something has changed recently in the svn or whatever, because if you only reduce the viewport for the lists you
>still see the wps "in the background", and the scrolling of lines, but the actual data (runtime, next, album etc) is not updated during
>list-display.
I know that behavior from the old viewport-list patch, but it doesn't happen in this patch.

>this is indeed updated when using the statusbar patch in parallel to reducing viewport size for lists.
>but i will try and report tonite
What statusbar patch?

Anyway, I updated the patch. A bit bug fixing, especially when statusbar is displayed. Test on sim which cabbiev2 (no customlist) and azure_ultimate (which uses customlist).

Ok, this one should actually apply cleanly on the latest SVN (r16800).

You're breaking the way every other line in every other config file is handled with this. Functionality is most certainly lost: That functionality is the explicit previous assurance that "if a config file doesn't include a line, that setting is not changed by loading it."

As I said, this feature will result in the loading of iconsets resetting list viewports too. Frankly, it's juts not thought out. YOU don't have to update the broken themes. And in fact nobody does, they don't *have* to be updated.

But as it stands, background colors and other things can already result in an unusable Rockbox if themes don't properly include all the lines they should. Theme .cfg files must be properly structured.

You don't have to fix it, I suppose, but it will get changed before commit I'm quite certain, so you might as well go ahead with it.

i tested it, and have some problems:
1) when scrolling, the last 2-3 lines are missing from display (when the selected line goes beyond the display range it does not scroll until you go 3 lines further down the list)

2) when opening a list (for example context menu) the display is cleared (so the wps does not display anything). is this on purpose?

Paul Louden, I don't get understand what you are trying to tell me. In your latest comment you said older themes "don't *have* to be updated", but in a previous one you said that older themes must be updated (f.e. to say "the list viewport should be fullscreen").

The point is, when I used a customlist theme and go back to cabbiev2, cabbiev2 would use the same dimensions, since it doesn't include anything regarding customlist. I prefered the way I choosed over updating older themes to specify that their list-vp should be fullscreen.

And, I'm aware of that, that every theme that just changes the icon set will be broken, as it will reset the list dimensions too.If you can tell me a way to get this working without resetting the 4 values but without updating every single theme, tell me please.

How about that: We introduce a setting called like "do not reset customlist", so that icon changer themes etc could include it in order to only change the icons? This would minimize updating older themes. Else I don't see a way (without resetting customlist settings) to make i.e. cabbiev2 not using customlist without inserting the default values into cabbiev2.cfg

Michael Hahn,

Can you specify your problem 1 please. Which theme, scroll-/statusbar on? Which target? I actually thougt I fixed this issue in the v2 version.

2) I'm not sure what you are meaning. I don't change how the display is cleared/updated, I just change the viewport (x,y,width,height) dimensions. Technically the behavior should be the same. What do you mean by "so the wps does not display anything"? Of course the wps doesn't show up when you enter a menu. This patch doesn't mean to show parts of the wps in lists (yet).

regarding
1) the simple-theme on the wps-gallery, target sansa e200
your patch v2b applied to latest svn

2) strange. so which viewport do you use for the list? in the "old" patch, when the list was displayed it was in a new viewport ontop of the others. so the remaining parts of the wps still could be seen (even though it didnt update them).
it seemed to me that now the viewport is drawn, but on a clear screen.
(nothing is "left" of the original wps).
thats what my question at the beginning was about: if the wps elements can be seen while displaying the list viewport, will they be updated? thats what i was wondering about *g*
so that propably was just a misunderstanding.

again regarding 2):
if i manually change the viewport size in viewport.c (without your patch) then the wps is still displayed in the "background" (without updating tags).
so imho you must do more than just modify the viewport size. or you modify the wrong viewport (background?).

Have you even looked into the diff? I don't even edit viewport.c. Only the both list.c's. I don't see where I edited the wrong viewport.

I don't know what your problem is. It's working fine, isn't it? (Besides the issue you have posted)

How 'bout, you simply respect "The way Rockbox handles settings" and don't reset them rather than making up yet another new setting to complicate things further.

Older ones don't *have* to change, because they'll still work unchanged, they'll just look funny. When loading a settings file, the only settings that should ever change are those explicitly in that file.

You introduce another problem: It only happens on themes loaded while in the theme directory. That means the same .cfg file will work differently depending on where you invoke it from. This is destined to cause all kinds of user confusion.

Simply put, to be committed, this needs to be fixed.

Edit: And as a note, it would take one shell script, and probably less than a few minutes of processing, to cat the necessary lines to fix every theme, ever.

Michael Hahn, I've downloaded the simple theme in order to reproduce your issue. I coun't reproduce it. I tried long lists (i.e. volume) and some dirs with much files in it. Everything was just fine.

The attached should be the same patch, without breaking Rockbox Config handling or introducing new inconsistencies with it.

i used the attach config file on a fresh build of the svn.
tools/configure 50 S
make
make install

the put the config inside archos/.rockbox

the i browse to the rockbox folder and see the following list:
http://img149.imageshack.us/img149/1820/dump080326105148nl3.png
which looks ok. but when going down with the cursor after the position in the pic above, it needs 5 times "down" to get the scrolling to show this
http://img180.imageshack.us/img180/964/dump080326105158ug4.png
in which you see that the marked line is far (4 lines) below the current y-width of the viewport

Ok, the above issue should be fixed.

This version removes the hack (it's not based on Llorean's version, since I allready had this done before he uploaded his patch), and implements settings in Settings->Theme Settings (to set the 4 values and to reset the list).

@Llorean, before you ask why I implemented the reset list option, let me answer now: Firstly, there's a reset color option (which disproves your bin size argument a bit).

Also, I think it's very handy, since there's a slight chance that the list is messed up, so that you need to reset it (I was able to do so, while my list width was near 0).
At last, I think the possibitly to quickly reset a messed up list is very nice (and heavily used while testing my code).

It doesn't "disprove" anything. Please, learn English. There's no point to be "proved". Binsize is valuable, when possible it shouldn't be wasted. That doesn't mean it hasn't been wasted in the past, and I never claimed it was.

Meanwhile, "resetting" is unneeded in the final version. A .cfg file can be used for testing. Why are you so resistant, constantly, to such things? The Colors have a very customized UI that makes it much less simple to reset them. There are very, very few settings that have individual reset areas, and there's no sense in going out of your way to try to create a trend of doing so. The UI doesn't need more clutter. Please be so kind as to remove it.

anyways, the bug with the list-scrolling is fixed.
just wanted to report that.

Here's a new version. It changes the settings. A more %V like one is used now, to be a bit more consistent with the wps.

Setting is:
list viewport: x|y|width|height|

Currently only these four values are supported, but it can be extended to font, colors etc later.

This version also removes the stuff in Settings->Theme Settings alltogether. Use a .cfg containing "list viewport: 0|0|<LCD_WIDTH>|<LCD_HEIGHT>|"

BTW: I have some problems with the default value of this setting. Currently the default values are situated in apps/bitmap/list.c instead of settings_list.c. The problem is to have a string as default value, which contains the values for LCD_WIDTH and _HEIGHT (which are integer). If anyone has an idea how to do that in a good way, please state it here.
I'm way too tired to think of one today.

Oh yea, and I also added a theme for testing (it's for e200).

would it be possible to use list viewport: |||| for resetting to defaults = fullscreen?
so if any value is empty, the value gets reset to either 0 (for x/y) or screen-value (width/height).
that way a vp-definition could use 50||200|| for x=50,y=0,height=200,width=screenwidth
for config-saving this would of course be converted to the actual values.

also shouldnt the directive have a leading | ? like in "list viewport: |0|0|....|"

Kugel,

I've only looked briefly at your patch, but can see various issues:

1) You seem to be ignoring the fact that Rockbox works on devices with multiple LCDs (i.e. a remote LCD)

2) A viewport isn't a "FILENAME_SETTING" and doesn't need 261 bytes of storage (MAX_PATH+1). IMO you should add a new "VIEWPORT_SETTING" type to the settings system, which can then be used for any viewport we want to make user-customisable.

3) You should use the viewport parsing function from wps_parser.c to parse any viewport struct - you MUST validate the values of this struct against the LCD size and there's no point re-inventing that function. To do this, you should probably move the majority of that function into apps/gui/viewport.c - so it can be used by both the wps parser and settings code.

4) In draw_title, I see you are copying the viewports to local copies on the stack - that won't work with scrolling lines, as the viewport must still exist in memory when the line is scrolling (the scrolling code stores a pointer to the viewport). That's why the viewports were declared statically in the first place.

5) In list_draw, you always subtracts the statusbar height (if enabled) from the custom list height, but only adjust the y co-ordinate if it is < the size of the statusbar.

6) It's very inefficient to keep reparsing the string representation of the viewport every time the list is drawn on the screen - I would just do it once.

Would it be possible to assume list co-ordinates and dimensions as default, if they're not specified in theme configuration file? I hate when the list position and dimensions (of the initial theme) are retained while switching over to a theme with unspecified list co-ordinates and dimensions.

~ Karl

That feature was intentionally taken out of the patch. Create a theme that just resets the list, and load that one first, or make sure you don't have themes that are broken with this patch.

So, here we got a new version. I've fixed and added several things. I also fixed Dave's issues, except the last one, which I can't think of a solution right now. Any help here?

Anyway. I've added a viewport parsing function (viewport_parse_viewport) in list.c (mostly copied Dave's wps_parser code). This function is able to parse any viewport like structure, even if the seperator isn't a pipe. I've changed the wps parser to use that function. My list code does that as well. This function contains the lcd bounds, which means, the list viewport will be validated against the LCD size, and rejected if needed.

I extended the setting to: list viewport: x,y,width,height,font,fg_color,bg_color. This means, you aren't only able to change the dimensions, but also to change the fonts (0 (sysfont) and 1 (font_ui) are supported), as well as the colors for the list.

I also implemented this patch for remote lcds now. You can change the lists dimensions (and font, color) on a remote now too. I doubt it will be used much though :P

I've modified all default themes to contain "list viewport:" this this is needed to reset the list. So, all default themes will be doing well, and not broken (aka every default theme will reset the list). Themes that want to use the whole screen should insert this line too.

This version is tested a bit more, only on simulators though. I tested it on e200, ipod video, ipod mini, h300 (incl. remote) and archos player (to check if my code doesn't break charcells). Real target testing is very much appreciated.

My TODO list: text editor uses the list viewport as of now. As there's no backdrop in the text editor, I wouldn't let this be. text editor should be full screen.
I wanted to make the quickscreen and some other screens (those which show the backdrop especially) uses the dimensions to have a consistent look. I'm not quite sure if this should be the object of this patch (especially since those screens need to get viewportified before). Any comment?
Fix Dave's last issue. As I allready said, I might need help here.

Nothing more. Besides that, I think this patch is working pretty well.

Small fix to wpslist and wpsbuild.pl. Should output correct theme.cfgs now.

Another small bug fix with statusbar when list was set to 0,0,... explicitly.

Tiny bug fix. I'm working on a version which removes that reparsing at every redraw. I also hope to get the parent edited. The attached patch still in the "old" way.

Sync.

Sorry, haven't had time recently for working on. I have many exams at the moment (A-level).

sync I'm breaking from your naming convention so as not to confuse one of my dodgy syncs with the official work on this patch :-)

Nice.

I think I'll be putting some effort into this again soon (not too soon though). I hope JdGordons commits in the past few month make things a bit easier.

I'm having troubling with this patch. it patches okay, but I have build errors with viewport.c 'struct screen; has no member named 'width' and 'heighth' lines 129-132

Change width and height to lcdwidth and lcdheight on those lines.

What do you think about applying your list settings to the quickscreen as well since it's viewportificated now?

Hi,
I tried to make this patch working but I didn't.

I added this line to my theme.cfg file:
list viewport: 6|16|118|118|1|FFFFFF|000000|

What am-i doing wrong ?

Thanks

The answer was "use ',' instead of '|' and don't choose a height bigger thant your screen" :D

-> list viewport: 6,16,118,100,1,FFFFFF,000000

So, finally an update.

This version mainly addresses the efficiency issues mentioned by linuxstb. It should now be much more effective. It only parses the custom list vp config string once, unless/until the user changes settings - instead of parsing it 4 times at every redraw of the list. I suppose this will work better.

Also, the quickscreen now uses the dimensions.

Known issues:
- Plugins with lists, like text editor, use the dimensions even though they don't show the backdrop. This is NOT intended, and I tried to overcome that, but rb->do_menu doesn't work as I expected (i.e. I told it to do the menu in a new fullscreen parent vp, but it still uses the custom list dimensions). I don't think I can fix this soon'ish. Any help is of course appreciated.
- when the user toggles the statusbar, the dimensions change takes effect too late, i.e. only after the user selected another item in the display menu. The complete list should probably redrawn when the statusbar changes.


It's not too heavily tested. Although I tested it with h300 sim (the remote should still work), and succesfully compiled the recorder, player, h120 sim. Of course, it's mostly tested with the e200 sim. Tests on real targets are appreciated.

Outstanding! I've only found one small glitch other than what you have already mentioned. When going from the WPS to the quickscreen the screen is not cleared like it is when you go from the WPS to the main menu so you see some lingering text from the WPS on the edges of your viewport in the quickscreen.

Ah, I see. You're right. I'll see what I can do about that. To be honest, I've implemented it very quickly for the quickscreen (took me <5 min) and I only tested going to the quickscreen from the menus.

Ok, another update:

-fix quickscreen colors by defaulting the viewports before parsing in viewport_parse_viewport (aka make sure the list_info viewport is fully initialized and not only with the values given by theh vp string)
-fix quickscreen showing "dead" parts of the wps

-allow "-" in the remote/list viewport string in the theme.cfg (useful for colors, so a x,y,width,height,font,-,- will use the global_settings colors)
-finally implement the the functionality added in r17690 properly
-optimize the VIEWPORT_SETTING macro a little bit

ok, ive just had a very quick look at the patch...
firstyl, the quickscreen changes look wrong.. your putting the qs in the lists vp and not its own one?
Is the point of this patch to move the whole ui into a smaller rectangle than (0,0)(width,height)?

I think you should concentrate more on getting a framework where screens can get their viewport from the config string very easily.
something to tihnk about...

set up a viewport manager which can load the vp definitions from a single file specified in the config. then each screen would do something like this...

struct viewport *vp[NB_SCREENS];
FOR_NB_SCREENS(i)
{
vp[i] = load_viewport_config(i, VP_QUICKSCREEN);
}

each theme should have its own .vpcfg file which would define all the vp's for the theme (browser, menu, wps, QS, .....) any missing would be given "default"...
One thing that is 100% required for this is a simple plugin which would allow you to manage the definitions on the target. all it would need to do is have a list of viewports in the .vpcfg file, user choose one to edit, the screen then displays a rectangle for the vp and allows the user to move and resize it and change its colours.


once thats done the "only" issue remaing is making sure everything uses the right viewport which needs to be done anyway.

I'm not really a fan of different files for different contexts.

Look, if one wants a custom list, then he'll does so, because his favorite backdrop contains a rectangle or something so that it looks nice if the list is within this rectangle.
And for me, it's just logical, that every context which shows the backdrop uses this custom list (the same list). Multiple files is redundant because they'll most likely end up containing the same list dimensions.

The framework you are talking about sounds nice, but in a similar way the patch versions before v9 worked this way. There you could do "viewport_parse_viewport" in every context to parse the settings string into a vp.
However, this way the viewport was parsed multiple times, when it was actually only needed once.

What I've done now is: Just #include "list.h" and you'll have access to the custom list.

For the quickscreen changes:
I'm still putting the quickscreen in it's own parent (struct viewport vp[NB_SCREENS]; and then gui_quickscreen_draw(qs, &screens[i], &vp[i]);).
What I changed is basically limit the parent viewport in quickscreen_fix_viewports (the parent itself isn't really used, but the dimensions), so that the little vps for the items would not exceed the list dimensions.

Oh, I just see I've left the second gui_quickscreen_draw use list_info, but that's a leftover, which I've forgotten to change before uploading. Basically, only quickscreen_fix_viewports should use list info.

In the end, I changed 2 lines in quickscreen.c: #include "list.h" and the quickscreen_fix_viewports call. I don't think this is wrong.

OK, I started a reply yesterday but must have stopped myself from hitting add comment....
Attachd is how I think this patch should be done. It needs some polish and the plugin to configure it, but its 90% complete.
to test it... put the viewports.vpcfg file into the .rockbox/themes folder and open the root/main menu.

if you look in viewport.h there are now half a dozen viewport config defines ready, so all that is needed is for the screens mentinoed to call viewport_get_viewport() and the preloaded viewport will be retrieved. if one hasnt been defined then the default one is used... if there is no default one then the fullscreen is used.

right now only the main screen is setup for this, but the change is a 3 line fix (could be changed to 1 line with a function call...)

The viewport'ification of the rec screen revealed an (imho) unwanted behavoir/bug in this patch: Any list is drawn within the list dimensions. Even those who actually define their own viewport. That means, with this patch, the list on the bottom in the rec screen is drawn within the list dimensions, and not in the dimensions specified in recording.c. I'll look into it.

I think this is a major update:

While fixing the rec screen issue, I've noticed the major design flaw of the previous versions. I kinda knew before about this flaw, but it didn't cause issues and I couldn't seem to make it work otherwise.

This version now uses finally the parents properly and minimizes the changes to apps/bitmap/list.c and apps/list.c. This means, the list drawing code now receives the list info from the parent, and not from the global list_info viewport anymore. Also, you can now initialize lists with custom lists just by passing a vp. do_menu also uses the passed parent properly. This means, I found a working combo of screen.clear_display, screen.update etc.

calling do_menu and gui_synclist_init with NULL as parent will make it use the custom list, for a fullscreen vp you'll want to create one before.

This version also fixes plugins using the custom list due to the old do_menu bug. I've converted star, pictureflow and text editor. Note: You need to reinit the list viewport on plugin exit with rb->init_list_info(enum screen_type screen), otherwise the list shown after exiting the plugin will use the plugin vp.

I also made rec screen use the custom list. THere's a minor glitch though: The top viewports dissapears noticeably for a short time. This is probably due to some extensive usage of clear and update functions.

I still think my method is better, and that comment proved it.
The list code was designed from the start to accept a parent viewport. simply adding
FOR_NB_SCREENS(i)
vp[i] = *viewport_get_viewport(VPDIM_RECSCRN_LIST, i);

and passing that vp in would have solved the problem.. and extra 3 lines to my original patch...

I've never argued against passing them as parent. I was actually always in favor for that. It was just the problem that without other changes the unused parts of the screen would show stuff from the previous screen, so that I couldn't get it to work. Your old custom list patch at  FS#8457  had this problem too.

Implement parent vp for simplelist (NULL for auto custom list).