Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Bugs
  • 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 glued - 2011-08-20
Last edited by jdgordon - 2011-12-14

FS#12237 - WPS/FMS string space bug introduced by r30302

An extra space seems to be added to strings, which makes %ac & %ar to render at incorrect positions. At the attached screenshot a 35pt font was used to demonstrate it more clearly.

Closed by  jdgordon
2011-12-14 09:50
Reason for closing:  Fixed
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

r31246

glued commented on 2011-08-21 00:14

The problem seems to be not in the r30302 itself, but in the switch from putsxy() to puts_style_xyoffset().
I don’t get why the latter uses the space char width to calculate the x offset. Same with the space_width var in skin_display.c, to be honest.
Is it from the old fixed-width-only days? There must be a less square way to calculate the x pos.

yeah, that’s pretty much what I guessed was the issue.
There are a few ways to possibly fix this, the best would be cleaning up the puts() API so there is a pixel based putsxy which allows us to set a style, otherwise possibly anotherfunction to just set the draw style.. dunno.

glued commented on 2011-08-21 02:20

Cleaning? Yeah. I went there with a broom, and they took it, along with my socks. Unless you bend spoons, I suggest to go with a new style func + old putsxy.

give this patch a try… takes a hacksaw to the lcd api (still not enough imo)

glued commented on 2011-08-21 11:38

Perfect positioning.
Tried various stuff to be sure, all ok.

thanks for testing. dunno how long till its in svn.

Testing on r30334 with the above patch on sansa e200v1 and default settings:

on rolo, I see some extraneous characters. It looks like this: ( s/-/ / )

ROLO… Loading
———————–ocessor…

The “o” is not complete, but is cut-off - only the right half of the character is displayed. The cut-off “o” appears to start at approximately mid-screen.

In firmware/rolo.c - line 259 “Waiting for coprocessor…” is written to the screen. Line 264 writes an equal number of space characters to the same line to clear what was written. Apparently, this will no longer work if the space character is not the same width as the text characters.

Clearing a line by writing spaces is just broken :) In particular with a proportional font.

indeed. I dont understand why M isnt used which is generallly accepted as the widest ascii charachter. Of course, we should be able to just clear a full line simply also.

glued commented on 2011-08-31 13:35

Looks like spaces are used only in rolo.c.
Maybe switch there to “M” / clear full line and commit this?
The patch not only fixes a bug, but brings sanity to lcd.

yeah, need some motivation to finish this off though… feel like finishing it? :)

glued commented on 2011-08-31 19:27

After you’ve spend a night cleaned up one of ugliest code parts I’ve seen in rockbox so far… cursing on irc like a drunk sailor?
I’m afraid nobody will submit the rolo patch after that :) because such a tiny addition to your work looks pity.

I mean we all have some skill to replace " " with “M”. Some (maybe) can even just clear it up. But the history should remember you alone.
And if you spend just 19 seconds more, it will.
Fame, money, easy chicks – I don’t understand how all that can’t motivate you.

like you said, when I spend half the night arguing in IRC over bullshit I dont have very much motivation to work outside of things I persoanlly find interesting. cleaning up old API‘s not in the things that I could consider being interesting :)
fixing rolo wont be an issue but even with that done this isnt quite finished so not commitable yet

glued commented on 2011-09-01 08:23

Yeah, I knew it’s couldn’t be the tiny rolo tweak, it’s the perfectionism… Now it’s a weird situation:
1) you’ve fixed the bug, a rather noticeable one for themes with centered 14+ pt font.
2) you’ve made the api much more sane. not 100%, but dear god, it at least makes sense now.
3) yet some parts will benefit from more cleanup.

To me it looks like this work is worth to be committed with a comment that it needs more cleanup.
If you don’t commit it, a dev with both the skill and interest might simply never come, and the bug will live on.
I honestly tried cleaning the api before you, and I couldn’t do even the half of your work.

Not to mention your moral obligations to people of earth – while r30302 didn’t create the bug, it did revealed it. Would be a nice manner to kill it… You said you can’t work if uninterested. Well, don’t anymore. Your patch doesn’t solve world hunger, it fixes a bug.

Please click on the screenshot again. If you think this is how Rockbox should look like, don’t commit your work, ok.

you’re lucky I’ve got a sense of humour…. that patch breaks charcell so regardless it isnt ready for commit

glued commented on 2011-09-01 21:10

What about reverting r30302 and committing it back when this is fixed?

nickp commented on 2011-11-12 22:52

Not realising this bug was causing the alignment problem, I created a test wps and reported it.

In case it’s useful for debugging, I’ve attached it here.

BTW, is this down as a bug that needs fixing before 3.10 is released? If not, I think it’s enough of a regression to deserve it.

no, this bug doesnt need fixing for 3.10, it appears you two are the only people to notice the change and it isnt a trivial fix. Zagor has taken over the lcd api rework and will hopefully fix this bug soonish

nickp commented on 2011-11-12 23:31

Ah, ok. I think plenty of users will notice it when 3.10 is released and existing themes start looking odd though.

Give this a try

nickp commented on 2011-12-12 00:07

I applied changes.diff to 3.10 & trunk for clip+ & e200 sims, everything looks as it should with the aligntest I posted (clip) & a couple of themes I wrote that make heavy use of centre.

What about general themes which use left, center and right in various combos?

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing