Rockbox

Tasklist

FS#10350 - plugin lib highscore: fix and improvement

Attached to Project: Rockbox
Opened by Teruaki Kawashima (teru) - Friday, 19 June 2009, 14:03 GMT
Last edited by Johannes Schwarz (Ubuntuxer) - Saturday, 18 July 2009, 15:19 GMT
Task Type Patches
Category Plugins
Status Closed
Assigned To No-one
Operating System All players
Severity Low
Priority Normal
Reported Version Version 3.3
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

*Close file descriptor in highscore_load.
*Replace tabs with spaces.
*Change format of score in file.
Change format from "name:score:level" to "score:level:name".
In this way, name can contain any kind of char including ':' (but currentry, name doesn't seems to be used) and settings_parseline can be used to parse string.
*make highscore_update to return the postion of newly added score.
return 0 if the score is not added.
this value can be used to display something like "New High Score #1".
*change highscore_update, reverse the oder of scores.
Highest score is stored at the end of array and it is not intuitive.
*Modify rockblox to fit above change.
rockbox is the only plugin using highscore_update.
This task depends upon

Closed by  Johannes Schwarz (Ubuntuxer)
Saturday, 18 July 2009, 15:19 GMT
Reason for closing:  Accepted
Additional comments about closing:  Commit in r21960; exclude the usage of the standard list widget in the function show_highscore, because further changes in the list would needed
Comment by Johannes Schwarz (Ubuntuxer) - Monday, 22 June 2009, 14:23 GMT
I reviewed the patch and I think the patch is sensible. Just one request, add a short usage description in file highscore.h, please.
Comment by Alexander Levin (fml2) - Wednesday, 24 June 2009, 12:52 GMT
I like the patch. While we're at it, I'd put the check for "fd<0" in the function 'highscore_load' more close to "open" call, without interleaving the both with "memset" (move memset before open or after if). Some comments about what the parameters and the return values mean would be very welcome!

Also, in the highscore_update function, I'd introduce another parameter for the score buffer capacity. The existing one tells us how many entries there are now, and the new parameter would tell how many entries we can store so that we can avoid overflow if a score is inserted. Or do I miss how this API is used?
Comment by Teruaki Kawashima (teru) - Thursday, 25 June 2009, 14:05 GMT
Update patch.
*add some comments to source code.
*move "memset" before "open" in function highscore_load.
*change function highscore_update a bit. hopefully this change makes it clearer.

In my understanding, num_scores, parameter of function highscore_update, is how many entries we can store.
And the code I wrote wouldn't cause overflow. but it didn't seems undarstandable a bit.
Comment by Alexander Levin (fml2) - Thursday, 25 June 2009, 14:50 GMT
Ok, now I understand how it should be used. No need for an additional parameter. But now I have another question. Why isn't there a parameter for the name in the update function? The name member is never used! In rockblox we could use date/time (on RTC targets) as the name.

I updated some comments.
Comment by Teruaki Kawashima (teru) - Thursday, 25 June 2009, 15:52 GMT
hmm. I assumed the name is intended to be input by user.
In this case, it makes sense that name is not passed to the update function.
e.g.
int pos;
if((pos = highscore_update(...)) > 0)
rb->kbd_input(scores[pos-1].name, sizeof(scores[pos-1].name));
Comment by Alexander Levin (fml2) - Thursday, 25 June 2009, 22:54 GMT
Maybe. If so, we should write it as a comment about the intended (or possible, since rockblox doesn't do it) use. But I think it's not a good API. If you need an input from the user (we can provide a function to test whether the entry would be inserted) then you should first get the input and then call score_update, passing all the values for the entry. In rockblox, no user input is required. We could pass the date/time on RTC and an empty string on non-RTC targets. Or the name of the currently playing song (on both)!

BTW: wouldn't it be better (and more C-like) to return a 0-based value in the update function (and a negative one if the entry was not inserted)? You could then use it as the index in your example above, no need to subtract 1 (in a message for the user you'd have to add 1 though). But I think it's more natural to work with 0-based values in C.
Comment by Alexander Levin (fml2) - Friday, 26 June 2009, 21:53 GMT
This is how I would do it. (Populating of the name with the current time in rockblox is not implemented.)
Comment by Alexander Levin (fml2) - Sunday, 28 June 2009, 12:09 GMT
Here is the patch that adds the date/time of the score (for RTC targets only, for non-RTC targets, "???" is written). The date/time is not displayed in the plugin but can be seen e.g. in an editor.
Comment by Alexander Levin (fml2) - Sunday, 28 June 2009, 16:28 GMT
In rockblox, there is no need to check for RTC since get_time work on non-RTC platforms as well.
Comment by Johannes Schwarz (Ubuntuxer) - Friday, 17 July 2009, 23:25 GMT
I wrote a extension for the highscore lib, which prevents to save an unchanged highscore. Besides I move the function ..._show_highscores to the lib.
Comment by Teruaki Kawashima (teru) - Saturday, 18 July 2009, 07:21 GMT
i wonder why you reopened this instead of creating new one...

*use "highscore_update" & "highscore_show" for names so that it would looks more consistent.
*remove weird "num_scores > 1" from lib/highscore.c
*add some "highsocre_updated = false;"
*disable "highscore_show" for charcells lcd.
Comment by Teruaki Kawashima (teru) - Saturday, 18 July 2009, 07:34 GMT
sorry, forgat to change "show_highscores" to "highscore_show" in some files.
Comment by Teruaki Kawashima (teru) - Saturday, 18 July 2009, 09:20 GMT
fix bugs in bubbles.
Comment by Alexander Levin (fml2) - Saturday, 18 July 2009, 11:14 GMT
> i wonder why you reopened this instead of creating new one...

Same here :-) I also wonder why you didn't implement the "show" function using the standard list widget. Because it doesn't allow to display data in columns?
Comment by Johannes Schwarz (Ubuntuxer) - Saturday, 18 July 2009, 12:15 GMT
> I also wonder why you didn't implement the "show" function using the standard list widget. Because it doesn't allow to display data in columns?
As I started to write the "show_highscore" function I don't know anything about the standard list widget. I wouldn't change the function, even though it looks strange on big screens if sysfont is used.

The patch from terru works fine for me.

Loading...