Rockbox

Tasklist

FS#8457 - lists using viewports

Attached to Project: Rockbox
Opened by Jonathan Gordon (jdgordon) - Monday, 14 January 2008, 00:58 GMT
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 05 March 2008, 10:00 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

This is my WIP to finish using viewports in the lists.
I have split the drawing code into gui/bitmap and gui/charcell to simplify it a bit, and the logic code is in gui/.

apps/viewport.c is for misc viewport funcitons which dont belong in firmware/.

the only thing in svn which is not yet in this patch is the text offset drawing stuff (when you hold left/right to show more of the item text), but I'm thinking about dropping that completly if noone minds...

the lists viewport can be setup by a .vp file in /.rockbox/themes which looks like:

x:10
y:50
height:80
width:100
foreground color:aabbcc

you then need to add the line: "list viewport file: /.rockbox/themes/list.vp" and it will be loaded.

post any bugs you find.
   changes (41.6 KiB)
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 05 March 2008, 10:00 GMT
Reason for closing:  Accepted
Additional comments about closing:  unless someone wants to pick up the customizability stuff before I do, I'll open a new task for it when I get to it...
Comment by Jonathan Gordon (jdgordon) - Monday, 14 January 2008, 05:14 GMT
this should compile on all targets and work exactly like svn...
Unless there are any bugs found, I tinhk this is done.

edit: seems to be about +500bytes on the rec rockbox.bin file with this...
   changes (43.8 KiB)
Comment by Travis Tooke (tdtooke) - Monday, 14 January 2008, 12:12 GMT
Cool!, does this require your patch on the viewports page?
Comment by Dave Chapman (linuxstb) - Monday, 14 January 2008, 13:23 GMT
A few comments (I haven't had chance to test the patch, these are just observations from reading the code):

1) apps/ code MUST validate viewports to ensure they are within the LCD bounds

2) Don't forget to add the new files to apps/FILES

3) Why is there an HAVE_RTL_DRAWING #define (and why is it 0) ? Are there target where we don't want to support it, or is it just a work-in-progess?

4) I haven't tried running your patch, but you only appear to be checking the statusbar setting once - during list_init_viewports. What happens if the user enables/disables the scrollbar? I think the status bar setting is going to be a pain with viewports...

5) +#ifndef HVE_LCD_CHARCELL in gui_list_put_selection_on_screen() seems to be a typo (twice)..

6) In my WPS patch, the %V tag takes the font as a number, but your patch is parsing a text string. I think we need to decide how fonts should be specified, and be consistent. Probably a central function to parse and check the font value would be helpful.

7) There appears to be an unrelated whitespace change in apps/tree.c

8) Why not allow Greyscale targets to specify the fg/bg colours as well? I know the existing code doesn't, but don't know if that was a deliberate decision, or just that no-one has cared enough to implement it. I've included it in my implementation of the WPS %V tag, allowing "colours" in the range 0-3.

9) I'm still not convinced by the .vp file concept. Do we really want separate .vp files for every viewport that we are going to allow the user to configure? Are there even going to be any other user-configurable viewports, in which case, do we need to create a new filetype just for one configuration? It's just that I dislike the gradual addition of more and more individual config files, and where possible would like to reduce their number.

Could the configuration instead be implemented as some kind of ".theme" file with one line per viewport (e.g. of the form "list_viewport: |50|50|100|100|1|FF0000|00FFFF|" - so it can share the WPS %V tag parsing code). Also, I would have hoped that the separate components of the list widget could be themed individually - e.g. different fg/bg colours and eventually even font for the title. Having a generic ".vp" file doesn't seem to allow this kind of "screen-specific" customisation.

10) Unless I'm misreading the code, you appear to be using the same viewport structs for both the main and remote LCDs. This would mean that both the main and remote LCD scrolling lines will be linked to the same list_text viewport, which won't work... This global approach also won't allow two different lists onscreen at the same time.

Comment by Jonathan Gordon (jdgordon) - Monday, 14 January 2008, 19:25 GMT
1) shuoldnt that be checked and forced to fit in set_viewport()? saves doing it for every single viewport?
2) how could I forget that :p
3) it was something I was playing with, but its broken so untill its fixed there is no point having the code compiled
4) depends how viewports are managed. if we go the route I tried in the plugin in the viewports patch then the statusbar wont be a problem, and this patch is done with that assumption, but yeah your correct.. some extra statusbar handlgin code needs to be added (same problem with button bar... this patch assumes all lists have a BB)
5) yep, typo
6) I dont know how multifont is going to eventually work, so I felt it would be nicer for the user to be able to use a string instead of numbers, but yeah, we should be consistant
7) meh
8) I actually always thought greyscale could set colours and was confused why it wasnt working when I had it as LCD_DEPTH>1... I used _lcd_color because thats whats in settings.h.. but this can be fixed if they can do colours..
9) well, remember, in all likelyhood there wont be any way to edit the viewports except with a text editor, so I thought making it simple would be better. does the wps parser for the vp line allow missing bits? and extra items? (e.g the colour items on non colour targets..)
10) yes, I seem to have fudged the internal viewports for the remote, I'll fix that. as for using globals.. I'm trying to avoid having to pass in any viewports untill the rest of the ui is ready for it, and anyway, you want to statically allocate 6*NB_SCREENS viewports for each list in the code?
Comment by Dave Chapman (linuxstb) - Tuesday, 15 January 2008, 00:13 GMT
1) I thought about this when implementing viewports into firmware/, and IMO the apps/ code is better suited for rejecting viewports than firmware - because it can decide what to do (i.e. revert to a default amd tell the user that the file specifying the viewport is invalid).

Also, it's only the user-specified viewports that need to be checked - there shouldn't be a need to check viewports which have been calculated - if the parent viewport has been checked to be within the LCD bounds, then any child viewport you calculate to be within that viewport will also be OK.

6) Briefly, my thoughts about multifont is that the theme will be able to specify a set of fonts that are loaded, and then these are simply referred to (in the WPS, viewport definitions etc) by their number in that list, with 0 being the built-in system font, and 1..n being the user-defined set of fonts (currently n=1).


9) No, the wps parser for %V doesn't allow missing bits, although that could be added if desired (just leaving a field blank). I'm not sure what you mean by "extra items" - why would you define colour items for non-colour targets?

I want to try and rewrite that wps parsing code anyway - it's one of the reasons it's not committed yet.

Comment by Jonathan Gordon (jdgordon) - Tuesday, 15 January 2008, 01:15 GMT
just about the extra items, what about themes which are identical for a ew targets except one doesnt have colour screen or something?
Comment by Dave Chapman (linuxstb) - Tuesday, 15 January 2008, 08:37 GMT
I'm not aware of any themes that work on different LCD depths (rockbox-themes.org splits by WxHxD), but of course, if there's a requirement to do that, then it can be implemented.
Comment by Thomas Martitz (kugel.) - Friday, 18 January 2008, 15:12 GMT
Just a small question. It was desired to increase the configurablility (i.e. the scroll bar to the left of the list, varying indent of the list title, etc), wasn't it?

I think it would be great to have some more options. And to not increase the complexity for non-hardcore-themers default values could apply if only the main list viewport is specified.
Comment by Jonathan Gordon (jdgordon) - Friday, 18 January 2008, 23:50 GMT
to start off with we just want to get it with the same features as svn using viewports.. once thats done we can figure out how far we want to take it
Comment by Travis Tooke (tdtooke) - Sunday, 20 January 2008, 19:45 GMT
I think it's a pretty good starting off point, the only thing I think is really missing is more on the scrollbar and icons so it will display correctly in all cases. Not to volunteer anyone.. :-), but I know for a fact Thomas is pretty hand with that sort of thing from the old customlist patch.
Comment by Jonathan Gordon (jdgordon) - Sunday, 20 January 2008, 19:49 GMT
well seen as I never used the old patch, you better explain exactly what your missing and are requesting...
Comment by Travis Tooke (tdtooke) - Sunday, 20 January 2008, 23:53 GMT
Currently the icons display on top of the scrollbar in some lists and the scrollbar extends below the lowest icon in some cases.
Comment by David Maliniak (major_works) - Monday, 21 January 2008, 06:12 GMT
What is confusing to me is whether this patch supercedes the viewports_list patch added to FS #8385 on January 9? It'd be helpful if the author could clarify. Thanks.

Comment by Dave Chapman (linuxstb) - Monday, 21 January 2008, 19:23 GMT
Yes, the implementation of lists here is just a continuation of the patch posted in  FS#8385 , and hence supercedes it.
Comment by David Maliniak (major_works) - Monday, 21 January 2008, 20:20 GMT
Thanks for the clarification. A couple of questions, please?

In the details at the top of this page is the following:

"the lists viewport can be setup by a .vp file in /.rockbox/themes which looks like:

x:10
y:50
height:80
width:100
foreground color:aabbcc

you then need to add the line: "list viewport file: /.rockbox/themes/list.vp" and it will be loaded."

