Rockbox

Tasklist

FS#9455 - Add half the viewport width in spaces for forward scrolling.

Attached to Project: Rockbox
Opened by Alexander Spyridakis (xaviergr) - Sunday, 05 October 2008, 02:13 GMT
Last edited by Alexander Spyridakis (xaviergr) - Monday, 22 August 2011, 13:26 GMT
Task Type Patches
Category LCD
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Daily build (which?)
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

From the beginning Rockbox scrolling made me dizzy, either in bidirectional or normal scrolling mode.

The fact is that without a static point in a scrolling line the eye can be confused and loose focus. This patch alleviates the problem by padding in the string enough spaces to full half the width of the scrolling viewport.

EDIT: The patch now uses a setting (number of pixels) to calculate the padding. It is no longer hard-coded to half the viewport width.

EDIT 2: Latest patch stores a custom string from virtual keyboard to append at the end of a forward scrolling line.
This task depends upon

Closed by  Alexander Spyridakis (xaviergr)
Monday, 22 August 2011, 13:26 GMT
Reason for closing:  Rejected
Additional comments about closing:  Rejected due to settings policy and similar functionality in  FS#11892  and committed with r29104.
Comment by Paul Louden (Llorean) - Sunday, 05 October 2008, 07:30 GMT
Isn't the whole point of viewports so that you can just define a viewport to limit where scrolling happens on the screen?

What exactly is the point of having this extra padding when the padding can be defined via the positioning of the viewport?
Comment by Alexander Spyridakis (xaviergr) - Sunday, 05 October 2008, 14:54 GMT
No I don't think making a smaller viewport solves the issue.
The end of the scrolling line will still be only 2 spaces apart from the start, making it hard for the eye (at least for me) to focus. The patch makes scrolling similar to H100/H300 OF (though in that case it was rather limiting), where the end of the line is clearly separated from the start with more spaces.

Maybe this is a little bit nitpicky from me and I posted this patch, in order to hear more opinions about the current 2-space padding, which I find rather small.
Comment by Paul Louden (Llorean) - Sunday, 05 October 2008, 15:12 GMT
Then wouldn't another solution be to simply put some spaces on the line after the the tag (for in-WPS scrolling) or in the .lang file after the string (for other cases)?

