Rockbox

Tasklist

FS#8799 - User definable list dimensions (custom list for viewports)

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 05:36 GMT
Last edited by Thomas Martitz (kugel.) - Sunday, 16 August 2009, 22:35 GMT
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

So, this is my first shot at custom lists for viewports for LCD displays.

This task should succeed  FS#5899  and  FS#8457 .

This task adds the possibility to add custom dimensions to lists.

The settings in the theme.cfg file should look like following:
listxpos: 47 (default: 0)
listypos: 47 (default: 0)
listwidth: 140 (default: LCD_WIDTH)
listheight: 150 (default: LCD_HEIGHT)

Note: As there's no validation yet, some values might crash the player. Some values are for example when listxpos+listheight are bigger than the height of the LCD.

A preview is available here: http://www.alice-dsl.net/simonemartitz/asdf/custom_list.png

This patch contains backwards compability, as it resets the settings to defaults upon theme change.

Note: This patch is not finished yet, there's still some work to do, like:
* Verification of the values (see above)
* Extend to other contexts to be consistent (i.e. quickscreen)
* Remotes?
* Code cleanup

Note: It's tested on the e200 sim. Not yet heavily tested. If any error occurs, reset your config upon boot (see manual).

Please test and report problems as well as suggestions.
This task depends upon

Closed by  Thomas Martitz (kugel.)
Sunday, 16 August 2009, 22:35 GMT
Reason for closing:  Accepted
Additional comments about closing:  Accepted in r22365...finally :p
Comment by Paul Louden (Llorean) - Tuesday, 25 March 2008, 05:41 GMT
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.
Comment by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 05:46 GMT
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.
Comment by Paul Louden (Llorean) - Tuesday, 25 March 2008, 05:52 GMT
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.
Comment by Michael Hahn (disorganizer) - Tuesday, 25 March 2008, 09:49 GMT
why is this filed under "bugs" and not "patches"?!?
Comment by Paul Louden (Llorean) - Tuesday, 25 March 2008, 10:05 GMT
Because I was too busy complaining about it to look for errors as well. :-P
Comment by Michael Hahn (disorganizer) - Tuesday, 25 March 2008, 11:46 GMT
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
Comment by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 14:00 GMT
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.
Comment by Michael Hahn (disorganizer) - Tuesday, 25 March 2008, 15:17 GMT
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.
Comment by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 15:51 GMT
>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).
Comment by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 16:01 GMT
Ok, this one should actually apply cleanly on the latest SVN (r16800).
Comment by Paul Louden (Llorean) - Tuesday, 25 March 2008, 17:02 GMT
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.
Comment by Michael Hahn (disorganizer) - Tuesday, 25 March 2008, 17:56 GMT
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?
Comment by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 18:15 GMT
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
Comment by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 18:27 GMT
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).
Comment by Michael Hahn (disorganizer) - Tuesday, 25 March 2008, 18:52 GMT
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.
Comment by Michael Hahn (disorganizer) - Tuesday, 25 March 2008, 19:56 GMT
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?).
Comment by Thomas Martitz (kugel.) - Tuesday, 25 March 2008, 20:18 GMT
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)
Comment by Paul Louden (Llorean) - Tuesday, 25 March 2008, 20:50 GMT
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.
Comment by Thomas Martitz (kugel.) - Wednesday, 26 March 2008, 01:52 GMT
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.
Comment by Paul Louden (Llorean) - Wednesday, 26 March 2008, 06:59 GMT
The attached should be the same patch, without breaking Rockbox Config handling or introducing new inconsistencies with it.
Comment by Michael Hahn (disorganizer) - Wednesday, 26 March 2008, 09:59 GMT
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
Comment by Thomas Martitz (kugel.) - Wednesday, 26 March 2008, 14:31 GMT
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).
Comment by Paul Louden (Llorean) - Wednesday, 26 March 2008, 14:41 GMT
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.
Comment by Michael Hahn (disorganizer) - Wednesday, 26 March 2008, 14:47 GMT
anyways, the bug with the list-scrolling is fixed.
just wanted to report that.
Comment by Thomas Martitz (kugel.) - Thursday, 27 March 2008, 04:53 GMT
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).
Comment by Michael Hahn (disorganizer) - Thursday, 27 March 2008, 08:39 GMT
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.
Comment by Michael Hahn (disorganizer) - Thursday, 27 March 2008, 09:05 GMT
also shouldnt the directive have a leading | ? like in "list viewport: |0|0|....|"
Comment by Dave Chapman (linuxstb) - Thursday, 27 March 2008, 09:26 GMT
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.
Comment by Karl Anderson (D-Shadow) - Friday, 28 March 2008, 17:47 GMT
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
Comment by Paul Louden (Llorean) - Friday, 28 March 2008, 17:48 GMT
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.
Comment by Thomas Martitz (kugel.) - Saturday, 29 March 2008, 02:58 GMT
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.
Comment by Thomas Martitz (kugel.) - Saturday, 29 March 2008, 16:52 GMT
Small fix to wpslist and wpsbuild.pl. Should output correct theme.cfgs now.
Comment by Thomas Martitz (kugel.) - Saturday, 29 March 2008, 17:28 GMT
Another small bug fix with statusbar when list was set to 0,0,... explicitly.
Comment by Thomas Martitz (kugel.) - Tuesday, 08 April 2008, 22:29 GMT
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.
Comment by Thomas Martitz (kugel.) - Wednesday, 14 May 2008, 23:58 GMT
Sync.

