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
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by paprica - 2006-01-28
Last edited by Llorean - 2007-11-14

FS#2954 - Scrolling text margin for the wps

The patch adds the possibility to restrict the width of a (scrolling) line.
It enables you to position the scroll text inside the line and give him a
starting and ending position.
It extents the %s WPS tag to the following:
%s%m|x1|x2|
where x1 is the x-position where the line text starts and x2 is the
ending x-position of the scroll text (actually, when I look at the
code, it should also work for nonscrolling text…)
Only one margin is allowed per line.

Thanks Massa for the description

Closed by  Llorean
2007-11-14 20:02
Reason for closing:  Rejected
Additional comments about closing:  

The desired parts of this task have been accepted, and the remainder is rejected, to be done via viewports instead.

Any patch that seeks acceptance should work in this direction and be a new task.

i found a bug

dont download it, i will upload new version in the noon

the bug fixed and now you can use also %ar & %ac

if you want a regular margin after a irregular one,
just use %m without nothing

work with the archos recorder too

Anonymous Submitter commented on 2006-02-01 19:41

iuoçiuç

version 2, work with all the targets… CVS 2006-02-06

buggy, dont download.

tfact commented on 2006-02-07 01:11

old version for CVS 0207
http://takka.style.coocan.jp/wiki/?
plugin=attach&pcmd=open&file=scroll-
margins.patch&refer=RockBox

i synched this patch with the latest CVS (17.02.06). get it
here :
http://nicolas.pennequin.free.fr/scroll_margins.patch

nicolas_p's version doesn't compile.
neither does Paprica's orginal.

Massa version, synced with the cvs

mmohr commented on 2006-04-24 11:31

sync with today's CVS (20060424):

Pool commented on 2006-05-12 23:53

Can we get another sync please with the latest CVS I'm getting HUNK errors 1 out of 9

mmohr commented on 2006-05-15 15:41

Sorry, I don't have time atm. - so somebody else has to do it.
But beware, I think it's not only synching - there has to be some rework to make it properly work again… (I tried it a week ago and it had some weird effects…)

senab commented on 2006-05-29 08:51

Here's a resync made by P.I. Julius

Sync to current CVS

Would someone be so kind to sync it to a recent cvs? since I can't get it to work properly myself. Thanks in advance.

My request is already fulfilled thanks to drippydonut @ misticriver

Updated patch to work agains CVS HEAD from 12.08.2006

Hi Max!
The recent changes in cvs (23.8,20:07,Mark Arigo):

    Allow scrolling lines with different x-margins. The margin at the time of the call 
    to puts_scroll will be used as the margin for that line.

have broken the last patch. Does this mean that a similar functionality will be included in CVS soon, or can we adjust the patch to work with the new CVS code.

Thanks a lot and all the best

Norbert

I am still at CVS from 20060820 where the patch still applies
Will check with CVS HEAD asap

IHMO the change is not changing this patch
just the order of two lines has switched

Updated patch to work against CVS from 20060824

Hi Max!
Thanks a lot. The patch does work after some small changes:

  drivers/lcd-h100-remote.c:1398: error: `xmargin' undeclared (first use in this function)

I changed it to leftmargin. And

  debug_menu.c:1240: error: too few arguments to function `lcd_setmargins'

there is a lcd_setmargins(0,0) left over, I added in the middle the LCD_WIDTH. This way it did compile.

I attach a fixed patch against current cvs for those who want to have it.

Thanks a lot and all the best

Norbert

Ooops!