It just seems like, if this is going to be hard-coded it should be the minimum number of spaces (since you can't subtract spaces through trickery like this, but you can very easily add them).
Comment by Alexander Spyridakis (xaviergr) - Sunday, 05 October 2008, 15:36 GMT
Well I don't find your idea of padding tags and strings beforehand very intuitive. The padding is meant to be scroll engine's job.
On the other hand though, I understand that my patch is merely a detail, which most people wouldn't have noticed. So I am waiting for a consensus, and if the patch is not needed I will happily close the task.

Another last option is to make it a setting (Width of scroll padding?), but I understand that this hits the "too-many-settings" argument.
Comment by Alexander Levin (fml2) - Sunday, 05 October 2008, 19:31 GMT
I also find RockBox's scrolling a bit confusing, here I'm fully on Alexander's side. I find the gap between the end and the start too small. Other programs use e.g. "+++" to separate them. It's another question how to separate -- by increasing the space or by inserting some separating characters. If space, then how big? Half the viewport width might be too much for a fullscreen wide viewport. Can it be made a function of the LCD API (e.g. set_scroll_separation)?

And I also agree with Alexander that adding spaces to strings is not a good solution. Strings should contain just the text they should and shouldn't be modified/adjusted because of displaying issues.
Comment by Paul Louden (Llorean) - Monday, 06 October 2008, 01:03 GMT
I'm suggesting a way he can privately fix the problem for his own personal use. I don't see why "modifying the strings" is such a bad solution for personal use.

What I'm suggesting is "Enforcing a very long gap" on all users, when users can privately increase the gap size, is a bad idea. It either need to be configurable, or it needs to be the minimum possible size exactly *because* it is possible for users to increase the gap but not decrease it without knowledge of C.
Comment by Alexander Spyridakis (xaviergr) - Monday, 06 October 2008, 01:24 GMT
Lloeran: Then do you think it would be worthwhile to make it a configurable setting, or will it be considered a bloat?
The fact is that only 2 spaces in scrolling seems confusing for some, but I understand that having half the viewport filled with spaces will annoy others.
Comment by Paul Louden (Llorean) - Monday, 06 October 2008, 01:25 GMT
I don't know how other people will feel if it's a setting, but I think hardcoding it to something large is (to my mind) absolutely unacceptable. So, personally at least, I'd prefer a setting over this patch if it absolutely must change. I don't think it's "bloat" if it causes a real problem for the ability of people to read what's onscreen.
Comment by Alexander Spyridakis (xaviergr) - Monday, 06 October 2008, 01:32 GMT
Ok, then I will try to make it a setting and see if more people are interested in this.
Maybe then we can ask people about it in IRC, no point to commit it if my opinion is in a very small minority.
Comment by Dan Everton (safetydan) - Monday, 06 October 2008, 03:14 GMT
I might be misunderstanding the purpose of this request, but it sounds sort of like the jump scrolling that the Archos targets have. As far as I remember, this was just something that the LCD driver needed to do, but no one's done it yet.
Comment by Paul Louden (Llorean) - Monday, 06 October 2008, 03:23 GMT
If I understand correctly, the problem is that the start and end of a string appear to close when scrolling.

A scrolled line reading simply "Text" can look like "Text Text Text Text Text" as it's scrolling, and this would increase the apparent spacing so it's "Text Text Text" improving readability (I haven't used a realistic number of spaces here, and "Text" is obviously too short to scroll, but you get the idea).
Comment by Alexander Spyridakis (xaviergr) - Monday, 06 October 2008, 03:26 GMT
Jump scrolling? Can you clarify it a bit please?
The term jump scrolling as I understand it, is achieved by lowering the scrolling speed (0-1) and increasing the scroll step size.
That way instead of a normal scroll you get a somewhat jumpy scroll (a large amount of text changes slowly) which is better for slow response LCDs like H100.
Comment by Alexander Spyridakis (xaviergr) - Monday, 06 October 2008, 03:35 GMT
Here you can look at an example. In the picture you see a scrolling line with this patch applied.
The end of the line is padded with enough spaces to fill half the width of the scrolling viewport.

Without the patch the end of the line is separated from the start with only 2 spaces, making it difficult for the eye to focus, when you are not looking carefully to it. (e.g the word "Adagio" would be too close to the number "01")
Comment by Jonathan Gordon (jdgordon) - Monday, 06 October 2008, 05:16 GMT
I'm chucking my 2 cents in here... I'm all for the patch. I dont know if adding width/2 is a good amount though.. even width/4 might be enough.
Also, it probably is overkill to add a setting for this...

But, I'm not sure the way your doing the patch is great. It doesnt look like your checking the string length so there is always the chance of buffer overflow with your patch.
A better way to do it is to change the scroll engine to not start redrawing the start untill there is a gap of X pixels between the end of the text and the edge of the viewport. This way eliminates the need for memset's, no exra overflow chance, and can probably be done in a single file (scroll_engine.c probably?)
Comment by Alexander Spyridakis (xaviergr) - Monday, 06 October 2008, 14:52 GMT
Currently even with a 255 character file it is impossible to have an overflow.
Also I tried to do it at first the way you are suggesting but I was getting unexpected behaviour. I will try again though to see if I can make it more clean.

As for making it into a single file, currently, I don't think it can be done. The initialization of the scrolling string happens on the lcd-x files respectively and scroll_engine.c has nothing to do with it (as I understand it).

Ok as discussed on IRC, I will adapt it to make it a setting.
The setting will prompt for 10s of pixels and the space padding will be Setting / "width of space character".
Comment by Alexander Levin (fml2) - Tuesday, 07 October 2008, 18:41 GMT
I like the way WinAmp does it, i.e. separating end and the next start with "+++". Couldn't we do it so that a positive value of the setting would mean the number of spaces (or pixels), and a negative value would be the negated number of "+" that, surrounded with spaces, are used as the separator? I.e. "-3" would mean "<space>+++<space>".
Comment by Alexander Spyridakis (xaviergr) - Wednesday, 08 October 2008, 03:02 GMT
I was wondering, will this setting be general only for the main LCD, or should we enable it for the remote too?
Imho, it would be better for both LCDs taking into account the other scroll options. Also a very large setting for the main LCD would be abnormally long for the remote LCD.

On another matter, what about the default value? Should it just be 10 pixels (which is almost the same as now for Nimbus 14) or maybe keep the current default of 2 spaces by choosing a 0 pixel setting (the latter is inconsistent for the user though)?
Comment by Alexander Spyridakis (xaviergr) - Wednesday, 08 October 2008, 15:52 GMT
Hopefully one of the last revisions of the patch.

Adds one setting for Scroll padding (two if target has an LCD_REMOTE).
The range of the setting is 0 - LCD_WIDTH normalizing it to the nearest multiple of ten pixels.
The calculation of the number of padded spaces is "Number of selected pixels" / "Width of space character of selected font" (obviously ignoring any decimals).
Also added a small description for the manual.

Bin size difference seems to be 612 bytes for H300 and 192 for OndioFM.
Comment by Alexander Levin (fml2) - Wednesday, 08 October 2008, 19:58 GMT
Why did you decide to go with pixels and not with spaces? Spaces are better IMHO because they adjust automatically to the font chosen. I.e. 20 pixels might be too much for a small font but would be too narrow for a large font. Whereas "4 spaces" would always fit. And it would also simplify the calculation of the number of spaces (no calculation at all). And what about the "+" if the value is negative?
Comment by Alexander Spyridakis (xaviergr) - Wednesday, 08 October 2008, 21:13 GMT
On the IRC it was discussed and the consensus was for pixels, that's more clear because we want a fixed space between the start and the end of the scrolling line. If we choose spaces then the padding will be no longer fixed and will change with the selected font. That is inconsistent (at least in my eyes).

I don't really know about the "+" and negative values. It would seem weird to me to put negative values on the setting selection.
Of course I am not the one to decide about it, if the devs think it will be worthwhile I don't have any problem adding something like this.
Comment by Alexander Spyridakis (xaviergr) - Wednesday, 08 October 2008, 22:06 GMT
Another idea (bluebrother's actually) is to let the user select his own padding string via the virtual keyboard.
The setting options will ask for a string and append it to each forward scrolling string.

What do you think? This way more people will be pleased.
Comment by Alexander Levin (fml2) - Thursday, 09 October 2008, 07:35 GMT
A custom padding string is the solution I'd prefer! I just didn't know that it's possible to let the user to input a string while in the settings. I don't like padding with pixels because it doesn't scale with fonts.
Comment by Alexander Spyridakis (xaviergr) - Sunday, 19 October 2008, 02:17 GMT
New patch adds a setting (one for main screen, another one for remote screen if available) which prompts the virtual keyboard.
The user can input any preferred string which is limited at a length of LCD_WIDTH / 4 and LCD_REMOTE_WIDTH / 4 respectively, the inserted string will then be appended to forward scrolling lines.

The string is stored and contained by quotes in the configuration file. It must be contained in quotes (""), because the configuration file is parsed in a way that ignores spaces. This way all characters (including spaces) can be used as padding.

The user doesn't have to worry about the quotes as they appended to the string after it has been typed. If in any case the quotes are missing from the configuration file (e.g manually editing), rockbox won't crash or alter the way scrolling lines and padding is inserted and next time the user inserts a new string it will be resolved.
The default setting isn't altered and still remains at 3 spaces.

Of course this patch is more bin/ram size dependent than the previous one.
H300: Bin difference: 848 RAM difference: 752
Recorderv2: Bin difference: 408 RAM difference: 376
Comment by Paul Louden (Llorean) - Sunday, 19 October 2008, 05:27 GMT
What happens if the person inputs "word" as the string (including "s), will Rockbox load an empty string because the quote is closed before you even get to 'word'?
Comment by Alexander Levin (fml2) - Sunday, 19 October 2008, 09:44 GMT
Alexander, why do you declare the padding string as 'scroll_padding[LCD_WIDTH / 4]'? LCD_WIDTH is measured in pixels IIUC, and not in characters. Hence it will be too big IMHO. Why not do it just, say, 10? Same for the remote.

Another remark:I don't quite like the fact that quote stripping is done at the firmware level. IMHO this should all be handled at the app level, and the padding string should be passed to the firmware level functions. I mean the function lcd_scroll_padding in firmware/scroll_engine.c.

Also, it shouldn't actually be of any importance what chars are used as 'quotes' as long as it's not a space. So that before saving the settings, you just blindly add the character at the beginning and at the end, and before using (or right after loading the settings), the are blindly stripped off. This would also address the Llorean's concern. And also simplify the code.

But the feature itself is a very welcomed one!
Comment by Alexander Spyridakis (xaviergr) - Sunday, 19 October 2008, 14:18 GMT
Llorean: Quotes are fine, the configuration file will have double quotes, but that is not affecting the padding mechanism. So yes the padding will be "word" on screen and ""word"" on the configuration file.

fml2:
About padding length:
There must be a way to define the scroll padding length which will be proportional to target's LCD. If the value is constant it could be very large for a unit with small lcd width and too large for a unit with a bigger screen. Eg. a value of '10' as you say is quite small, 10 spaces in a large screen are negligible.

Firmware/App level:
I considered that as well, at first stripping was done only on the app level. But that had the shortcoming that when the unit boot, the app level function (prepare_lcd_scroll_padding) never run, so the padding was passed with the quotes (E.g "word" instead of word). If someone has an idea to overcome that problem then of course, having the stripping out of firmware level is preferred.

About stripping and quotes:
Yes, it couldn't matter in what chars the string is encased but I chose quotes for consistency and in order to make it clear to someone that alters the configuration file manually. Also keep in mind that we have to look for a final specific character to be able to end the string (the last quote). I add "blindly" the quotes at the beginning and the end in order to avoid crashes or problems with scroll padding if the quotes are missing from the configuration file. The overhead for this protection mechanism is just 3 commands, so I think that it is too small for the gain we get.
Comment by Alexander Levin (fml2) - Sunday, 19 October 2008, 20:13 GMT
My reasoning (re. constant max. number of padding characters) was that on large screens, you usually use bigger fonts which are also wider. Hence the gap (pixel wise) would also be bigger. But even with small fonts: what matters IMHO is the gap width relative to the font used, not relative to the viewport or screen width. And 10 (or, say, 20) would be more than enough. You'd need more characters if you use spaces as the filler, 10 spaces should be enough. And if you use other characters (e.g. "+++") you'd need even less.

About using other symbols as a trailer: what I meant is that you can blindly cut them off and not look for a quotation mark. Since the app would also add them when saving, we have to choose some symbol for that. And a " is a good choice.

As for the firmware/app level: ok, I understand the problem that there is no "hook" when the settings are loaded. It's a pity since it makes the firmware API not as clean as it could be. But then I'd made the firmware level function just ignore the first and the last symbol whatever it is. I.e. in lcd_scroll_padding you don't have to look for the ", just blindly cut off the last char (if there is one) -- like you do it with the first.

And I also think that the fact that the first and the last chars are ignored should be very well documented (in a comment, all in capital letters :-)
Comment by Alexander Levin (fml2) - Sunday, 19 October 2008, 20:32 GMT
Another advantage of the fixed max. length is the fact that it's simpler to describe in the manual :-)
Comment by Alexander Spyridakis (xaviergr) - Monday, 20 October 2008, 02:09 GMT
I don't understand you opposition to variable length padding. It handles memory dynamically without being more complex than a constant definition. If you look at the code (even before the patch) the scrolling lines array is defined with LCD_WIDTH in mind (though LCD_WIDTH is pixels, that array holds characters). There is no difference for scroll padding. Large screens need more space while small screens shouldn't take that overhead and should be limited to something less. If that number has to be fixed the array must be very large to cover all habits (small fonts, big padding) and that will obviously be a waste for small screens.

As for simplifying the app level, by all means if you have something in mind please post a patch. The way things are now I can't see how to make it simpler (and at the same time failsafe) without seriously restructuring the code. It would be good to remove <string.h> from scroll_engine.c and use a memset instead, but how will it be able to know the length of the string without running prepare_lcd_scroll_padding on boot?

About comments: In both cases that the quotes are stripped and restored there are IMHO sufficient comments (2 lines explaining the reason). Okay they are not in capital letters but I didn't see that habit anywhere else.
Comment by Jonathan Gordon (jdgordon) - Monday, 20 October 2008, 02:48 GMT
I agree that having a variable length is overkill... 10-20 chars is PLENTY... also, iirc the settings reading/writing code can help you a bit so you dont have to worry about the quotes... put a suffx/prefix string in the settings_list.c macro, they will be stored in the .cfg but stripped when they are put into the buffer in global_Settings.
Comment by Alexander Spyridakis (xaviergr) - Monday, 20 October 2008, 03:48 GMT
Jdgordon: Where's the overkill exactly? What's the difference writing array[50] than array[LCD_WIDTH \ 4]? Computationally it is exactly the same and more proper for screens that greatly differ in size.

As for the suffix/prefix thanks. So obvious and didn't thought of utilizing it. That will indeed make it far more simpler.
Here is the patch simplified a bit after Jdgordon's comment. No more quote handling by the code.

H300 bin size difference dropped to 700.
Comment by Dominik Riebeling (bluebrother) - Monday, 20 October 2008, 06:59 GMT
If I have a larger screen why should I want to use more characters for padding than on a small one? Larger screen will usually mean a bigger font, thus making the number of padding characters the same, so no need to allow more for those.
Another thought: those padding characters could always be enclosed in spaces (i.e. a space before, another after it) -- who would want to have the padding character sticking to the tag? That way an empty setting would be almost the same as the current behaviour and you have two "hard" characters less you need to store. Haven't checked if you did that already though ...
Comment by Alexander Spyridakis (xaviergr) - Monday, 20 October 2008, 14:27 GMT
Because if you use a small font on a big screen (and a lot of people do) then you are stuck with a padding that won't be able to fill e.g half the screen, or in the extreme occasion one screen. My preference is a half screen of space padding, others might need more. I don't really understand why choose a constant value for it, we are talking about 28 characters (ondio) to 60 (ipod 5.5g). The difference is negligible and the user has more options without wasting resources for ondio.


That would make it more complex. After JdGordon's comment it is rather simple, the value inside the quotes (which are automatically appended by the settings mechanism) is literally the padding string. I prefer not to automate it because this will limit behaviour.

"" = no padding (last letter sticks to first)
" " = the default, which occurs on erroneous settings (e.g altered manually on the configuration file), or when the setting is missing
Comment by Alexander Levin (fml2) - Monday, 20 October 2008, 17:43 GMT
Alexander, I understand your point in making the max length of the padding string dependent on the LCD width (BTW: how does this apply to the character displays?). While I still think that a fix length (e.g. 10) would be enough, I wouldn't object at all if it'd be done your way. In the end, it's your patch.

I also agree with you that there shouldn't be automatically added spaces. I.e. "" means no padding at all.

I have some more comments.

1. display_menu.c:prepare_lcd_scroll_padding: I'd declare the array buf as buf[sizeof(global_settings.scroll_padding)]. Thus you don't have to repeat the array length. Same applies to remote.

2. There are several places where you use strncpy, and use the length of the *source* string as the value of third parameter. This is dangerous IMHO. The third parameter should usually be the size of the *destination* buffer.

E.g. in scroll_engine.c:lcd_scroll_padding you have strncpy(lcd_scroll_info.padding, str, strlen(str) + 1); (but there are more such cases in the patch; actually, all strncpy use this pattern effectively turning strncpy to strcpy)

3. lcd-xxx.c: isn't it dangerous to blindly append the padding string to the actual string. Especially now that the padding string has variable max length? I mean the call strcat(s->line, lcd_scroll_info.padding);

4. The new functions are not documented. I'd add a comment before the function (.c or .h?) telling what the function does.

5. The comma after 'lines' is not needed in display_options.tex ("This makes forward scrolling lines, look...")

Hopefully, soon we will come to the end point!
Comment by Alexander Spyridakis (xaviergr) - Monday, 20 October 2008, 20:00 GMT
1. Agreed.

2. After some talking on IRC, Agreed.

3. The same happens a little bit further with original code. The array that holds the scrolling string is vast even before the string padding. I tested on a real target a 255 character filename with maximum padding and it didn't overflow. See scroll_engine.h the define SCROLL_LINE_SIZE (that's the size of a whole scrolling line with padding). From my tests I consider it is adequate.

4. The only file where functions are not documented is in scroll_engine.c, but other functions don't have comments there. I guess it doesn't hurt to add a comment.

5. Agreed.
Comment by Alexander Spyridakis (xaviergr) - Monday, 20 October 2008, 22:58 GMT
New patch with after mentioned changes.
Comment by Alexander Levin (fml2) - Tuesday, 21 October 2008, 20:09 GMT
Hello Alexander. First of all: I seem to be in a different time zone than you are hence can only reply with a significant delay.

The size in an array declaration must be known at compile time. Hence arr[sizeof(xxx)] is ok. But arr[n] isn't (if n is a variable).

I've read the discussion in IRC about the use of strncpy. It should be always used if you're not sure that the destination buffer is big enough to accept the whole source string. In this case, two commands should be used (as Domonoky said): first, strncpy(dst, src, sizeof(dst)), and then dst[sizeof(dst)-1] = 0. Thus you're sure that the buffer is not overrun and that the string is terminated by a '\0'. Another possibility would be just the single call to snprinf(dst, sizeof(dst), "%s", src); But it's probably a little bit slower.

So I still have some remarks.

1. firmware/scroll_engine.c: lcd_scroll_padding: better explicitly set the last char to 0 (after calling strncpy) since here you don't know that the str has the same (or less) length than lcd_scroll_info.padding. Same for remote.

2. lcd_xxx.c: strcat is dangerous because of possible buffer overrun. With sizes as they are defined *now* it may be Ok, but it's a slowly ticking bomb. I'd use strncat instead, or (since we don't have it) snprintf. Speed can and IMHO should be traded here for more solidness in the firmware layer.

3. Wouldn't it be sensible to ensure that the max. length of the padding string is at least 10? I.e. use MAX(10, LCD_WIDTH/4) in the declaration of the setting. Other places do not need to be touched since you use sizeof() and safe string copy functions.

4. In prepare_lcd_scroll_padding, the buf is locally defined to be large enough, so here simple strcpy can be used safely, no need to use strncpy.

5. I'd change the comment in firmware/scroll_engine.c to "Sets the string to use as the padding string for forward scrolling lines". It should not mention anything from the app layer ("copy string from prepare_lcd_scroll_padding to struct lcd_scroll_info").

6. Padding string array is declared as unsigned char[] at one place (settings) and as char[] at another place (firmware). Why the difference? I'd use just char[] everywhere.

7. Why is the size repeated (in settings_list.c)? What does the length param in the macro mean? Couldn't we avoid repeating? This becomes more urgent if you implement the point 3 (hence making size definition more complex).
Comment by Alexander Spyridakis (xaviergr) - Tuesday, 21 October 2008, 23:20 GMT
1. I don't really understand this. lcd_scroll_info.padding is ALWAYS the same size as global_settings.lcd_scroll_padding. These two arrays are interconnected and should not differentiate EVER. The loading settings mechanism ALWAYS ensures by design that the string will end in a null character, also the virtual keyboard does that as well. So we are sure that in both cases that these two arrays change value, the null character won't be omitted. Taking all these into account, it is quite clear to me that it's impossible to have a stray string without a proper end so no overflow or unexpected behaviour can occur. If it was for unrelated arrays I could understand this, but in fact both of them are about the same setting.

2. Actually we have strncat.

3. Yes if that macro is already defined it would be better instead of the ifdefs I use for the player. I will have a look.

4. Well that is the opposite of (1), someone could argue (like you did on (1)) that it should check in case the definition changes. But if all the arrays are of the same size why don't we just use strcpy everywhere?

5. The first time I was told that comments were too few, only to be told now that they are too detailed. I guess I could use it your way, there is no harm really, but aren't you a little finicky? The code is quite simple and complete omission of comments should be fine too (as other functions in there).

6. I only chose to follow the programming habits of each file. I've seen that strange phenomenon in many parts of Rockbox code. I will see what is used mostly and change to that.

7. It isn't repeating. It's just different for charcel displays. The player has an LCD_WIDTH of only 11 so only 2 characters padding after the division. I ifdefed the length for the player and made it LCD_WIDTH. If there is a MAX macro defined then it should be avoided as you say in (3).

We should talk on IRC about it further....
Comment by Alexander Levin (fml2) - Wednesday, 22 October 2008, 05:15 GMT
Alexander, I hope, we'll be able to talk in IRC. Communication here is really a pain.

To 1 and 4: in 4), you define the buf is declared in the same layer and even in the same function and in terms of the setting's size as the copying is made (i.e. you have bug[sizeof(pad_setting)]). Hence here you're always sure. But the array in the firware just happens to be defined the same size as the setting. They are not related to each other that close. That's waht I mean. If someone would change the size of the setting, the size of the array in the firmware would very bprobably be forgotten. Hence here I would be more defensive. But in the first case you never should need that.

To 2. If there is strncat then it would be safer IMHO to use that instead of strcat. The reason is the same as above.

To 3. I don't necessarily mean that macro, it can be also done 'by hand'. I only wanted that even on small displays (e.g. < 40 pixels -- we don't have such but could) the setting would be at least 10 chars. I ask myself whether it's really needed though :-)

To 5. I don't mean that there is too much comment, just the comment in the firmware should IMHO only be in terms of the firmware and not mention anything from the app layer (prepare_xxxx is from app layer).

To 6. Yes, that's weird. I don't know why it's done like this. Just wanted to ask.

To 7. I mean that the size is once defined in settings.h (separately for char cells and dot deisplays) and then the same logic is repeated in settings_list.c So if you decide to change the logic in settings.h (e.g. make it WIDTH/5 instead of WIDTH/4) you have to also change it in other places (and now in the firmware layer as well to be safe; with p.1 and 4 you wouldn't necessarily have to do that). I think that the last param in the FILENAME_SETTING macro is not needed and asked JdGordon on IRC whether it can be eliminated. If it could then we'd have no repetition here. Which is good ("don't repeat yourself" principle).

Sorry for neatpicking. But I think RockBox is a very good software and want it to be so further! :-)
Comment by Alexander Spyridakis (xaviergr) - Thursday, 23 October 2008, 23:19 GMT
After thoroughly discussing things in IRC here is the latest patch with all suggested changes.
Hope everyone is happy for now. :)
Comment by Alexander Levin (fml2) - Friday, 24 October 2008, 16:59 GMT
Almost perfect! ;-)

