Skip to content

Conversation

Henje
Copy link
Contributor

@Henje Henje commented Apr 9, 2022

I was checking out Ulauncher when I noticed that the builtin calculator lacks math functions as well as constants. This PR adds this functionality and corresponding tests.

I had to change the is_enabled regex to allow expressions to start with functions or constants. This also allows expressions like +3 to be picked up, whereas it was not before. From my limited testing this expansion does not conflict with other operations, but expressions like )+3 are picked up. This seems like a good trade-off as such a string seems like a mathematical expression and the calculator just emits an invalid expression error.

With mathematical functions expressions like sin(pi) are possible, which should be 0, but with machine precision it results in 0.xxE-16, therefore I added a rounding step after evaluating the expression.

Checklist

  • Verify that the test command ./ul test is passing (the CI server will check this if you don't)
  • Update the documentation according to your changes (when applicable)
  • Write unit tests for your changes (when applicable)

@friday
Copy link
Member

friday commented Apr 15, 2022

Thank you for this contribution. I'll try to have a look at this soon. Been busy with some bugs for the time I had to put on Ulauncher.

@friday friday mentioned this pull request Apr 17, 2022
@friday
Copy link
Member

friday commented Apr 17, 2022

First off, thank you again for your time and effort on this.

Ulauncher has a calculator mode, but it isn't a calculator. It was never intended to be a fully featured calculator like Qcalculate for example, and it still suffers from floating point issues (#1013), and as you know the Python constants are limited to 15 decimals because of being defined as floats, so there's already issues there. But in addition the calc mode mustn't get in the way of other functionality of Ulauncher such as application search and keywords, which limits it further as a calculator.

Supporting these functions and the constants is a nice addition however, and you demonstrated it can be done without adding a ton of complexity to the code, which is great. But this PR needs more work and tests to ensure it behaves consistently and doesn't break other functionality.

  • Many extensions use short default keywords and there are applications with short names or perhaps even if they don't have them it will still annoy people that typing "e" doesn't show a search for "Eclipse" or "Enpass" like they're used to, or that "pi" doesn't show "Pinta Image Editor". Maybe that's ok. It's a bit like the calculator already getting in the way for searching for "1password" or the game "0 A.D.", but I think it might be an improvement for a small minority of the users at the cost of confusion or annoyance for many, so I'm leaning on it not being such a good thing to support these constants isolated, without a context. In other words I think pi * 2 is fine to count as a calculator expression, but not pi.

    The functions also would get in the way of search, but for no benefit. Let's say you want to search for "Experience". "E" will show 2.71... So you type "Ex" which shows the search results, but maybe you have too many apps starting with "Ex" like "Extensions" and "Ex Falso", so you continue and type "Exp" which shows "Error", so you type "Expe", which still shows calc error because "Expe" is "Exp" followed by "E", which your regexp says is a calculator expression. This was the worst example I could think of of course, but you probably see my point.

  • Similarly, but worse: When you type in something followed by a space, that's how keywords works. Both shortcuts and extensions use keywords and this PR breaks keywords using mathematical operators, comma, dot, parenthesis, or the functions or constants you added support for. This is something users can add themselves as well as override the defaults, so I think we should expect there to be a few users that added e (e ), dot (. ) or exp (exp ) as a keyword for example, which this PR would break, and only to show "Invalid expression" for most of them.

  • 3e and e3 should either be supported to mean e * 3 or show as invalid. Currently it shows as valid because of the flawed fallback in normalize_expr() (fixed in V6 calc improvements #1014). If you rebase and fix the conflicts this should be ok.

  • The rounding you added actually pads the result. If the user types in something like 5.6 it adds 14 zeros to it. And it is not applied consistently to the result. Both of these are fixed in V6 calc improvements #1014.

  • I also think we should have one test for each of these functions, like you did for sqrt(2)**2') == Decimal('2') gamma(6)') == Decimal('120'). It's rather easy to add and it's great to ensure we don't break these if we make further changes.

2022-04-17_03-19
2022-04-15_19-31
2022-04-15_20-55
2022-04-15_20-56
2022-04-15_20-54
2022-04-15_20-56_1
2022-04-16_04-53
2022-04-15_21-05
2022-04-15_19-21
2022-04-15_19-22
2022-04-15_19-33
2022-04-15_23-42

@friday
Copy link
Member

friday commented Apr 17, 2022

Merged #1015 to v6. We can always tweak it if needed, but it seems pretty safe.

Thanks again. It was your work and you are listed as author for your commits :)

@friday friday closed this Apr 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants