gwps-common.c skip_conditional() strangeness

[CapnBry]

I was digging into why my image-based battery meter didn't seem to be working correctly.  Of my 5+1 images, The highest value was only displaying at 100%.  This thanks to the "dynamic battery meter" feature added by learman on 2006-08-26 (r1.61).  

Before you say the obvious "because he added a '-1' value to display 'unknown' values", that's not the cause.  The problem is that skip_conditional() counts the number of |s in a conditional, not the number of options in the conditional.  The code in get_tag() for ?%bl assumes you're passing it the number of options.  Take the following example:

Code: [Select]

%?bl<U|A|B|C|D|E>

skip_conditional returns that there are 5 |s (in enum), which is correct.  Note that we have 6 options (5 levels + 1 unknown). Now let's look at the get_tag code:

Code: [Select]

*intval = (enum - 1) * battery_level() / 100 + 1 + 1;

this tries to quantize the battery level to 4 values, instead of 5.

I'm posting this here for discussion instead of in bugs because it can be changed in one of three places:
1 -- Change the bl tag code to not subtract 1.  Easiest fix, no chance of regressions in other areas.
2 -- Change the conditional code to add one to the enum result of skip_conditional().  Affects all conditionals (currently only %?pv, %?bl).  This would affect the behavior of the volume conditional, in that now it shows the highest option only when volume is at 100%.  Changing it here would show the highest option for the full volume / N quantization.
3 -- Modify skip_conditional to return the count of options, not the count of pipes.  This is a slightly larger code change, which requires it to keep track of if the last character encountered before the > is a |.

#2 Seems like the way to go, but would like input before I formally submit the bug.

[Massa]

I remember that I also found some weird behaviour in skip_conditionals when I had a look at it:

As far as I remember
 - it does not take care of "%|" and does also count it as conditional separator (I'm not sure about that - it's been a while since I had a look at)
 - it's not able to handle tags with parameters inside conditionals (they also use the '|' as separator).
The second one is currently not relevant because there are no tags which are allowed inside conditionals which have parameters, but that may change in future (e.g. a tag for user ID3 tags).

So actually I vote for your #2 but I also vote for a complete redesign of the WPS parsing.

BTW, I already started this and started to write another token based WPS parser
(a first incomplete testversion for review can be found at FS#4826 — RFC: New WPS Parser)
- but nobody seems to care about

[bluebrother]

IMO the problem is that there are a lot of more important issues -- just think of the playback engine bugs. For myself, I started working on archived themes but (like your new wps parser) there seemed not much interest about. OTOH, if the main devs would follow every suggestion there won't be much time for fixing real bugs (and we would get more and heavier hiccups like that when the action code came in). So I agree with the devs to somewhat conservative decisions, especially if the feature in question is already working.
But I think we should keep working on such stuff as there actually is a chance to get stuff to cvs, it only is a somewhat long way. You always need to meet that people on IRC that are actually at least a bit interested in your ideas (which can be pretty hard)
(personally, I'd really like to see the in-menu-editing stuff by A_M added and similar, and I think those patches and ideas are really worthful -- in that case there was already a fairly amount of interest).

[LinusN]

Most often, not "caring" about a specific patch or feature is simply a matter of not having an infinite amount of time to spend on Rockbox.

[Lear]

I was digging into why my image-based battery meter didn't seem to be working correctly.  Of my 5+1 images, The highest value was only displaying at 100%.  This thanks to the "dynamic battery meter" feature added by learman on 2006-08-26 (r1.61).  

Before you say the obvious "because he added a '-1' value to display 'unknown' values", that's not the cause.  The problem is that skip_conditional() counts the number of |s in a conditional, not the number of options in the conditional.  The code in get_tag() for ?%bl assumes you're passing it the number of options.  Take the following example:

Code: [Select]

%?bl

skip_conditional returns that there are 5 |s (in enum), which is correct.  Note that we have 6 options (5 levels + 1 unknown). Now let's look at the get_tag code:

Code: [Select]

*intval = (enum - 1) * battery_level() / 100 + 1 + 1;

this tries to quantize the battery level to 4 values, instead of 5.

The above code may look a bit odd, but is in fact correct. Note that battery_level() returns a number between 0 and 100 (inclusive). In this case, enum is 5, so the highest value you can get from the "first" part is 4 * 100 / 100, which is 4. The lowest value is 0. 0, 1, 2, 3, 4 - that's five values, which is what you want.

Because of the way the values are used in the calling function, 1 needs to be added, so you get a value in the 1 - 5 range. The final "+ 1" is to allow room for "unknown level", yielding a 2 - 6 result.

I'm posting this here for discussion instead of in bugs because it can be changed in one of three places:
1 -- Change the bl tag code to not subtract 1.  Easiest fix, no chance of regressions in other areas.
2 -- Change the conditional code to add one to the enum result of skip_conditional().  Affects all conditionals (currently only %?pv, %?bl).  This would affect the behavior of the volume conditional, in that now it shows the highest option only when volume is at 100%.  Changing it here would show the highest option for the full volume / N quantization.
3 -- Modify skip_conditional to return the count of options, not the count of pipes.  This is a slightly larger code change, which requires it to keep track of if the last character encountered before the > is a |.

#2 Seems like the way to go, but would like input before I formally submit the bug.

What bug?

[Lear]

I remember that I also found some weird behaviour in skip_conditionals when I had a look at it:

As far as I remember
 - it does not take care of "%|" and does also count it as conditional separator (I'm not sure about that - it's been a while since I had a look at)

I'm pretty sure "%|" isn't counted...

- it's not able to handle tags with parameters inside conditionals (they also use the '|' as separator).

True, but that is a problem with the tags that take parameters. "|" is a poor choice for a separator there.

The second one is currently not relevant because there are no tags which are allowed inside conditionals which have parameters, but that may change in future (e.g. a tag for user ID3 tags).

So they'll need to use a different separator, or maybe support "%|" as a separator too.

[CapnBry]

The above code may look a bit odd, but is in fact correct. Note that battery_level() returns a number between 0 and 100 (inclusive). In this case, enum is 5, so the highest value you can get from the "first" part is 4 * 100 / 100, which is 4. The lowest value is 0. 0, 1, 2, 3, 4 - that's five values, which is what you want.

Well it didn't seem right to me that if you give it 5 icons, there's one icon which is only used when the battery is at 100, and 4 for the remaining 100 levels.  I was thinking that if I gave it 5 icons, it would quantize that down to 20% for each icon.  The more I think about it now, the more I realize it doesn't make a lick of difference.  If I want 20% per icon, I can just give it 5 + 1 + 1, one for unknown, and one for 100%.

[Lear]

By the way, my change didn't add the "only show the highest value at 100%" behaviour. It has been like that since enum support for the battery level was added, about a year ago.