Does it matter what I name this *.vp file that I would add to /.rockbox/themes?
Where is this line, "list viewport file: etc." to be added? To the *.vp file? Or... ?

Comment by Jacob Brooks (jac0b) - Monday, 21 January 2008, 23:09 GMT
Resynced
Comment by David Maliniak (major_works) - Tuesday, 22 January 2008, 02:03 GMT
I don't mean to be a pest but can someone possibly help me with my question of earlier today? It's the latter one that's vexing me in particular. Where is this line that Jonathan speaks of supposed to be added?

I compiled a build with this patch and the menus come up unreadable. The icons are there but there is evidently blue text on an identical blue background, rendering it invisible.
Comment by Sasha Khamkov (sanusart) - Tuesday, 22 January 2008, 02:12 GMT
@major_works: The line to add in .rockbox/themes/{ThemeName}.cfg file.
but the latest patch is causing hunk failures in apps/settings_list.c therefore failing to build.
Or it's just me?
Comment by David Maliniak (major_works) - Tuesday, 22 January 2008, 02:15 GMT
Ah... thank you. So you have to add the line in each *.cfg file for each theme?

It compiled for me OK. Maybe *I* did something wrong. :-)
Comment by Sasha Khamkov (sanusart) - Tuesday, 22 January 2008, 02:19 GMT
As I understand - this works somehow like the old list_menu patch but reading the data from external source.
I might be wrong though!
Comment by Sasha Khamkov (sanusart) - Tuesday, 22 January 2008, 02:27 GMT
Oh! Tried to patch in fresh 'source' and no hunk failures appeared - I'm terribly sorry for previous post.
Comment by David Maliniak (major_works) - Tuesday, 22 January 2008, 02:31 GMT
That was it, Sasha... got it working now. Thanks so much!
Comment by Michael Hahn (disorganizer) - Monday, 28 January 2008, 15:01 GMT
could it be that this needs a resync? if possible also with versioning and a more "inuitive" filename *g*
Comment by Karl Anderson (D-Shadow) - Tuesday, 29 January 2008, 17:52 GMT
Hi, I'm using a build similar to tdtooke underground. I noticed a bug in this patch. The display becomes sloppy if I switch from WPS screen to file viewer, and even when I switch the theme. Check out the attached screen dumps.

I'd also like to make a feature request. Could you add a number of lines parameter, as in custom list position patch, and make the list height parameter optional? I'd also like the ability to specify the list parameters in the theme config file. I hate the idea of having a separate file for that.

Definitely a good start, by the way.

~ Karl
Comment by Travis Tooke (tdtooke) - Tuesday, 29 January 2008, 21:39 GMT
I don't think that's a bug. I believe what you're seeing is your list viewport being displayed as normal, but since the list viewport is not fullscreen you can still see bits of the WPS on the outside. I think what you want is probably for the main screen to clear and display your backdrop before the list viewport is displayed. I don't think that will happen since I think? that a smaller viewport displaying on top of the wps is what the devs are going for..could be wrong As far as nb lines goes, if you had the main screen change like I said I don't think that would be necessary, come to think of it it might not be a bad idea for another patch to do something like that for all us old customliners :-)
Comment by Jonathan Gordon (jdgordon) - Tuesday, 29 January 2008, 21:49 GMT
travis: PARAGRAPHS.....

Karl: yep, what Travis said about the redrawing problem is "by design"... we need to figure out who shold be responsable for clearing the unused parts of the screen... I'm inclined to get around this by not allowing the customization just yet.

and I dont think we will allow chooisng the number of lines in a viewport..
Comment by Karl Anderson (D-Shadow) - Wednesday, 30 January 2008, 04:39 GMT
@Travis: I see... and, yep, that's exactly what I'm after.

@Jonathan: Ah, okay. That's fine with me. What about the ability of specifying the list parameters in theme cfg itself? Like I said earlier, I hate having a separate file.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 30 January 2008, 07:21 GMT
finally an update...
fixed it so seperate viewports are used for the remote and main screen which should solve some problems
_should_ load the viewport config from the .cfg in a simliar way to the wps syntax. (I say should because for some reason its not working....)
for some reason the remote lcd viewport isnt using the whole screen... probaly something stupid but I cant see it in my current state.
   changes (45.5 KiB)