1. Why did you move the setting 'scroll step' under HAVE_LCD_BITMAP? Don't char cell displays also have this setting?

2. The comment 'for low-width charcell displays redefine scroll padding' in settings.h isn't quite correct. The setting is not REdefined. It is just defined differently for char cell displays.

3. settings_list.c's changes are slightly outdated. The setting is now defined with TEXT_SETTING, and the last parameter in the macro is not needed anymore.

But otherwise I think it's committable. Thank you for your work!
Comment by Alexander Spyridakis (xaviergr) - Friday, 24 October 2008, 17:45 GMT
1. Nope they don't. I guess it was a left over. All other instances of scroll step on other files were under HAVE_LCD_BITMAP
2. Corrected
3. Ah didn't notice that your patch was committed.
Comment by Alexander Levin (fml2) - Friday, 24 October 2008, 22:00 GMT
Now even I can't add anything! :-) We just have to get on the developers' nerves so that they commit it!
Comment by Dominik Riebeling (bluebrother) - Friday, 24 October 2008, 22:40 GMT
I just tried building this for mini2g. Linking fails:
LD rockbox.elf
/home/dom/projects/rockbox/build-ipodmini2g/librockbox.a(lcd-2bit-horz.o): In function `lcd_puts_scroll_style_offset':
lcd-2bit-horz.c:(.text+0x1148): undefined reference to `strncat'
collect2: ld returned 1 exit status
Comment by Alexander Levin (fml2) - Friday, 24 October 2008, 22:50 GMT
Hrm... Might be. It can be that Alexander has strncat in his host environment. I also couldn't find strncat in RockBox, at least in the plugin API. And I thought that everything we have is there. Hence I had a doubt whether we have it (and wrote about it). But OTOH I could compile the sim for my H120. I don't know why it worked for me. Probably also because of the environment?
Comment by Dominik Riebeling (bluebrother) - Friday, 24 October 2008, 23:34 GMT
The sim does support strncat (see the firmware/export/string.h). As the linked host library does have strncat it works, but linking will fail when building for the target. I'm a bit surprised we have this inconsistency -- maybe that header should get the reference to strncat removed.
Comment by Alexander Spyridakis (xaviergr) - Friday, 24 October 2008, 23:41 GMT
Yup, I was taken by surprise too.
I tested numerous times the patch on a real target but not the final versions where small corrections were made. I am sorry for not testing properly the last patch.

