Support and General Use > Theming and Appearance Customization
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: ---%?bl<U|A|B|C|D|E>
--- End code ---
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: ---*intval = (enum - 1) * battery_level() / 100 + 1 + 1;
--- End code ---
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:
--- Quote from: CapnBry on September 13, 2006, 01:37:51 PM ---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: ---%?bl
--- End code ---
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: ---*intval = (enum - 1) * battery_level() / 100 + 1 + 1;
--- End code ---
this tries to quantize the battery level to 4 values, instead of 5.
--- End quote ---
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.
--- Quote ---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.
--- End quote ---
What bug? ;)
Navigation
[0] Message Index
[#] Next page
Go to full version