Rockbox

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

Quick links: Bugs · Patches · Rockbox frontpage

Tasklist

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

Attached to Project: Rockbox
Opened by Glued (glued) - Saturday, 20 August 2011, 15:36 GMT+2
Last edited by Jonathan Gordon (jdgordon) - Wednesday, 14 December 2011, 10:50 GMT+2
Task Type Bugs
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

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.
   r30302-space-bug.png (21.2 KiB)
This task depends upon

Closed by  Jonathan Gordon (jdgordon)
Wednesday, 14 December 2011, 10:50 GMT+2
Reason for closing:  Fixed
Additional comments about closing:  r31246
Comment by Glued (glued) - Sunday, 21 August 2011, 02:14 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Sunday, 21 August 2011, 02:53 GMT+2
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.
Comment by Glued (glued) - Sunday, 21 August 2011, 04:20 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Sunday, 21 August 2011, 09:21 GMT+2
give this patch a try... takes a hacksaw to the lcd api (still not enough imo)
   lcdapi.diff (13.5 KiB) 
 b/apps/gui/bitmap/list.c               |    4 +-
 b/apps/gui/skin_engine/skin_display.c  |    7 ++---
 b/apps/plugin.h                        |    4 +-
 b/apps/screen_access.c                 |   12 +-------
 b/apps/screen_access.h                 |    9 ------
 b/firmware/drivers/lcd-bitmap-common.c |   45 ++++++++++-----------------------
 b/firmware/export/lcd-remote.h         |   11 --------
 b/firmware/export/lcd.h                |    9 ------
 8 files changed, 27 insertions(+), 74 deletions(-)

Comment by Glued (glued) - Sunday, 21 August 2011, 13:38 GMT+2
Perfect positioning.
Tried various stuff to be sure, all ok.
Comment by Jonathan Gordon (jdgordon) - Sunday, 21 August 2011, 13:54 GMT+2
thanks for testing. dunno how long till its in svn.
Comment by Michael Chicoine (mc2739) - Sunday, 21 August 2011, 21:07 GMT+2
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.
Comment by Thomas Martitz (kugel.) - Monday, 22 August 2011, 15:18 GMT+2
Clearing a line by writing spaces is just broken :) In particular with a proportional font.
Comment by Jonathan Gordon (jdgordon) - Monday, 22 August 2011, 15:20 GMT+2
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.
Comment by Glued (glued) - Wednesday, 31 August 2011, 15:35 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Wednesday, 31 August 2011, 15:45 GMT+2
yeah, need some motivation to finish this off though... feel like finishing it? :)
Comment by Glued (glued) - Wednesday, 31 August 2011, 21:27 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 01 September 2011, 07:48 GMT+2
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
Comment by Glued (glued) - Thursday, 01 September 2011, 10:23 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Thursday, 01 September 2011, 11:05 GMT+2
you're lucky I've got a sense of humour.... that patch breaks charcell so regardless it isnt ready for commit
Comment by Glued (glued) - Thursday, 01 September 2011, 23:10 GMT+2
What about reverting r30302 and committing it back when this is fixed?
Comment by Nick Peskett (nickp) - Saturday, 12 November 2011, 23:52 GMT+2
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.
   aligntest.zip (0.9 KiB)
Comment by Jonathan Gordon (jdgordon) - Sunday, 13 November 2011, 00:15 GMT+2
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
Comment by Nick Peskett (nickp) - Sunday, 13 November 2011, 00:31 GMT+2
Ah, ok. I think plenty of users will notice it when 3.10 is released and existing themes start looking odd though.
Comment by Jonathan Gordon (jdgordon) - Sunday, 11 December 2011, 14:43 GMT+2
Give this a try
   changes.diff (6.9 KiB) 
 b/apps/gui/skin_engine/skin_display.c  |   32 +++++++++++++++++---------------
 b/firmware/drivers/lcd-bitmap-common.c |   12 ++++++++++--
 b/firmware/export/lcd.h                |    5 +++++
 3 files changed, 32 insertions(+), 17 deletions(-)

Comment by Nick Peskett (nickp) - Monday, 12 December 2011, 01:07 GMT+2
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.
Comment by Jonathan Gordon (jdgordon) - Monday, 12 December 2011, 01:11 GMT+2
What about general themes which use left, center and right in various combos?

Loading...