This is the bug/patch tracker for Rockbox. Click here for more information.
Quick links: Bugs · Patches · Rockbox frontpage
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, 04:13 GMT+2
Last edited by Alexander Spyridakis (xaviergr) - Monday, 22 August 2011, 15:26 GMT+2
Opened by Alexander Spyridakis (xaviergr) - Sunday, 05 October 2008, 04:13 GMT+2
Last edited by Alexander Spyridakis (xaviergr) - Monday, 22 August 2011, 15:26 GMT+2
|
DetailsFrom 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. 
scroll_space_margin6.diff
(14.7 KiB)
apps/lang/english.lang | 14 ++++++++++++++ apps/settings.c | 2 ++ apps/settings.h | 2 ++ apps/menus/display_menu.c | 7 +++++-- apps/settings_list.c | 7 +++++++ firmware/export/scroll_engine.h | 1 + firmware/export/lcd.h | 1 + firmware/export/lcd-remote.h | 1 + firmware/scroll_engine.c | 10 ++++++++++ firmware/drivers/lcd-1bit-vert.c | 11 +++++++++-- firmware/drivers/lcd-charcell.c | 13 ++++++++++--- firmware/drivers/lcd-2bit-vert.c | 11 +++++++++-- firmware/drivers/lcd-2bit-horz.c | 11 +++++++++-- firmware/drivers/lcd-2bit-vi.c | 11 +++++++++-- firmware/drivers/lcd-16bit.c | 11 +++++++++-- manual/configure_rockbox/display_options.tex | 3 +++ 16 files changed, 101 insertions(+), 15 deletions(-)
scroll_padding_string11.diff
(18.1 KiB)
apps/lang/english.lang | 14 +++++++ apps/settings.c | 2 + apps/settings.h | 7 +++ apps/menus/display_menu.c | 48 +++++++++++++++++++++++++-- apps/settings_list.c | 5 ++ firmware/export/scroll_engine.h | 8 ++-- firmware/export/lcd.h | 1 firmware/export/lcd-remote.h | 1 firmware/scroll_engine.c | 17 +++++++++ firmware/drivers/lcd-1bit-vert.c | 10 +++-- firmware/drivers/lcd-charcell.c | 13 ++++--- firmware/drivers/lcd-2bit-vert.c | 10 +++-- firmware/drivers/lcd-2bit-horz.c | 10 +++-- firmware/drivers/lcd-2bit-vi.c | 10 +++-- firmware/drivers/lcd-16bit.c | 10 +++-- manual/configure_rockbox/display_options.tex | 3 + 16 files changed, 143 insertions(+), 26 deletions(-) |
This task depends upon
Closed by Alexander Spyridakis (xaviergr)
Monday, 22 August 2011, 15:26 GMT+2
Reason for closing: Rejected
Additional comments about closing: Rejected due to settings policy and similar functionality in FS#11892 and committed with r29104.
Monday, 22 August 2011, 15:26 GMT+2
Reason for closing: Rejected
Additional comments about closing: Rejected due to settings policy and similar functionality in
What exactly is the point of having this extra padding when the padding can be defined via the positioning of the viewport?
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.
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).
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.
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.
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.
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.
Maybe then we can ask people about it in IRC, no point to commit it if my opinion is in a very small minority.
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).
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.
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")
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?)
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".
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)?
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.
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.
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.
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
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!
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.
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 :-)
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.
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.
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 ...
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
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!
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.
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).
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....
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! :-)
Hope everyone is happy for now. :)
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!
2. Corrected
3. Ah didn't notice that your patch was committed.
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
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.
And once more: I like padding with characters much better than padding with pixels.
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.
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.
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
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.
Another attempt with that specific change only.