Here is another with snprintf instead of strncat.
Comment by Alexander Levin (fml2) - Saturday, 25 October 2008, 08:31 GMT
Out of interest: what did this (strncat -> snprintf) do to the bin size?
Comment by Alexander Levin (fml2) - Saturday, 25 October 2008, 17:44 GMT
As to the bluebrother's idea in the IRC (that there will hardly be the cases of padding strings not starting and ending with a space): I think this is true. Even more: I like the string <five spaces>+++<five spaces> (with nimbus-14) best. To ensure that the padding string starts and ends with a space you can just change the prefix and the suffix in settings_list.c setting it to "\" " and " \"" respectively. Nothing else needs to be changed. You CAN, but you don't have to. Those who explicitely change the setting will know what they are doing. BTW: there is no manual entry!

And once more: I like padding with characters much better than padding with pixels.
Comment by Alexander Levin (fml2) - Saturday, 25 October 2008, 17:44 GMT
Er... Sorry, there IS a manual entry, it's just not as long as it could be. But we'll leave it for the future! :-)
Comment by Alexander Spyridakis (xaviergr) - Saturday, 25 October 2008, 18:39 GMT
I don't like to limit options, the setting is literal. If some weirdo wants no spaces I am not the one to prohibit him (or want '+'s directly appended to the screen). It is not confusing either, even the default settings has the 3 spaces shown on the virtual keyboard. Of course other's might have a different opinion.

