• Status Closed
  • Percent Complete
  • Task Type Patches
  • Category User Interface
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Daily build (which?)
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by kugel. - 2008-08-02
Last edited by jdgordon - 2008-10-05

FS#9231 - viewports for pitchscreen

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.

Closed by  jdgordon
2008-10-05 13:02
Reason for closing:  Accepted
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407


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).


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.

Take this one, small comment cleanup.

Again a minor typo fixed.

Minor bugfix.

I tested the patch with my custom list vp patch( FS#8799 ). Seems to work well :)

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

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! :)

Sigh, this one should actually compile.

Haha, another one. I could slap myself…minor bug fix (which I apparently already fixed, but it somehow got in again).

Ok, update:
-gui_syncpitchscreen_run accepts a parent viewport
-pitch icons viewport dropped, use parent directly instead
-move #defines into the .c again

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

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.

Ahh well, the optimizing for small screens, and the statusbar in pitchscreen should be considered as bug fixes imo!

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

- 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.

: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…

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.

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?

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

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.


Available keyboard shortcuts


Task Details

Task Editing