I only tried it for the simulator and x5 :-(

Thanks for fixing it

updated for current CVS.

mmohr commented on 2006-09-04 21:27

I currently notice a problem when combining this patch with the albumart patch ( FS#3045 ):
I tried to use margins with a bitmap right to it (the margins starts at x=1 and ends one
pixel before the bitmap begins)
If a line left of the bitmap starts to scroll, the line flickers inside the bitmap…

Any idea what can cause this behaviour?

patching file apps/plugin.c
Hunk #1 succeeded at 560 (offset -1 lines).
Hunk #2 succeeded at 574 (offset -1 lines).
Hunk #3 FAILED at 600.
Hunk #4 FAILED at 610.
2 out of 4 hunks FAILED – saving rejects to file apps/plugin.c.rej

patching file apps/sound_menu.c
Hunk #1 succeeded at 742 (offset 1 line).
Hunk #2 succeeded at 790 (offset 1 line).
Hunk #3 FAILED at 1041.
1 out of 3 hunks FAILED – saving rejects to file apps/sound_menu.c.rej

And your point is?

The point appears to be that the patch is out of sync with CVS and needs updating.

mmohr commented on 2006-10-23 08:40

Small bugfix (old declaration in plugin.h and wrong function calls in viewer and solitaire-plugins)
and sync to today's CVS

mmohr commented on 2006-10-25 08:40

sync to today's CVS

sync to current CVS

seems to be out of sync again.

Sync to today's CVS

The patch applied successfully, but I get the following errors when trying to compile for the Gigabeat F/X port 20070210-build.

debug_menu.c: In function 'dbg_lcd_power_off':
debug_menu.c:2069: error: too few arguments to function 'lcd_setmargins'
debug_menu.c: In function 'dbg_buttonlights':
debug_menu.c:2111: error: too few arguments to function 'lcd_setmargins'

I am able to compile a clean build and also after adding the album art patch.

Thanks for any help.

This one should work.

Breaks the 4G Grayscale and iPod Mini 2G builds, seems to be okay with 5G and Nano (problem with b/w targets?)

Better now ? :)

Cheers ^^

Don't think this is quite working on the 5G, for example,

%s%m|18|209|%al%?ia<%ia|%?d2<%d2|(Artist Unknown)»

the |18|209| actually come up on screen?

Hi, great patch. Recent changes to bookmark.c seem to have broken it though. It patches fine, but when I try to compile, I get the following error:

bookmark.c: In function 'select_bookmark':
bookmark.c:553: error: too few arguments to function 'screens[i]setmargins'

I don't get the error in compiling when I remove the patch. I'm compiling for a Gigabeat F.

Thanks for any help.

edit the line to include 'screens[i].width' as second to last argument

So it reads: screens[i].setmargins(XXXX, screens[i].width, XXXX);

Just a question… will this ever be committed? The progress shows 100% complete, and the last "small bugfix" was on Monday, 23 October 2006… Are there any known bugs left? What are they?

Sync'ed to latest svn

You patch is for revision 12701
Since revision 12704 this patch is not working due to changes in the files list.c and solitaire.c

Don't worry about that, those changes to list.c and solitaire.c were reverted in svn anyway (as 12719). I just tried repatching latest svn (12723) and it worked fine.

Dano commented on 2007-03-12 10:10

Hello, I am new to this lark and am having some trouble using your patch. This is probably not the place to ask but ohwell. When I try to patch and compile using cygwin from the latest build for my iRiver H320 I get the following error:

CC drivers/lcd-remote-1bit-v.c
drivers/lcd-remote-1bit-v.c:82: error: conflicting types for 'lcd_remote_setmargins'
export/lcd-remote.h:145: error: previous declaration of 'lcd_remote_setmargins' was here
drivers/lcd-remote-1bit-v.c:82: error: conflicting types for 'lcd_remote_setmargins'
export/lcd-remote.h:145: error: previous declaration of 'lcd_remote_setmargins' was here
make[1]: * [dir] Error 1
make:
* [all] Error 2

This is doubly annoying since I don't own an LCD remote so really don't care what is displayed on it!

I have the following code in my wps-file:
%s%m|16|144|%?it<%it|%d2>
%s%m|16|144|%alNext: %?It<%It|%Fn>

The first line scrolls to the end of the text, stops and scrolls back to the beginning.
In the second line it seems the text will get copied several time.
Like:
strcpy(showntext, "Test ");
for(a=0;a<UNKNOWN_NUMBER;a++)

strcat(showntext, "Test ");

⇒ showntext="Test Test Test Test Test Test Test" (or something like that).
The result is, that the text keeps scrolling without changing direction.
Is this a bug or a feature?

I'm pretty sure its supposed to keep scrolling one way. The 'back and forth' scroll is another FS patch, can't remember which number

senab commented on 2007-03-12 10:50

Chris,

The scrolling does change direction in this patch, at least it should :D

The last time I tried it didn't =/

The scrolling "type" depends on the length of the string

But can be influenced from the setting "Bidirectional Scroll Limit"
(which is 50% by default IMHO)

The scrolling "type" depends on the length of the string

I just found that one out.
so it's a feature :)

Dano: I'll take a look at updating in a minute or two, I confess I didn't try all targets

Dano: hm, the H3xx lcd driver seems to build ok here. Did you get a fresh update of source from svn and did you apply any other patches first (and/or were any conflicts or failures reported when you applied the scroll_margins_20070310 patch?)

Is the code for the file sound_menu.c needed?
This file has been removed from the current svn-tree.

Dano commented on 2007-03-14 10:15

Yeah I just noticed that when I went to try and confirm stripwax's questions with the latest build.

Anyway previously I had got the 20070312 source from the archive daily builds page, and I had also applied the latest album art patch and eq wps tags patch, and there weren't any errors or conflicts applying any of the patches.

Thanks very much for your help.

Will there be a patch for the current svn-build?

Rasmusma: Sync'd to svn as-of a couple of hours ago
Dano: hopefully this time the patch will work for you also, I've manually updated the 1bit remote driver

Dano commented on 2007-03-18 18:29

It works beautifully now, thank you very much for your help stripwax!

Soap commented on 2007-03-28 00:40

I am not 100% confident in my sync to 12944. Someone probably should test with a LCD remote. (Or just manually sync stripwax's lastest work.)
It appears to work fine on my iPods. (Famous last words.)

With some minor failed hunks in gwps/gui/gwps-common.c that can easily be manually added in by hand, Soap's patch works the best right now.

Would there be any problems to add this to the svn-tree?

I can't seem to get this to work in the new SVN.
I get "gui/gwps-common.c: In function ‘get_line’:
gui/gwps-common.c:1421: error: ‘fmt’ undeclared (first use in this function)
gui/gwps-common.c:1421: error: (Each undeclared identifier is reported only once
gui/gwps-common.c:1421: error: for each function it appears in.)"
And 1421 is the whole "case 'm': /* margins */" hunk of code.
Can someone upload a sync'd patch, or at least tell me what I've done wrong?

Some parts of the patch will need to be rewritten to be made compatible with the WPS tokenizer.

I'm working on the rewrite.

Sync'd. Added new WPS virtual tokens to handle the left and right margins. Shout if you find any problems with scrolling margins in WPSes that used to work but break with this new patch.

Sync'd again.

Sync'd again. Unfortunately the new WPS parser is a moving target, and the underlying function definitions keep changing..

Sync'd again. Also fixes problem of dynamic data on non-scrolling lines not updating correctly now that gwps-common has been fixed in svn to handle WPS_REFRESH_SCROLL properly (the %m tag should NOT have had WPS_REFRESH_SCROLL flag in wps_parser.c in the first place - apologies)

Needs another sync. Applies with many failed hunks.

I think you may be doing something wrong. I just updated to revision 13174 just now and this patch applied without incident.

Hmm. I'll try to apply to a clean SVN then. Maybe some other patch is messing it up.

Sync'd and fixed. Today's wps parser fix in svn revealed an off-by-one error in my parse function which was now caught by the improved error check (meaning wps that used scroll margins were no longer loadable!) Also needed resyn due to bookmark.c change in svn

hmm
I ask again: Would there be any problems to ADD this patch to the svn tree?

for anyone building for a gigabeat and getting errors on debug_menu.c
edit line 1140 in apps/debug_menu.c so it reads:

lcd_setmargins(0, LCD_WIDTH, 0);

sync'd and updated. If using albumart too, use latest albumart patch too to avoid sync conflicts.

senab commented on 2007-04-28 22:17

Has anyone tried this? Because I'm having trouble loading up WPS's. It's either this or the Album Art patch.

Senab - I have been running my own build using this patch and the album art patch without any problems.

Out of sync- I fixed one simple update hunk but someone who knows the code needs to look at the others and see if they're needed

Hunk 1 of apps/onplay.c and Hunk 20 of apps/debug_menu.c

Attached is the otherwise synced version.

I wish I could say whether or not those 2 hunks are truly necessary, but for what it's worth I haven't had any problems whatsoever using the broken patch.

Same here.

FS #7158 being commited breaks this patch.

Are you sure it's broken? I took a look at the .rej and it's trying to add rb→lcdsetmargins(0,LCD_WIDTH, 0) so I took a look at bookmark.c for
rb→lcdsetmargins, this function (or whatever its called) isn't in bookmark.c anymore, so I tried compiling and it works. Did you try compiling?

Ah, I should probably do my homework before posting.

I think it's out of sync

I think this works.

I get two hunks failing and it won't make.

i too get a failure at lcd.h (hunk #2 at 393) and at lcd-16bit.c (hunk #7 at 947)

what to do…

Sync again then.

While the patch is working for me, I seem to be having trouble using the regular %m tag, such that it is a normal line. No matter what I do, the margin designated with just %m retains the same left position as the custom lines. Is anyone else having this issue, or perhaps I'm doing something wrong?

To clarify, when you say custom lines are those custom lines as in jbuild %e.. lines? If so I think that's the problem, mixing %e with %m on the same wps. If that's not the case then I have no idea.

I am actually not making use of the %e tag, but I found a solution to my problem anyways. The solution was simply to use a %m tag along with |x1|x2| positions related to the width of my wps, which was |0|240|. This does make sense, although it isn't what I had expected.

That's because that is how the scrolling margins patch works. Every text line needs to have a margin applied.

dip commented on 2007-07-06 14:47

Out of sync.

Sync Please :)

senab commented on 2007-07-12 15:06

Resync :)

It's just the older version with the hunks that didn't patch taken out (they weren't needed anymore).

Changed to apply cleanly following this morning's SVN changes.

My first re-sync :)

Only one small change; it should work with SVN as of 2007-08-15.

dip commented on 2007-08-18 20:29

The last patch does not work. It misses the changes in alarm_menu.c (which were included in the previous patch).

That's odd. It applies just fine for me on a Gigabeat F40.

It builds fine, but I think it might be causing some weird behavior. Perhaps someone should do a proper re-sync based on the previous patch.

dip commented on 2007-09-02 08:27

When trying to compile for the iPod Video with 64MB this patch still does results in the following compiling error:

… CC alarm_menu.c
alarm_menu.c: In function 'alarm_screen':
alarm_menu.c:73: error: too few arguments to function 'screens[i].setmargins'
make[1]: * [/home/user/rockbox/build/apps/alarm_menu.o] Error 1
make:
* [build] Error 2

When including the previously included changes on alarm_menu.c it compiles fine.

idak commented on 2007-09-14 15:14

resync and fix buggy jumping strings

Hmm.. out of curiosity what exactly was it doing and what lines did you change? I pretty much just did a resync of 2007-08-15 with the hunk for alarm_menu.c put back in. I never noticed it doing anything on my iPod and I tested it on various themes some plain, some customline.

idak commented on 2007-09-14 22:39

I added setmargins(0, LCD_WIDTH, 0) in get_line().

Please test follow wps with 2 patches.
— %wd
%s%m|25|100|%?it<%it |%fn>
%ar%pc/%pt
%arTrack %?in<%in|unknown> (%pp/%pe)
%s%m|100|175|%?it<%it |%fn>

idak commented on 2007-09-14 23:38

I change from LCD_WIDTH to gwps→display→width for rwps.

I can definitely see that bug now with that test wps! Thanks for the fix!

Anonymous Submitter commented on 2007-09-24 14:07
idak commented on 2007-09-29 04:02

A long string was displayed protruding to the right when new track info was loaded.
So I fix it.

idak commented on 2007-10-02 00:48

more suitable for rwps.

When setting margins for a non-scrolling line (i.e. just %m without %s), still the complete line is cleared by write_line()/gui_wps_refresh(), destroying e.g. album art that might be next to the line and which is supposed to be protected by the margins.

The attached patch fixes this for me. Does anybody see a reason why it shouldn't be merged into the scroll-margins patch?

idak commented on 2007-10-05 16:33

add lcd-charcell.c bogus support.
Now all target can be supposed to use this patch…

@Martin, I agree that should be merged too, and on another note thanks for posting that. Looking at that area of code made me rediscover something I meant to do a long time ago on another patch and just forgot to.

Sorry for the double post but this just occurred to me, I'd still go with display→width on that instead of scroll_width.

idak commented on 2007-10-06 01:26

Does this patch fix that problem?

idak commented on 2007-10-06 03:41

Fix clearing the center aligned string.
Sorry for above buggy patch…

@idak, what do you think of something like:
if (left_width || center_width || right_width) {

             display->set_drawmode(DRMODE_SOLID|DRMODE_INVERSEVID);
             display->fillrect(left_xpos, ypos, display->width, string_height);
             display->set_drawmode(DRMODE_SOLID);
         }
idak commented on 2007-10-07 11:27

@tdtooke, Can I have your test wps, please?
I don't understand what has occurred…

Nothing happened, the patch performs flawlessly. I just thought that would be easier. Well….actually my motivation is a little bit selfish. If that part looked that way then all I'd have to do to sync another patch would be to just manually edit the patch, otherwise I have to do a lot of typing.;-)

jac0b commented on 2007-10-18 23:36

Out of sync, I tried to figure out the problem but it looks like a code change and I have no experience coding.

jac0b commented on 2007-10-21 20:37

getting a error on main_menu.c, I looked for the proper line but there must have been another code change.

I just igonired that HUNK, with success :)