About the manual, I know the description is not as detailed as it could be, but I find it descriptive enough and well "laconic". :P

The binsize difference between snprintf and strcat on a real target is 24 bytes for H300 and 16 bytes for ondio.
Comment by Jonas Häggqvist (rasher) - Saturday, 25 October 2008, 20:30 GMT
I strongly oppose making this a setting. If the current amount of spacing isn't sufficient to notice the string repeating, we should look into tweaking that (which seems to be where this task started).

Creating a setting to allow for any random string is simply far overkill. I do understand the reasoning behind being able to set a "marker", but I think we need to be critical before accepting any more settings to the already huge array of tweakables. This is a setting for a small minority, which is simply too small to get an option added to Rockbox.

Problem: It's hard to tell when a string repeats.
Solution: Figure out a more sensible amount of nothingness to put before repeating the string - don't give up and say "oh we'll make it a setting then!". That is cowardice.
Comment by Jonathan Gordon (jdgordon) - Sunday, 26 October 2008, 06:31 GMT
whats the argument against a setting?

bin size increase? Adding a setting doesnt add very much at all, especially a string setting, the extra bin comes in the menu option which its certain peoples forcibly stopping us from either adding settings which are only available in the .cfg, or adding settings which are only available in a plugin.

menu bloat? Fair enough, but unless someone who argues this helps come up with an alternative then I reject this.

an extra setting which the majority (we will never have numbers to support this) wont use? SO? if they don't use it they will ignore it, why disadvantage part of the userbase because they don't have a loud voice? my config.cfg has 19 settings in it... out of 180+... you don't hear me complaining about the other 160 settings being bloat or waste or confusing
Comment by Alexander Levin (fml2) - Monday, 27 October 2008, 21:31 GMT
Alexander, as has been said in IRC, using something like snprintf(dst, size, "%s", dst) (i.e. the destination string is printed to itself) is dangerous and should be avoided. It doesn't work e.g. in Ubuntu Linux. It might work on target (where we have our own implementation; on Linux we use the host implementation) but if there is a chance that it wouldn't we should avoid such code. Better use a temporary buffer, like this:

char tmpbuf[sizeof(s->line)];
snprintf(tmpbuf, sizeof(tmpbuf), "%s%s", s->line, lcd_scroll_info.padding);
strcpy(s->line, tmpbuf);

Another way would be to implement strncat.
Comment by Alexander Spyridakis (xaviergr) - Tuesday, 28 October 2008, 03:36 GMT
Strange that snprintf used like that caused problems on some platforms, for me it worked as expected (both in sim and real target).

Another attempt with that specific change only.

Loading...