Sorry, haven't had time recently for working on. I have many exams at the moment (A-level).
Comment by Travis Tooke (tdtooke) - Monday, 23 June 2008, 23:55 GMT
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 :-)
Comment by Thomas Martitz (kugel.) - Tuesday, 24 June 2008, 13:08 GMT
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.
Comment by Corey (blackthunder) - Thursday, 03 July 2008, 01:28 GMT
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
Comment by Travis Tooke (tdtooke) - Thursday, 03 July 2008, 02:36 GMT
Change width and height to lcdwidth and lcdheight on those lines.
Comment by Travis Tooke (tdtooke) - Friday, 04 July 2008, 02:49 GMT
What do you think about applying your list settings to the quickscreen as well since it's viewportificated now?
Comment by TheKind (TheKind) - Saturday, 05 July 2008, 11:39 GMT
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
Comment by TheKind (TheKind) - Saturday, 05 July 2008, 23:00 GMT
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
Comment by Thomas Martitz (kugel.) - Tuesday, 22 July 2008, 04:02 GMT
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.
Comment by Travis Tooke (tdtooke) - Thursday, 24 July 2008, 08:02 GMT
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.
Comment by Thomas Martitz (kugel.) - Thursday, 24 July 2008, 09:49 GMT
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.
Comment by Thomas Martitz (kugel.) - Saturday, 26 July 2008, 19:15 GMT
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
Comment by Jonathan Gordon (jdgordon) - Tuesday, 29 July 2008, 04:42 GMT
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.
Comment by Thomas Martitz (kugel.) - Tuesday, 29 July 2008, 12:08 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 30 July 2008, 13:51 GMT
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...)
Comment by Thomas Martitz (kugel.) - Wednesday, 13 August 2008, 01:41 GMT
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.
Comment by Thomas Martitz (kugel.) - Wednesday, 13 August 2008, 23:23 GMT
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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 14 August 2008, 01:29 GMT
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...
Comment by Thomas Martitz (kugel.) - Thursday, 14 August 2008, 06:35 GMT
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.
Comment by Thomas Martitz (kugel.) - Friday, 15 August 2008, 23:30 GMT
Implement parent vp for simplelist (NULL for auto custom list).
Comment by Thomas Martitz (kugel.) - Tuesday, 02 December 2008, 15:54 GMT
I'm happy to announce a major update:

This version makes it all cleaner, using the existing parent passing scheme, which also means less changing existing code. The customlist is now the real parent of any gui_list structure which you pass a customlist-initialized viewport or NULL to. We don't need a global customlist viewport anymore.

To use the customlist, you can now call customlist_init(*viewport, screen) to initialize a viewport with the custom screen dimensions. This is similiar to viewport_set_defaults (the next version will probably fuse both). Passing NULL to a list results in using the customlist. Passing any other viewport will make this viewport the parent.

This patch adapts the pitchscreen to use custom list, and fixes quickscreen icon arrangement (quickscreen uses customlist as well). Also, this version changes the way simplelist uses the customlist. It uses it by default, and you cannot change it per screen for now, in order to fix that involve FS#9514.

This patch cleans up list drawing functions to NOT accept a parent seperately. The parent is already part of the list structure passed to it.

Three notes:
-star plugin crashes upon exit: I have no idea why.
-this patch suffers from  FS#9600 
-binsize increase on my e200: <1k

Please test!
Comment by Karl Anderson (D-Shadow) - Tuesday, 09 December 2008, 08:06 GMT
Hey I stumbled across a weird "bug" in v14. I'm using r19324 with the patch applied. The viewports at origin (in the WPS) are drawn at (0,9) instead if status bar is enabled in the settings. However on reloading a theme with status bar disabled in the settings, it works flawlessly. As you may have already noticed, I had the status bar disabled in the wps during the entire process.

I double checked it by testing my themes on an official build, and I've come to a conclusion that it happens only while using the patch. I'm attaching a screen dumping depicting the error.

Just thought I'd let you know about that.
Comment by Thomas Martitz (kugel.) - Tuesday, 09 December 2008, 09:32 GMT
Try this version. It does the things a very little different, as it incorporates  FS#9603 . At least, I cannot reproduce your problem with themes using a statusbar in the menus but no statusbar in the wps on my e200.

But I've noticed that you have at least 1 other patch applied. Don't report problems if you didn't make sure it's *only* caused by this patch.
Comment by Karl Anderson (D-Shadow) - Wednesday, 10 December 2008, 04:42 GMT
Works just fine now. However an argument is missing in viewport_set_defaults in alarms_menu.c. I fixed it myself, but just thought I'd point that out.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 21 January 2009, 12:37 GMT
kugel, this is a very quick patch to show what I was trying to explain in irc.... just imagine that instead of the hardcoded numbers in viewport_set_defaults() its the values from the viewport string.... this works fine...
There is a problem with the statusbar handling in the WPS which is why your pastebin before didnt work... I'm going to have a quick attempt at fixing that now
Comment by Thomas Martitz (kugel.) - Wednesday, 21 January 2009, 23:08 GMT
current progress in implementing this as a vp which is supposed to apply to the whole UI. Doesn't quite work yet.
Comment by Karl Anderson (D-Shadow) - Saturday, 24 January 2009, 04:36 GMT
With v16, it seems the screen is not cleared and the backdrop is not displayed before drawing the list viewport. To reproduce it, you can switch themes or go to the WPS screen and come back to the main screen. Check out the attached screen dump.
Comment by Thomas Martitz (kugel.) - Saturday, 24 January 2009, 14:55 GMT
I know...I write that it doesn't work.
Comment by Thomas Martitz (kugel.) - Thursday, 05 February 2009, 01:50 GMT
So, this one is supposed to work again.

Many changes to gwps.c to consistently set the main backdrop when exiting the wps, this solves many "dead parts" issues.

This is the first version I'd consider as committable soon'ish. I've reduced the redraws to a minimum (basically, only when the wps or some fullscreen plugin is exiting, like it's supposed to be). Also, mainly only code that doesn't want to use custom vp are changed, other code is mostly untouched.

Please test and report any issues!
Comment by Travis Tooke (tdtooke) - Thursday, 05 February 2009, 12:59 GMT
I have noticed a few things: When changing a theme from one without a custom list to one with a custom list the selector bar settings are not changed. If I turn off my iPod off and turn it back on and the theme I'm using has a custom list then the selector bar is a solid block that covers up whatever the selected text is if the theme uses a gradient bar. I also can't seem to change the colors on my gradient bar anymore with rockbox, I can only do it in a .cfg file. I noticed while saving a .cfg that after I saved it part of the screen up top was not repainted. This only happens with a custom list. I did the test with this patch and the sort tags patch only. I would have done a pure test but I didn't want to have to rebuild my tagcache twice. I seriously doubt sort tags can effect any of this at any rate.
Comment by Thomas Martitz (kugel.) - Thursday, 05 February 2009, 13:16 GMT
I cannot reproduce your first issue, please make sure the theme you change to really specifies line selectors. Many themes don't that's why you get the old one.
I can reproduce your second issue, I'll look into it.
Comment by Thomas Martitz (kugel.) - Thursday, 05 February 2009, 13:35 GMT
Ok, this addresses the line selector colors.
Comment by Dieter (dip) - Tuesday, 03 March 2009, 22:34 GMT
out of sync
Comment by Thomas Martitz (kugel.) - Wednesday, 25 March 2009, 03:13 GMT
Sync
Comment by Thomas Martitz (kugel.) - Sunday, 29 March 2009, 20:30 GMT
Sync
Comment by Karl Anderson (D-Shadow) - Friday, 03 April 2009, 17:35 GMT
With v17d, the backdrop is no longer cleared before drawing the list. The status bar also blinks on scrolling up and down. This only happens when the theme is changed. It does not happen while switching between the menu and WPS screen.

I'm using build 20573 by the way. I'm attaching a screen dump, as always.
Comment by Dieter (dip) - Thursday, 07 May 2009, 22:04 GMT
out of sync
Comment by Thomas Martitz (kugel.) - Friday, 08 May 2009, 01:29 GMT
Here's a sync'd version. I fixed up splashes and set_int() a bit (not yet decided if that's the proper way though).

Please report any issues!
Comment by Thomas Martitz (kugel.) - Tuesday, 12 May 2009, 00:52 GMT
Fixed theme loading, and do after splash cleanup a bit different.
Comment by Jonathan Gordon (jdgordon) - Sunday, 24 May 2009, 16:37 GMT
a general comment for viewport.c ... the funciton names there are not brilliant, and this patch doesnt help that at all... you *have* to add comments in there so its clear what each function does untill we come up with a better naming scheme for them (well, the comments should be there anyway..)
I'm not crazy about the idea of calling it "custom vp" everywhere.. maybe "user viewport" instead?

what is viewportmanager_themechanged_cb() doing? and why is it using cb_func() without making sure its not NULL?

also, "list viewport" in the wpslist and settings? again, lets call this "ui viewport" or similar...
Comment by Thomas Martitz (kugel.) - Sunday, 24 May 2009, 16:55 GMT
Yea, I'm planning to revisit and revise later for better names and comments. Also, I planned to rename the vp it to "ui_vp" ("list viewport" is from the very beginning, I didn't rename that yet to not break my testing themes again and again).

themechanged_cb can refresh the whole screen with the clear/update combo, but with calling a "content drawing" (for example a gui_synclist_draw() wrapper as in the above patch) function inbetween so that the screen is not empty after the clear/update combo. I make sure it's not NULL in my local version.
In the above patch, it's used to redraw the lists after changing the theme (hence the name), but it could be used more generally (and it is in my local version).
Comment by Jonathan Gordon (jdgordon) - Tuesday, 26 May 2009, 06:05 GMT
and this is why this patch is so awesome.... introducing background WPS... (proof of concept of course)
summary of changes from v19 to make this easy to get in as a seperate patch later:
- add a call to gui_wps_update(&gui_wps[i]); in viewporot_*_redraw() and at the end of _leave_wps()
- add the enum for the WPS mode, music/background... in gwps_refresh() check if the mode changed, if it did do a full redraw
- add the mode tag to the wps so users know if the WPS should be allowed to draw in the UI viewport or not.
- (not in this patch) add an option to not bother with the background WPS updating... *some* "people" might not want this ( :O )


Thomas, can we get a synced update of the patch and see about getting it commited before the feature freeze which will be happening in the next 2 weeks probably...

edit: attached the test wps and the config to try with
Comment by Thomas Martitz (kugel.) - Tuesday, 26 May 2009, 07:38 GMT
Ha, yea awesome!