Comment by Jonathan Gordon (jdgordon) - Friday, 01 February 2008, 07:19 GMT
just about ready to commit now.
FIXED:
- what I thought was the remote lcd no being used fully (turns out i had a few bugs in the drawing code... should be all fixed now)
- remote inverted line was shoing even if the cursor should have been shown
- cursor icon covering 1 pixel of the item icon
- viewport not being loaded from the .cfg
- viewports not being resized when the statusbar is shown/hidden (not done nicely but good enough for now

unless any bugs show up or i get some comment ill commit this on the weekend probably. Dave, you happy with the config parsing?
   changes (46.6 KiB)
Comment by Daniel Siebrecht (DimensionSix) - Friday, 01 February 2008, 13:19 GMT
I noticed that scrolling contents aren't cleared when going to another screen.
How to reproduce:
-create a folder with a long name so that it's srolling
-insert only a few files so that they don't fill up the whole screen
-scroll to the folder so that it's at the very bottom of the screen
-select it and the selected scrolling foldername will show up in the new list, too

This also occurs if you have scrolling contents on the wps and go to the context menu.

Hope you understand this...

Another question. How can I set the list_viewport with the latest patch?
Comment by Thomas Martitz (kugel.) - Friday, 01 February 2008, 21:51 GMT
Jonathan, before you commit, I'd like to ask you if my approach would work too. I'm not sure, but I think my approach looks cleaner, and is more flexible.

It doesn't work yet, but you'll see the idea behind my approach from looking at the attached file (attached is apps/gui/bitmap/list.c).
   list.c (11.9 KiB)
Comment by Jonathan Gordon (jdgordon) - Saturday, 02 February 2008, 00:09 GMT
base it off my latest version and it will work fine.
as for cleaner/ more flexible... I hink its much of a muchness.... remember this [part of the code needs to be as fast as possible... looping over the items twice is slow (remember.. the gigabeat and viedeo can have a fair few items, and the video lcd isnt brilliantly fast.... heck even the sansa can have >20 items on the screen... any extra time drawing is wasted cpu.

also... the only viewports which actually needs to be global is the text ones (for scrolling), all the rest can be local vars which means you could neaten things up a bit by making your draw_icons and draw_cursor functions retunr the width that was used...
Comment by Travis Tooke (tdtooke) - Saturday, 02 February 2008, 00:55 GMT
Daniel: to set it you add a line in your config something like:
list viewport: 0|8|320|166|FONT_MENU|B5B5B5|2D2F2D|FFFFFF|B6C5E4|000000
It's x|y|width|height|font|fgcolor|bgcolor|lss|lse|lst. Right now for font if you put "system" you'll get FONT_SYSFIXED and FONT_UI for anything else. Looks like I need to add to the multifont patch to take advantage of these new options. I had to read the patch myself to figure that out, so don't feel bad.
Comment by Travis Tooke (tdtooke) - Saturday, 02 February 2008, 01:01 GMT
@Jonathan: Over on the multifont patch since with viewports it's looking like things like FONT_MENU/FONT_WPS are becoming irrelevant I'm thinking of taking those out and having just userfonts. Then I think I might put a switch case for the font for the list and have it take in a number instead of a string (on multifont that is, not going to meddle with what you've got going on here). What do you think?
Comment by Thomas Martitz (kugel.) - Saturday, 02 February 2008, 01:34 GMT
Jonathan, I'm not sure what you meant. Is my approach good or not? Is it as flexible as I'm expecting? Did you mean it's too slow?
Comment by Dave Chapman (linuxstb) - Saturday, 02 February 2008, 12:31 GMT
Here are some random thoughts about the patch posted at 01 February 2008, 08:19 GMT (descriptive patch names with a version number are helpful...):

1) Don't forget apps/FILES ;)

2) Why store the string (in a MAX_PATH length buffer) read from the cfg file in global_settings, rather than a "struct viewport" ? You also don't seem to be validating the viewport values (to make sure they are within the LCD bounds, or even checking the return value of viewport_load_config() at all. Also with your code, an invalid (but partially parsed viewport string) will change the passed viewport struct.

3) How should 0 for VP_ERROR in viewport_load_config() work when you | it with other values?

4) viewport_load_config needs to distinguish between the two LCDs it can load viewports for (pass it the display struct?). e.g. for a target with greyscale/colour main LCD and mono remote, it should read colours for the main LCD (in the range 0-3 for greyscale, rgb888 for colour) and not read colours for remote.

5) My preference is still for fonts to be specified as numbers, rather than strings (it keeps things simple - IMO multifont should allow a user to specify N fonts, and then Rockbox would simply use indexes into that array of fonts to refer to them, rather than having to match strings). This is how my WPS patch deals with them (0=system font, anything else=user font).

