Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category User Interface → Themes
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Version 3.1
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by fml2 - 2009-02-13
Last edited by kugel. - 2009-02-16

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

This is a fix for the bug reported in  FS#9894 

Closed by  kugel.
2009-02-16 08:37
Reason for closing:  Accepted
Additional comments about closing:  

Committed slightly modified in r20015, thanks you!

Slightly different, basically your patch but with other parameters for draw_progressbar and no +1 in line_num (what was that for?).

fml2 commented on 2009-02-15 19:35

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.

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.

fml2 commented on 2009-02-15 20:45

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

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.

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

Available keyboard shortcuts

Tasklist

Task Details

Task Editing