jac0b commented on 2007-10-21 23:29

it compiled but does it work?

If you mean, that all my themes are working as they should, yes! ;)

You don't' have to worry about it. They're doing things a different way now on main_menu.c that doesn't need that. Now if they had just moved that you'd not just get a warning, it wouldn't compile at all since you'd be calling setmargins with too few parameters.

Along with main_menu.c failing, debug_menu.c and scroll_engine.h now fail. Could someone please resync?

jac0b commented on 2007-11-12 23:15

Sync errors on todays SVN

8 out of 14 hunks FAILED – saving rejects to file apps/gui/gwps-common.c.rej
1 out of 1 hunk FAILED – saving rejects to file apps/gui/gwps.h.rej
1 out of 3 hunks FAILED – saving rejects to file apps/gui/wps_parser.c.rej
1 out of 16 hunks FAILED – saving rejects to file apps/debug_menu.c.rej

that's because part of the patch was committed. Should be an easy sync.

jac0b commented on 2007-11-12 23:25

Yeah I saw that but why only left margins why just commit this patch?

Adding support for the right margin is the intrusive part of this patch. The left margin part is pretty trivial in comparison and only affect the WPS code. More than what has been committed isn't wanted because viewports are planned.

With a bit of a rewrite this could be made to assume that no declaration of margins means %m|0|320| and %s|leftmargin| means %s%m|leftmargin|320|. This might be the best compromise with the latest commit. Thoughts?

or should I say where I listed what it'd be on an iPod 5.5 it should be %m|0|LCD_WIDTH| or something like that

Very quick and dirty. With this if you're using a new WPS which might make use of %s|leftmargin| that will work and as before it works with %m|leftmargin|rightmargin|. As always if you set a margin for one line you set a margin for all lines (with %m that is). I have noticed that placement of the %s tag in the middle somewhere as opposed to the beginning of the line can cause problems so if you're using partial-scroll be aware of that. Also if you use the scrollwheel acceleration patch for the iPod that patch will add a setmargins call that you will have to manually fix. I didn't include that because that would introduce a dependency.

Another quick and dirty. This one basically just extends the %m functionality just committed to include the rightmargin. As in SVN it will work with just %m or %m|leftmargin| but will also work with %m|leftmargin|rightmargin| as before. I changed the name because it really isn't scroll-margins anymore. Maybe this should have a separate page on flyspray? Also as before of you use the iPod scrollwheel acceleration patch there is a setmargins call in debug_menu.c that you need to manually set. I would have included that but that would introduce a dependency.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing