Rockbox

This is the bug/patch tracker for Rockbox. Click here for more information.

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9231 - viewports for pitchscreen

Attached to Project: Rockbox
Opened by Thomas Martitz (kugel.) - Saturday, 02 August 2008, 04:12 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Sunday, 05 October 2008, 15:02 GMT+2
Task Type Patches
Category User Interface
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This is my work-in-progress patch for viewport'ifying the pitchscreen.

The patch already works pretty well, but there's still some glitches.
- the statusbar doesn't show up in the pitchscreen even if it's enabled in the settings and the wps shows it as well (I think there's enough space in the pitchscreen for the statusbar
- the statusbar still has an influence, since the parent viewport's size depends on the statusbar (due to viewport_set_defaults). The weird thing is: The icons move up a little but if the statusbar gets disabled (since the parent is a bit taller), which is fine. The text though seems to move down actually, even though I used parent->height / 2.

The icons are drawn in a single vp (which is basically a copy of the parent), I think this is fine since the icons are at the borders, but one could consider having 1 vp for each icon, feel free to discuss.

Attached are 2 dumps, 1 shows the pitchscreen with statusbar enabled in the settings, the other without.

BTW: Bin size increase is about 500 bytes.
   pitchscreen_vp_v1.patch (9.5 KiB) 
 apps/gui/pitchscreen.c |  137 ++++++++++++++++++++++++++++++++++++++++++-------
 apps/gui/pitchscreen.h |   36 ++++++++++++
 2 files changed, 155 insertions(+), 18 deletions(-)

   statusbar_disabled.bmp (75.7 KiB)
   statusbar_enabled.bmp (75.7 KiB)
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Sunday, 05 October 2008, 15:02 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  cheers
Comment by Thomas Martitz (kugel.) - Saturday, 02 August 2008, 04:33 GMT+2
Type alarm: "- the statusbar doesn't show up in the quickscreen even if it's enabled[...]"
I meant pitchscreen of course (would be nice if someone with flyspray powers could correct his).
Comment by Thomas Martitz (kugel.) - Saturday, 02 August 2008, 18:07 GMT+2
Update:

I've fixed the 2 glitches from above, alignement is correct. Statusbar is shown (new feature!)
I also removed the global viewports, and put the middle (-2%, pitch, +2%) into one vp.
I've optimized the pitchscreen for smaller screens a bit more (e.g. for h300 remotes).
I removed some debug code, since I believe this is a first commit candiate.

There's just 1 glitch: The statusbar flickers upon changing values in pitchscreen. I don't know how to fix it yet.

Binsize delta is +304 bytes for e200.
   pitchscreen_vp_v2.patch (10.9 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  161 ++++++++++++++++++++++++++++++++-----------------
 apps/gui/pitchscreen.h |   46 ++++++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 158 insertions(+), 55 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 02 August 2008, 18:08 GMT+2
Take this one, small comment cleanup.
   pitchscreen_vp_v2b.patch (10.8 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  161 ++++++++++++++++++++++++++++++++-----------------
 apps/gui/pitchscreen.h |   37 +++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 149 insertions(+), 55 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 02 August 2008, 18:29 GMT+2
Again a minor typo fixed.
   pitchscreen_vp_v2c.patch (10.8 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  161 ++++++++++++++++++++++++++++++++-----------------
 apps/gui/pitchscreen.h |   37 +++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 149 insertions(+), 55 deletions(-)

Comment by Thomas Martitz (kugel.) - Sunday, 03 August 2008, 02:23 GMT+2
Minor bugfix.

I tested the patch with my custom list vp patch( FS#8799 ). Seems to work well :)
   pitchscreen_vp_v2d.patch (10.8 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  161 ++++++++++++++++++++++++++++++++-----------------
 apps/gui/pitchscreen.h |   37 +++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 149 insertions(+), 55 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Sunday, 03 August 2008, 08:17 GMT+2
move the viewport setup code out of the draw funciton and into the gui_pitchscreen_run() func so its only setup once.. not once every draw. that _may_ fix the statusbar flickering
Comment by Thomas Martitz (kugel.) - Monday, 04 August 2008, 00:15 GMT+2
So, probably my last update.

I've fixed the statusbar issue. Also, the viewports are only initialized once per opening the pitchscreen. Plus, I further optimized it for small and even smaller screens.

Don't forget the new feature: statusbar in pitchscreen! :)
   pitchscreen_vp_v3.diff (11.9 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  175 +++++++++++++++++++++++++++++++++----------------
 apps/gui/pitchscreen.h |   49 +++++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 173 insertions(+), 57 deletions(-)

Comment by Thomas Martitz (kugel.) - Monday, 04 August 2008, 01:47 GMT+2
Sigh, this one should actually compile.
   pitchscreen_vp_v3b.diff (11.6 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  172 +++++++++++++++++++++++++++++++++----------------
 apps/gui/pitchscreen.h |   49 +++++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 170 insertions(+), 57 deletions(-)

Comment by Thomas Martitz (kugel.) - Monday, 04 August 2008, 02:08 GMT+2
Haha, another one. I could slap myself...minor bug fix (which I apparently already fixed, but it somehow got in again).
   pitchscreen_vp_v3c.diff (11.6 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  172 +++++++++++++++++++++++++++++++++----------------
 apps/gui/pitchscreen.h |   49 +++++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 170 insertions(+), 57 deletions(-)

Comment by Thomas Martitz (kugel.) - Monday, 04 August 2008, 04:45 GMT+2
Ok, update:
-gui_syncpitchscreen_run accepts a parent viewport
-pitch icons viewport dropped, use parent directly instead
-move #defines into the .c again
   pitchscreen_vp_v4.patch (11.7 KiB) 
 apps/onplay.c          |    5 -
 apps/gui/pitchscreen.c |  172 ++++++++++++++++++++++++++++++++++---------------
 apps/gui/pitchscreen.h |   46 +++++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 171 insertions(+), 55 deletions(-)

Comment by Thomas Martitz (kugel.) - Saturday, 16 August 2008, 01:26 GMT+2
sync
   pitchscreen_vp_v4b.patch (11.8 KiB) 
 apps/onplay.c          |    5 -
 apps/gui/pitchscreen.c |  172 ++++++++++++++++++++++++++++++++++---------------
 apps/gui/pitchscreen.h |   46 +++++++++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 171 insertions(+), 55 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Saturday, 16 August 2008, 10:52 GMT+2
bugger... you forgot to hassle me about this so it might have to wait till after the freeze...
I'll have a look at the patch sometime toniught or tomorow though to make sure its good to go as soon as its allowed in
Comment by Thomas Martitz (kugel.) - Saturday, 16 August 2008, 14:04 GMT+2
I wouldn't consider this as a new feature. Pitchscreen exists for a long time, and so does viewports ;) Anyway, there's also no need to rush to commit this, as pitchscreen is bug free (afaik) and this patch doesn't really add new functionality.
Comment by Thomas Martitz (kugel.) - Saturday, 16 August 2008, 14:05 GMT+2
Ahh well, the optimizing for small screens, and the statusbar in pitchscreen should be considered as bug fixes imo!
Comment by Jonathan Gordon (jdgordon) - Sunday, 17 August 2008, 15:22 GMT+2
yeah ok, had another look... it needs to handle narrower screen/font combos better. It should only show +-2% if they wont be overlapped by the pitch value
Comment by Thomas Martitz (kugel.) - Monday, 18 August 2008, 02:08 GMT+2
- further optimizations for small screens
- fixed a glitch
- made the statusbar actually update.
- moved enums into the c file

And finally, revert to .diff file extension again! ;)

Testing on h300 sim and e200 target+sim didn't reveal any bugs. I've also tried giant fonts. Please commit :)

Bin size delta is about +400bytes.
   pitchscreen_vp_v5.diff (12.8 KiB) 
 apps/onplay.c          |    5 -
 apps/gui/pitchscreen.c |  207 +++++++++++++++++++++++++++++++++++--------------
 apps/gui/pitchscreen.h |   30 +++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 184 insertions(+), 61 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Sunday, 24 August 2008, 08:31 GMT+2
:D your gonna start wishing you could kill me about now... :D
pitchscreen_draw() doesnt really need the parent viewport, I see your using it only to draw the icons and to check the lines count, I think that should be changed. Also, struct viewport pitch_viewports[NB_SCREENS][PITCH_ITEM_COUNT] should be just struct viewport pitch_viewports[PITCH_ITEM_COUNT] seen as its in the FOR_NB_SCREENS() loop.

I've just had a look at the quickscreen code which I know you based it off and I'm going to do a bit of a clean up there also.
viweported screens (except WPS ones obviously) should have a fix_viewports(*parent, *out_viewports) function, and a draw(*viewports) which shouldnt care about the parent at all...
Comment by Thomas Martitz (kugel.) - Sunday, 24 August 2008, 20:20 GMT+2
I don't want to harm anyone, not even you ;)

Although I don't really agree to the parent stuff. I don't see why passing a parent to the draw function is bad.

In this case, the draw functions needs to know at least the lines (the icon draw could be done in the draw function or called by _run() directly [independently of _draw()]; the first one will make the optimization for small screens more complex) in order to optimize the pitchscreen for small screens. So I either pass the line number or a parent vp. A parent seems more logical to me, and could be useful in the future.
But if you don't like passing a parent anymore...

We need the [NB_SCREENS] in pitch_viewports[NB_SCREENS][PITCH_ITEM_COUNT], since this array is initialized once at the pitchscreen call, and not at every draw. This means the array needs to be accessed per screen at redraw. Omitting the [NB_SCREENS] will make the remote's viewports override the normal screen's viewports. But yea, at least for _draw() the NB_SCREENS can be ommitted (i.e. passing pitch_viewports(i) to _draw() ).

Anyway, I'll just prepare a version which pleases you.
Comment by Thomas Martitz (kugel.) - Monday, 25 August 2008, 03:24 GMT+2
There you go. I've also done a bit more code cleanup. gui_syncpitchscreen_run doesn't accept a viewport anymore, that's what you wanted, wasn't it?
   pitchscreen_vp_v6.diff (12.2 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  208 ++++++++++++++++++++++++++++++++++---------------
 apps/gui/pitchscreen.h |   30 +++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 182 insertions(+), 62 deletions(-)

Comment by Thomas Martitz (kugel.) - Monday, 29 September 2008, 01:53 GMT+2
Sync
   pitchscreen_vp_v7.diff (12.1 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  204 ++++++++++++++++++++++++++++++++++---------------
 apps/gui/pitchscreen.h |   30 +++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 178 insertions(+), 62 deletions(-)

Comment by Jonathan Gordon (jdgordon) - Sunday, 05 October 2008, 06:21 GMT+2
OK, i'm happy to commit v7 but going to wait till tonight when people are online so we can get a bit of discussion happening first....
I have a few points...

1) iirc its better to keep the function parameter list to <= 4 and pitchscreen_draw() has 5, one of which is the max_lines which I cant see any reason to not just call viewport_get_nb_lines() inside instead of passing it in?
2) parent in gui_pitchscreen_run() i'm not sure is needed.
3) pitch_mode is global... either remove the global var, or stop passing it around between functions.


other than that, I think its fine
Comment by Thomas Martitz (kugel.) - Sunday, 05 October 2008, 14:35 GMT+2
1) max_lines is used so that we only need to call viewport_get_nb_lines() at every redraw, since that will not change during redrawing.

2) For parent: That IMHO the whole idea of the conversion. To be able to specify a parent and get the pitchscreen drawn into it (e.g. my custom list vp patch will use this). So why "not needed" now?

3) Ok, removed the global'ity. If global is preferable, we could also make pitch global as well.
   pitchscreen_vp_v8.diff (12.2 KiB) 
 apps/onplay.c          |    3 
 apps/gui/pitchscreen.c |  206 ++++++++++++++++++++++++++++++++++---------------
 apps/gui/pitchscreen.h |   30 +++++++
 apps/gui/gwps.c        |    3 
 4 files changed, 178 insertions(+), 64 deletions(-)

Loading...