Rockbox

Nice!
Do you come on the IRC channel sometime ? I’d like to discuss your patch in further detail.
I’ve read most of it and two things caught my eye:
* You don’t seem to have respected how subline timeout values are refreshed, especially in the case of conditionals. This caused me problems and was the reason I added get_subline_timeout(), which you seem to have removed and not rewritten.
* Maybe having a “num_tokens” field in the subline struct would make things simpler and remove the need for wps_last_token_index()
* When I tested, the RTC tag caused a crash. That’s because the displaying code relies on the fact that there will be a WPS_TOKEN_RTC before any other RTC token.

from a quick look, wordForNext() catched my eye as it doesn’t comply to our code policy. Just thought I mention it

OK, I made some changes to the patch. I think it’s almost ready to commit, but I’d like you to review it first :)
* The main change was to make sublines work as they should. update_curr_subline() is back in its original state (only with the necessary changes to access the data struct), because your version with a while loop could cause infinite looping when no sublines are to be displayed, which is very possible. This lead me to make a few changed related to the time_mult values.
* There was a mistake in the RTC tag handling which I fixed.
* I renamed wordForNext().
* I made a few other minor changes.

I decided it wasn’t worth adding a “num_tokens” field in the subline struct, as the code is pretty clear as it is.
Also, binsize seems to shrink a fair bit, which is very nice :)

I hope to commit this soon.

   wps_parsing.patch (49.4 KiB)

Nico, I think that the subline timeout parsing (and handling) should be slightly changed. Timeout can also be specified outside of a conditional. It should then be applied to all cases of the conditional.

For example: %t3%?it<%it|%fn> (compare with %?it<%t5%it|%t10%fn>)

Is this correctly handled?

And what about %?it<%it|%fn>%t3, i.e. if the time_mult is specified at the end?
And this: %it%t3 , i.e. without conditionals at all?

In general, I find the code with ‘only_one_subline’ too tricky. Why is this fragment so complicated? The goal is simple: find the next subline with display time > 0. Apart from the fact that I understood time tags wrong, I find my impl simpler.

I think, one of the cases above doesn’t work. Try this: %t4%it;%t1%ia

Yes, subline timeout values are handled fine even when they are outside conditionals.
About the ‘only_one_subline’ code, we need to find the next subline with display time > 0, but sometimes there isn’t any so in your code there was an infinite loop.
Your %t4%it;%t1%ia example works fine for me.

but sometimes there isn’t any so in your code there was an infinite loop.

Hm… I thought I checked whether the starting point has been reached and leave the loop then.

I also don’t quite understand the stuff with 100*HZ as in

data→lines[line].subline_expire_time = (reset_subline ?

      current_tick : data->lines[line].subline_expire_time) + 100 * HZ;

And why the expiration time is calculated this way. Why isn’t time_mult added to the current tick?

OT: @Nico: have you seen my comment about the funny definition: %cp is AM/PM and %cP is am/pm (lowercase p is for uppercase AM/PM and vice versa). Shouldn’t we change this?

I agree P for lowercase and p for uppercase is weird, but I’m not sure it’s worth changing it now. It’s not the main purpose of this patch anyway so we can discuss it later.
About the current subline calculation code : the expiration time is another value. I’m not completley sure of its purpose but it does seem necessary.
If you agree I’d like to commit the patch soon. Do you have any other changes to make ?

I committed this with a few changes, the main ones being a mistake fixed in update_curr_subline() and much smaller struct wps_line and struct wps_subline.
If you have other changes to suggest, please submit a new patch :)