6) In an earlier comment you said your patch assumed all lists have a button bar - that looks to still be the case. gui/textarea.h uses the following expression for that:

((global_settings.buttonbar && display->has_buttonbar) ? BUTTONBAR_HEIGHT : 0)

7) Shouldn't the viewport structs defined in apps/gui/bitmap/list.c be static?

8) Is the change to apps/main.c to clear the display(s) required?
Comment by Jonathan Gordon (jdgordon) - Sunday, 03 February 2008, 02:24 GMT
Travis: I havnt looked at the multifont patch
Thomas: I cant really see much difference, I dont know how flexible your expecting it to be so cant comment.. and yeah, your way lops through every item twice (once for the icon, once for the text) so yeah, it will be "slower"... by how much is a different story.
Also, 1 thing which your way may not work so nicely with is redrawing a small section of the items. svn code has this but it is not used for some reason and its something which would be nice to put back in.

Dave:
1) yeah yeah :p
2) the setting reading/writing code needs more work to handle the viewport struct instead of just saving/loading the string, either way it has to get converted and I guess I figured it was easier to do it this way.
3) the idea of the return codes is that the user would check how much of the loaded viewport is valid, e.g if the list only cares about the dimensiions then it can still use the loade vp if the colours wernt loaded properly. the patch doesnt do this but it should.
4) arg.. yeah
5) from a users POV, I think a string is much nicer here.... what does 5 mean? how is "5" better than "courB08" or "ter-u12n"
6) yeah button bar is still going to cause problems... ill get to that
7) yep
8) yep, otherwise, if the whole screen isnt used, you get the lovely rockbox logo
Comment by Alexander Papst (DerPapst) - Sunday, 03 February 2008, 12:24 GMT
About 5: My coding skills are far below yours, but wouldn't it be a lot slower to perform all those sting operations such as strcmp instead of checking for an integer?
Comment by Thomas Martitz (kugel.) - Sunday, 03 February 2008, 13:45 GMT
Jonathan, 5 instead of a string would be consistent with the font selection a viewports wps.
Comment by Jonathan Gordon (jdgordon) - Sunday, 03 February 2008, 19:44 GMT
Alexander: that conversion is done once when its loaded so speed isnt important.
Thomas: so? the voewport ps code isnt in yet and the idea is to use a single funciton to parse viewpoertrs everywhere... I still think using the font name is much nicer than just an arbitrary number (which would then need to be mapped form a font name anyway)
Comment by Dave Chapman (linuxstb) - Monday, 04 February 2008, 06:55 GMT
Regarding the fonts, I think I'm now sold on the idea of using strings for font names in viewport declarations - mainly because that's what Rockbox does with all other settings (using sensible strings instead of numeric identifiers).

When multifont support is implemented, the font code in firmware/ will need to know the names of all the fonts anyway (and have an internal ID number), so the fontid_from_string() function would be something to implement in firmware/.
Comment by Sasha Khamkov (sanusart) - Wednesday, 06 February 2008, 13:19 GMT
@Travis Tooke - regarding this comment: http://www.rockbox.org/tracker/task/8457#comment21155
You can specify this "settings line" in <theme_name>.cfg file as well, this way the config file's line will be overwritten on every theme change, no?
Feel free to correct me if I'm wrong.
Comment by Travis Tooke (tdtooke) - Thursday, 07 February 2008, 07:13 GMT
Yes, that's what I meant to say.
Comment by greg (hweb21) - Sunday, 10 February 2008, 04:27 GMT
x|y|width|height|font|fgcolor|bgcolor|lss|lse|lst| <- need "|" at end of line
Code Works, No need for a .vp file, just put in the .ctf
Scroll Bar always on even when set to off.
Comment by Thomas Martitz (kugel.) - Sunday, 10 February 2008, 14:59 GMT
Yea, I've noticed that some time ago by looking in the code.
Comment by greg (hweb21) - Monday, 11 February 2008, 06:00 GMT
Here is an action shot if anyone wants
http://smg.photobucket.com/albums/v282/gregken/?action=view&current=dump080210-234745.png

It seems the scroll bar takes the invert colors of the background for the rim?
Comment by Bob (viperman3) - Thursday, 14 February 2008, 06:27 GMT
How is this patch used? I tried the .vp file but it's not working.
Comment by Sasha Khamkov (sanusart) - Thursday, 14 February 2008, 09:56 GMT Comment by Michael Hahn (disorganizer) - Sunday, 17 February 2008, 23:06 GMT
a question:
im using this patch together with multifont and vp-wps1.
now since i applied this patch (multifont recently need this) when going to the context menu (holding select) on my sansa during playback, the last scrolling line of the wps still stays visible (not the other lines).
could this be caused by the vw-list patch?
Comment by Michael Hahn (disorganizer) - Monday, 18 February 2008, 12:58 GMT
forget my previous comment, i noticed what went wrong.

