Rockbox

  • Status Closed
  • Percent Complete
    100%
  • Task Type Patches
  • Category Plugins
  • Assigned To No-one
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Version 3.3
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by teru - 2009-06-19
Last edited by Ubuntuxer - 2009-07-18

FS#10350 - plugin lib highscore: fix and improvement

*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.

Closed by  Ubuntuxer
2009-07-18 15:19
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

I reviewed the patch and I think the patch is sensible. Just one request, add a short usage description in file highscore.h, please.

fml2 commented on 2009-06-24 12:52

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?

teru commented on 2009-06-25 14:05

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.

fml2 commented on 2009-06-25 14:50

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.

teru commented on 2009-06-25 15:52

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;
if1) > 0)

  rb->kbd_input(scores[pos-1].name, sizeof(scores[pos-1].name));
1) pos = highscore_update(…
fml2 commented on 2009-06-25 22:54

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.

fml2 commented on 2009-06-26 21:53

This is how I would do it. (Populating of the name with the current time in rockblox is not implemented.)

fml2 commented on 2009-06-28 12:09

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.

fml2 commented on 2009-06-28 16:28

In rockblox, there is no need to check for RTC since get_time work on non-RTC platforms as well.

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.

teru commented on 2009-07-18 07:21

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.

teru commented on 2009-07-18 07:34

sorry, forgat to change "show_highscores" to "highscore_show" in some files.

teru commented on 2009-07-18 09:20

fix bugs in bubbles.

fml2 commented on 2009-07-18 11:14
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?

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...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing