Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

FS#9904 - Fix for FS#9894 - Position of the progress bar is not updated after the font is changed

Attached to Project: Rockbox
Opened by Alexander Levin (fml2) - Friday, 13 February 2009, 20:45 GMT+2
Last edited by Thomas Martitz (kugel.) - Monday, 16 February 2009, 09:37 GMT+2
Task Type Patches
Category Themes
Status Closed
Assigned To No-one
Player Type All players
Severity Low
Priority Normal
Reported Version Version 3.1
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Private No

Details

This is a fix for the bug reported in  FS#9894 
   pb_pos_after_font_change.patch (7.3 KiB) 
 apps/gui/gwps-common.c |   54 +++++++++++++++++++++++++++++--------------------
 apps/gui/gwps.h        |    2 -
 apps/gui/wps_parser.c  |    8 +++----
 3 files changed, 38 insertions(+), 26 deletions(-)

This task depends upon

Closed by  Thomas Martitz (kugel.)
Monday, 16 February 2009, 09:37 GMT+2
Reason for closing:  Accepted
Additional comments about closing:  Committed slightly modified in r20015, thanks you!
Comment by Thomas Martitz (kugel.) - Sunday, 15 February 2009, 20:20 GMT+2
Slightly different, basically your patch but with other parameters for draw_progressbar and no +1 in line_num (what was that for?).
   pb_fix.diff (4.2 KiB) 
 apps/gui/gwps-common.c |   18 ++++++++++++------
 apps/gui/wps_parser.c  |   12 ++++++------
 apps/gui/gwps.h        |    2 +-
 3 files changed, 19 insertions(+), 13 deletions(-)

Comment by Alexander Levin (fml2) - Sunday, 15 February 2009, 20:35 GMT+2
The negated line number must be 1-based (not 0-based) to clearly distinguish the case when it was not set. The comment about it was relevant IMHO. Hence, when computing the Y-pos of the progress bar we have to subtract 1. You patch does not add the necessary delta for the Y. The progress bar should be drawn in the middle of the line, not at the top or bottom.

But if the information about the font is present in the wps viewport it can be used as the parameter as well, it would be even better.

The local variable I used for vieports[v] should make binsize smaller. But I agree that it could/should be a separate patch.
Comment by Thomas Martitz (kugel.) - Sunday, 15 February 2009, 20:37 GMT+2
Why does it have to be 1-based? if it's 0, it doesn't matter if y was set or not.

Right, I forget about the draw-in-the-middle-of-the-line thing.
Comment by Alexander Levin (fml2) - Sunday, 15 February 2009, 21:45 GMT+2
I'd still prefer to preserve the information about whether it was explicitly set or not as long as possible. I even wanted to introduce a special flag for that but then went the "dirty" way by using a special value range. Effectively 0 would give the same result, yes. But to have this info is cleaner IMO (for possible changes in the future).

Regarding the parameters of draw_progressbar: if we pass the wps_viewport, we don't need to pass the progress bar explicitly since it's contained in the viewport struct. Thus the function gets the meaning "draw the viewport's progressbar given the specified gwps state".
Comment by Thomas Martitz (kugel.) - Sunday, 15 February 2009, 22:09 GMT+2
Yes, and I removed the progressbar from the parameter list (if you didn't notice).

>> "I'd still prefer to preserve the information about whether it was explicitly set or not as long as possible. I even wanted to introduce a special flag for that but then went the "dirty" way by using a special value range.
>> Effectively 0 would give the same result, yes. But to have this info is cleaner IMO (for possible changes in the future)."

a) We don't code for the future. Nobody can tell what will happen in the future, so we do not prepare the code for it. Especially not, if there's not even the slightest idea of "what might change".
b) I don't consider more complexity as cleaner. It does what we need, not more, with minimal complexity. That is what I consider as clean.
Comment by Thomas Martitz (kugel.) - Monday, 16 February 2009, 08:58 GMT+2
Ok, I thought about it again, and it makes a difference if it's 0 due to specific setting y or due to line number, since in the latter case we want to center the progress bar in the line.

I'm going to commit your version, but with my parameter change.

Loading...