but as i now understand the concept, nevertheless i wonder...
... if we use viewports for lists, shouldnt we also be able to let the wps update in background?
at the moment i have my wps running and replace the AA part of the screen with the list-viewport.
i would have expected that the wps still does update in the background, which does not happen.

will this in the future be possible at all? if yes then this would be a great feature, allowing me to browse for example the playlist during playback and still see the trackinfo of the song i hear and the custom time, clock and battery indicators.

if not then this is rather useless imho from the practical point of view.

btw:
i also found a "bug":
when you set your rockbox to display the statusbar and your wps to disable the statusbar, as soon as the list-viewport comes up the statusbar is drawn relative to the screen (aka on the top) and over the wps area. the behaviour should propably be that the statusbar should also use the list-viewport :-)
Comment by Michael Hahn (disorganizer) - Monday, 18 February 2008, 13:26 GMT
maybe another bug:
in the above setup (a wps in the background, the list-vp covering part of the screen)...
i have a vp with a scrolling line below the list-vp display area.
normally this vp has a dark blue background, but as soon as the context menu (and only that) pops up inside the list-vp, the background colour changes to a lighter blue.
also only when the context menu is used the line still scrolls (even though it is part of the wps).
during menu operation (for example in the file browser) the colour stays in its dark blue, and the line does not scroll (as expected due to the fact the wps is not updated during menu display).
Comment by Michael Hahn (disorganizer) - Monday, 18 February 2008, 13:50 GMT
the above "bug" again:
even when setting the list-vp to fullscreen (0|0|176|220|...) on my sansa the last vp of my wps (on the bottom line) is drawn OVER the list VP.
the vp-config of my wps of the line which shows "above" the list-vp is:
%V|34|211|140|8|2|999999|000080|
%s%Ia - %It
(theme is dis-wps-vp from ABI-forum)

and also regarding the statusbar-"bug":
when defining the list-vp fullscreen the statusbar shows "over" the list-vp text.
Comment by Michael Hahn (disorganizer) - Tuesday, 19 February 2008, 10:52 GMT
now after more observation of the "issue":
every wps-viewport containing a scrolling line does write over the list-viewport as soon as the content is scrolling (=> not fitting into the width of the viewport).
as the content of the "tags" inside the wps are not updated i can not tell whether this is just a problem with the viewports not know who is responsible for the screen area.
but imho this seems to be a bug in viewport handling, as normally the wps viewports should never be able to write "over" the list viewport.

how can we determine which viewport is "ontop" of which one? shouldnt we have a kind of "z-axis" to tell the viewport manager which viewport is "in the front" and which one is "in the back"?
with that we could also have overlapping viewports (one showing a left aligned tag and a right alligned tag, ontop of it a centered tag in another viewport) without any problem.
the z-axis could be defined as numerical bytes (0-255) with 0 being the front and 255 the back. 0 should be reserved for system use (lists, menus etc) as those normally should always be ontop of the other viewports.
Comment by Jonathan Gordon (jdgordon) - Monday, 25 February 2008, 08:34 GMT
quick resync.. needs testing
Comment by Michael Hahn (disorganizer) - Monday, 25 February 2008, 23:09 GMT
patched with 16421 fine, but does not compile
Comment by Barry Wardell (barrywardell) - Wednesday, 27 February 2008, 03:24 GMT
viewport.h needs to include screen_access.h. Fixed patch attached.
Comment by Barry Wardell (barrywardell) - Wednesday, 27 February 2008, 03:29 GMT
After a quick test of this I have noticed that the scrollbar seems to be one text line longer than the list. Not sure which one is the right height.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 27 February 2008, 05:23 GMT
arg, thanks for fixing that Barry.
My computer is hopefully somewhere over the pacific ocean atm so I cant verify that, but What I thought was happening is that the there isnt enough room at the bottom of the screen for the last line which is why the scrollbar is longer. Is there definatly enough room for one more line? Shold the scrollbar only be as high as the actual text lines?

definatly wont be an update untill the weekend at the earliest so if someone beats me to it please post a fix :)
Comment by Barry Wardell (barrywardell) - Wednesday, 27 February 2008, 10:36 GMT
Ah yes, that was the problem. My viewport was 2px too small to fit the text. I think I was expecting there to be another line and the text to be clipped, like how desktop file browsers work. Thinking about it I'm not sure which I'd prefer, but I'm leaning towards clipping. This might end up in annoying problems down near the bottom of the list though, so I'm not totally sure about that.

