calc() handeling of unitless numbers breaks decimals and causes rounding errors with fractions

[yuki-tsubaki]

I hope that I understood the request in If you have issues with themes in 1.7.0 please read this! · Issue #1397 · davatorium/rofi properly (that I should issues here rather than in the issues).

Situation

I was trying to use calc to position the rofi window in the center of the screen, while keeping the input bar in a constant position even when the number of lines is dynamically decreased in a way that makes it more straightforward to change when adjusting parameters like the number of lines and changing the element padding by showing the components that go into the offset.

Throughout the rest of my theme, I wrote the the padding in em with decimals (em because it is adaptive to the interface size (not just the screen size) and decimals because it is the only convenient way to set fractional values (as actual fractions require a calc function for each value.

Overview of Expected Behavior According to Documentation

The documentation about the calc function in the rofi-theme manpage was not very comprehensive (relative to the complexity that seems to be in its behavior), and didn't give any examples beyond, the what seems in hindsight, two of the most straightforward functions: min and -.
(I.e. I can't think of a situation where either of these functions would involve a value outside of the domain of the unit it is in. E.g. you would never need a decimal unit-less number as both all the inputs and outputs are in one of the input units.)

From what was there (a list of standard operations like +-/*% , some other documented functions, and the relevant order of operations), I thought that the calc function functioned a basic calculator where some values could have units.
From this, I would expect, for example:

• calc(5em/2) => 2.5em
• calc(5em*0.5) =>2.5em
• calc(1.5em*-16 / 2) => 12em
• calc(1.5em*-16*0.5) => 12em

As far as I know, the rofi-theme manpage established a few key things about distance units that are relevant.

1. em units should support any real value (although I don't think we have any way to enter anything beyond rational numbers given the operations in calc).
2. Distance numbers with no specified unit are interpreted as px.
3. Distance numbers in px must be integers.

What Actually Happens

• Unit-less numbers in calc seem to be interpreted as px
  As results of that:
• One cannot do math without some distance unit in every part, so doing something like calculating number of lines and multiplying them by a line height always comes with the caveat that internally, the all of the numbers in the line calculation are treated as px measurements.
• This means that unit-less sections of math must not include any non-integer items, and all fractional intermediate values will be rounded in some way.
• Calculations with cancelling units (e.g. calc(16em/1.5em) => 10 (with an assumed integer divide)) do not work (although this example is prescribed scope of the calc function in the manual (calculating sizes).

While these behaviors are consistent with the rest of the program, they certainly aren't what many would expect from a calc function. While I don't know if it needs to be fixed or just more explicitly documented, I do know that it took me about an hour of testing and rereading the manual before I really figured out what was going on.

(This was not helped by the error messages as apparently these error can cause parsing errors resulting in messages suggesting I was missing parentheses and or that I was somehow getting the syntax wrong for using the basic operations . I spent time messing with spacing and replacing negative numbers with subtractions from 0 (lol).)

A Few Examples of 'Weird' Things

(Yes, all of these are not 'weird' in that they are inconsistent with the rest of the system, but they are weird because they aren't what many would expect from a calc function, even if they have read the manual fairly diligently.)

1/2 of 500 = 0

y-offset: calc(1/2 * 500); => 0 This is a unexpected, but not absurd of because floor divides are things. It would at least be good to remind the user of this behavior.

em/em = px

    /* Equal */
    y-offset:               calc(-250em/1em);
    y-offset:               calc(-250);

Fractions work; Decimals don't

This includes the way I would have expected to solve my problem on the bottom and the way I found that works on the top.

    /* Works as expected */
    y-offset:               calc(-16em/2 * 3/2);
    y-offset:               calc(-1.5em*16/2);
    /* Breaks with `syntax error,  unexpected "Parent right (')')"` */
    y-offset:               calc(-16em/2 * 1.5);

This is the only thing that really feels 'broken.' I would expect to be able to use any type of number when using 'unit-less' numbers in my calculation, and then have the results rounded to fit whatever unit they're in. The current system, however, limits those numbers (used to represent lines or scaling factors) as if they were pixels which is limiting, and because of the lack of documentation/misleading error message, somewhat confusing.

[DaveDavenport]

Thanks for the detailed description.
Yes, this is something we should definitely fix!
I need to give it some more thought, my first idea is we should handle the pixel 'unit' as float and only 'round' when using them for sizing.
(distance_unit_get_pixel should return a float.)
This should fix the calculations.

[DaveDavenport]

Can you try the PR?

[DaveDavenport]

Ok, fix should be in calc-broken-fix branch.
Dit have to break syntax regrettable. % as modulo and % as percent broke stuff (kinda to be expected).
f.e.
100% - 10 .. is that 100 modulo -10 or 100% - 10..

Changed modulo to be 'modulo' now.

[DaveDavenport]

@yuki-tsubaki can you test this?

[yuki-tsubaki]

Yeah, I will test it today.

[DaveDavenport]

thanks!

[yuki-tsubaki]

    /* This still results in 0 offset */
    y-offset: calc(1/2 * 500);
    /* Still working */
    y-offset:               calc(-16em/2 * 3/2);
    y-offset:               calc(-1.5em*16/2);
    /* Still breaks with `syntax error,  unexpected "Parent right (')')"` */
    y-offset:               calc(-16em/2 * 1.5);
    /* Tested afterward to make sure it wasn't just a floor divide thing with some other int number  It also brakes when it is given as a decimal ( although the error about the unexpected thing element is the '*' operator itself) */
    y-offset: calc(0.5 * 500px);

The first test seems most decisive about weather it worked as it showed there is still intermediate rounding going on somewhere.
The last one also seems important as it shows the parser still doesn't like decimals in implied px measurements.

It seems I'm in the right branch because

> git branch --show-current
calc-broken-fix

The commits are showing in git log and I also used ripgrep to check that I have the changes in your commits and in the files and they're there.

Clearly something fishy going on here. I ran rg -e 'int.*pixel' and there's not a single line that suggests a pixel unit being handled as a int (except in the obvious case of converting rofi distance units into pixels). I would assume the problem has to be somewhere in the parser or other parts of the calculation. 🤷

[DaveDavenport]

(can you paste rofi -v output?)

[yuki-tsubaki]

I actually have a meeting right so I'll see if I can get it done really fast

[yuki-tsubaki]

Actually I notices forgot to run the patched version for that one

[yuki-tsubaki]

I know I did earlier for at least some of them, I will redo them all asap

[yuki-tsubaki]

Hopefully that was the problem

[yuki-tsubaki]

After a little bit of multitasking here are the results:
y-offset: -250; Gives a syntax error (unexpected ';' expecting number... ). The positive version works fine. This seems to be a regression, as when I run the same config with the my system installed version, it works as it should. It works in both versions when I wrap the number in calc first e.g. calc(-250).

On the good side, I retested all the ones that did not work and they do work (which is really good because the source looked very thoroughly fixed to me).

[DaveDavenport]

owh.. you are right. I changed the handling of negative numbers.. Moved it from the lexer to the grammar. I focused on the calc situation, not the normal one, will fix it later.
(thanks for testing, it really helps)

[DaveDavenport]

I've pushed another update. If you can, please test.

[yuki-tsubaki]

I just ran the tests and all of my test cases worked as expected with no errors.

[DaveDavenport]

Thanks for testing it out!
I will merge it back into the next branch.

[yuki-tsubaki]

The issue is resolved and the new behavior will be in the next release.