I'll try to bring something committable up this week. I think we don't necessarily need to activate this things in the release already, right?
Comment by Jonathan Gordon (jdgordon) - Tuesday, 26 May 2009, 16:00 GMT
no we dont, but its always nice gettings thinggs finished :)
Comment by Thomas Martitz (kugel.) - Wednesday, 27 May 2009, 00:10 GMT
Here you go!
Comment by Dustin Skoracki (sko) - Wednesday, 27 May 2009, 09:24 GMT
Hmm... is not compiling, I got
"apps/gui/list.c: In function 'list_init_viewports':
list.c:(.text+0x64): undefined reference to 'printf'"
for now I replaced printf() by DEBUGF on line 79 in list.c (as I don't know exacly which header to include for printf() ) and it worked.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 03 June 2009, 04:57 GMT
- the apps/plugins/test_fps.c changes look suspect... #if 0....
- I'm not keen on the GUI_EVENT_SPLASH_CLEANUP event and such, but not enough to totallty oppose the patch.
- apps/gui/splash.h empty change
- viewport.c functions need comments... especially viewportmanager_ui_vp_changed (and its assosciated event?)... what is its param?
- viewport.c in general... funciotn names arnt great... I'm ok with this being done later though
- list_vp_config[50] seems to be a waste... xxx|yyy|www|hhh|f|cccccc|cccccc| is the max (for now) needed... thats 32... at least use a "round" number... 48...
- setting name "list viewport"... its the whole UI... not just the list... but i guess this is fine if others agree...
- plugin api version needs bumping


other than that it looks good.... I dont see the need to find every tiny redraw issue before commiting... when they show up they can be fixed... as long as the main bits of the core are good, then its good enough.... (can you tell i want this commited? :p )
Comment by Thomas Martitz (kugel.) - Sunday, 28 June 2009, 23:04 GMT
latest work on the patch, fixes a few things, but reverts the splash things for now.
Comment by PaulJam (PaulJam) - Tuesday, 30 June 2009, 12:54 GMT
I tried the patch (v20) on my H300 with r21574, settings were reset during boot.
After boot the main screen is mostly blank, only the statusbar is shown. The same happens in the uisimulator. On the LCD remote screen the menus are visible (at least in the uisim; i don't have an LCD remote to test on target).
Comment by PaulJam (PaulJam) - Tuesday, 30 June 2009, 15:35 GMT
The problem described in the previous comment seems to be caused by a typo in apps/gui/viewport.c line 319: if (screen= SCREEN_REMOTE)
That should probably be a '==' .
Comment by Thomas Martitz (kugel.) - Tuesday, 30 June 2009, 18:53 GMT
Probably, have you tried it?
Comment by PaulJam (PaulJam) - Tuesday, 30 June 2009, 20:51 GMT
Yes, that fixes the issue. While playing around with that feature i have noticed a few other problems:

1. Sometimes parts of previous screens remain visible after leaving them. I have seen this when leaving the graphical equalizer or the virtual keyboard. (on a related note: those screens usually look bad with a backdrop that is designed for the use with a custom viewport, so they should either use the custom VP or clear the backdrop when they use the whole screen.)

2. Same as 1. but with splashes. There are a few slashes that are usually very wide (for example during creating a playlist or initializing the database) and if they are wider than the custom VP then the parts that are outside of the VP stay visible.

3. The width of the quickscreen elements doesn't seem to get adjusted. This looks bad when using a narrow viewport and a language where the quickscreen items have long names (for example deutsch.lang), then they even overlap.

4. When leaving the recording screen then the peakmeters stay on the screen and the menu is shown in the area below them. Sometimes the device freezes shortly after that.
Comment by Thomas Martitz (kugel.) - Tuesday, 30 June 2009, 20:59 GMT
1 and 2 are known (there are many situations like these, they're gonna need to be hunt down once this is in SVN), 3 seems like a bug in the quickscreen (I'll have a look), 4 sounds funny, I'll look at it too!

Thanks for the feedback! I appreciate that.
Comment by Thomas Martitz (kugel.) - Sunday, 12 July 2009, 22:56 GMT
I fixed the recscreen issue. The quickscreen one is a seperate bug.

The first issues are going to be adressed "as they appear" in SVN.
Comment by PaulJam (PaulJam) - Monday, 20 July 2009, 08:31 GMT
I tried the last patch (v21) on my H300 with r21979. I removed the four printf() lines because they caused build failures and seemed to be only debug messages for the uisim.

After booting up the player imediately freezes. the main menu is shown, but it doesn't react to any button presses. The disk keeps spinning and a hard reset is required.
The H300 uisim (on cygwin) also crashes.
Comment by Thomas Martitz (kugel.) - Thursday, 23 July 2009, 17:05 GMT
Fix the above problem and a few minor other ones.

edit: wrong patch
Comment by Thomas Martitz (kugel.) - Thursday, 23 July 2009, 18:01 GMT
a few minor changes,

changing the statusbar from top to bottom or vice versa seems a bit broken.
Comment by Thomas Martitz (kugel.) - Wednesday, 12 August 2009, 22:59 GMT
fixed the statusbar thing, and redid the api a bit.

consider this as commit candidate.

edit: correct patch how.

Loading...