Another problem I have come across is when I make the viewport larger than the screen, it crashes my Sansa straight away. I suppose the Sansa lcd driver sholdn't crash Rockbox like that, but still some validation in the list size could be good too.
Comment by Barry Wardell (barrywardell) - Wednesday, 27 February 2008, 11:26 GMT
My last patch forgot to add the new files that were in V!. Attached is an updated, working version.
Comment by Barry Wardell (barrywardell) - Wednesday, 27 February 2008, 11:35 GMT
Sorry for the third post in a row...The Sansa LCD driver clips properly, so the crash with viewports larger than the LCD must be elsewhere. Testing on H10 gave the same result.
Comment by Michael Hahn (disorganizer) - Wednesday, 27 February 2008, 16:16 GMT
maybe 2 dumb questions:
what is the setting "lst" for?
how can i change FONT_UI to another font?
Comment by Michael Hahn (disorganizer) - Wednesday, 27 February 2008, 19:03 GMT
just a small comment:
it seems you only use fontheight for calculating the line height, but imho you should use the larger value of font height and icon height (if icons are higher than fonts the line height should use the icon height instead).

i noticed this because sysfont is so small and my icons are so huge :-)
Comment by Michael Hahn (disorganizer) - Wednesday, 27 February 2008, 23:07 GMT
small "bug" discovered:
the newer multifont patches do set font_ui to font_sysfixed and not allow you to set font_ui any more.
instead they use font_menu
this is bad together with your patch, as you only distinguish between font_sysfixed and font_ui, which leads to only font_sysfixed being chosen :-)

i attached a small modification which used font_menu instead of font_ui as default and for the "any other" setting.

propably a better way to achieve this would be to use font_ui as default and modify the if/then clause for "system" to also allow "menu" as font setting. propably you should nevertheless use the same naming as in the multifont patches?

WARNING:
the file attached above is only simulator-tested! it is not perfect but just an example! use with caution!
Comment by Michael Hahn (disorganizer) - Wednesday, 27 February 2008, 23:35 GMT
another "bug":
you seem to use "viewportheight - titlelineheight" to determine the length of the scrollbar.
imho you should use "linestodisplay*lineheight" as length for the scrollbar.

reason:
at the moment if the viewport can not completely be filled with the chosen font (a "halfline" says empty on the bottom), the scrollbar still uses the complete viewport height instead of ending with the bottom of the last line.

also the screenlimits are not validated. you can define a viewport which is bigger than the screen, resulting in garbage on the screen (in case of the width) or lines being written "below the screen" (in case of the height).
imho values outside the screen should be cut down to the screen's width and height.
to illustrate what i mean (in simple code without c knowledge):
vp.max-width=lcd.widht-vp.x
vp.max-height=lcd.height-vp.y
if vp.width>vp.max-width then vp.width=vp.max-width
if vp.height>vp.max-height then vp.height=vp.max-height
i hope you get what i mean :-)
Comment by Michael Hahn (disorganizer) - Wednesday, 27 February 2008, 23:37 GMT
sorry for 3 posts in a row :-)
but is it technically possible to let the wps-viewports update in the background when displaying a list? this would really enable cute wps's where you can modify the playlist during playback without missing anything happening on the screen :-)
(for example by letting the list be on top of the aa and still displaying the other parts of the wps)
Comment by Travis Tooke (tdtooke) - Thursday, 28 February 2008, 07:07 GMT
The multifont patch has always replaced FONT_UI with the various fonts: menu/wps/recorder/etc.. That change you put selecting FONT_MENU for any other is already a part of the multifont patch. And you can use FONT_UI as you always have if you have multifont applied. It is backwards compatible, for .cfgs that just have a FONT_UI it will set all the fonts menu/wps/etc.. to whatever you have specified as your font.
Comment by Michael Hahn (disorganizer) - Thursday, 28 February 2008, 10:42 GMT
the problem here was:
the multifont patch only lets you set menufont etc from the menu. it does set font_ui to sys_fixed and not to the font you select from the menu (which is good imho to discourage use of font_ui in configs).
thus as the list vp patch uses only font_ui, you can not set the font at all any more after applying mf and vp-list3 (the original one, not the patched one).
resulting in the vp-list patch only using sys_fixed as font, no matter what you set in the config.

my patch above only change the vp-list usage of font_ui to font_menu (which it should be as this is meant to define the menu=list font). i do though not know whether this breaks compatibility with non-mf versions.

the better way to handle this for the vp-list patch would propably be to allow the usage of "font_menu", "font_ui", etc in the vp-config and use font_ui as default.
but anyways there should be some consensus on which font-tags to use for all patches. or we should hurry up to commit the multifont patch to prevent furure "incompatibilities" *g*

Comment by Michael Hahn (disorganizer) - Thursday, 28 February 2008, 15:43 GMT
while looking at the code:
wouldnt it make sense to have a global function converting font-names like in the config (system, menu, user1, ...) into font-numbers (0,1,2,...) and returning the appropiate font-value for the vp?
this could be used by both patches.
something like "vp.font = globalfunctiontodeterminevalue( stringidentifyer )".
maybe the default font for the whole system can also be delivered by this function (...("") could return font_ui by default). this eliminates the need to patch multiple places if you want to change the default font.

this would not only allow users to use a identical way of font-indication for wps and list, but also would allow list and wps vp to use the same functions and thus save space and lines.
we also should not cross patch between mf-patch and vp-list, even though vp-list still uses font_ui as default and mf wants to change this to font_menu because if we "cross patch" patches we will need to keep both patches in sync with each other :-)
Comment by David Maliniak (major_works) - Thursday, 28 February 2008, 20:43 GMT
Back on 2 Feb, tdtooke posted:
To set (viewports_list) you add a line in your config something like:
list viewport: 0|8|320|166|FONT_MENU|B5B5B5|2D2F2D|FFFFFF|B6C5E4|000000
It's x|y|width|height|font|fgcolor|bgcolor|lss|lse|lst.

This actually works... which is cool... but...

What are lss, lse, and lst? Line selector colors? I'm not sure what these parameters are affecting.
Comment by David Maliniak (major_works) - Thursday, 28 February 2008, 20:48 GMT
Please disregard the previous comment. Figured it out.
Comment by Travis Tooke (tdtooke) - Friday, 29 February 2008, 01:56 GMT
Uh...disorganizer, I'm posting this hunk directly from the last multifont patch I put up:

diff -u -r rockbox_svn.orig/apps/gui/viewport.c rockbox_svn/apps/gui/viewport.c
--- rockbox_svn.orig/apps/gui/viewport.c 2008-02-07 05:12:48.000000000 -0600
+++ rockbox_svn/apps/gui/viewport.c 2008-02-07 05:09:32.000000000 -0600
@@ -45,7 +45,7 @@
if (!strcmp(string, "system"))
return FONT_SYSFIXED;
else
- return FONT_UI;
+ return FONT_MENU;
}

What you're doing is already in the multifont patch. I know because I put it there. After you apply viewports-list as it was when you apply multifont it takes care of that for you. I had intended to replace that in time with some string comparison code that would load a named font as jdgordon intended (for the list anyway, still mulling it over for userfonts). If you want to do some rearranging on multifont that's cool, I just don't think you should be doing font things on this patch unless the devs are ok with it. If they are, go for it :)
Comment by Jonathan Gordon (jdgordon) - Friday, 29 February 2008, 03:57 GMT
> I just don't think you should be doing font things on this patch unless the devs are ok with it.

I arnt... Enough is enough witht he multifont patch... other patches are like unsupported builds... stop mentinogin them in this thread. the multifont patch wont get commited before this one so relax...
Comment by Michael Hahn (disorganizer) - Friday, 29 February 2008, 06:56 GMT
ok. last mentioning of mf here:
as of the discussion in the mf-patch fs-entry it would propably be better to use font_ui instead of font_menu in the mf patch. this would solve the problems with list-vp without modifying it.
can someone remove my obove patch because its obsolete? (listvp.3-fontmenu.diff) tnx.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 05 March 2008, 03:55 GMT
updated. new plan....
I've #ifdefed out the custom list code in an effort to get this commited... once its in (with that code ifdefed out) it will be easier for other patches to be reworked and commited maybe. let me know nay drawing bugs you see
Comment by Jonathan Gordon (jdgordon) - Wednesday, 05 March 2008, 06:22 GMT
make it work with the show icons setting
Comment by Jonathan Gordon (jdgordon) - Wednesday, 05 March 2008, 08:40 GMT
ok, about ready to commit now...
listvp.6.diff is what will be commited.
list_w_custom is the same but with the extra code needed for the custom vp's which is not used yet.
_w_rtl is above but also with the rtl drawing logic

Loading...