tannerhelland / PhotoDemon

A free portable photo editor focused on pro-grade features, high performance, and maximum usability.
https://photodemon.org
Other
1.36k stars 200 forks source link

Evaluate Formulas in FormCanvasSize Spinners #263

Closed jpbro closed 6 years ago

jpbro commented 6 years ago

Hi Tanner,

I was just using PD and I thought of a feature I'd love to have. It would be handy if PD evaluated formulas in the Width/Height boxes on the FormCanvasSize form. For example, I had an image that was 287 pixels wide and I wanted 3x the width. It would have been helpful to just add "*3" to the end of the existing width and when focus is lost, PD would do the math for me (that is convert the value to 861).

I have a feature like this one of my apps and it often comes in handy. Since I us vbRichClient5, I just use the CEvaluator class to attempt to evaluate text in any number box when focus is lost. If it succeeds, it replaces the text with the evaluated result - if not, I just ignore the error silently. I don't think you want to add any references to third party libraries like that, so it might make things a bit trickier to implement, but there's probably some freely available evaluator classes floating around out there. It wouldn't need to be a very complicated evaluator anyway - addition, subtraction, multiplication and division would probably be enough.

If you decide to go ahead with this, it might be useful as a general feature of the pdSpinner control as opposed to just being available on the FormCanvasSize form - I've just mentioned FormCanvasSize here as a specific use case.

Thanks for considering this request.

tannerhelland commented 6 years ago

Thank you for the great suggestion, Jason. I love this! GIMP recently added something similar and I remember thinking "wow, that would be awesome, but I have no idea how to do it."

I imagine Olaf's implementation is a million times better than what I could do, but I'm kind of intrigued by trying to write a mini recursive descent parser for basic math expressions. I might give this a go and if goes horribly, I'll look at dropping in a vbRichClient-based solution.

Thanks again. Hope you are doing well.

jpbro commented 6 years ago

I'm doing well Tanner, hope you are too!

I wasn't aware of the GIMP feature, but I guess that just proves the point that it could be a useful addition for a photo/image editor :) Irfanview handles the need in a bit of different way with "Half" and "Double" buttons, but I think an evaluator would handle more possibilities at the expense of being less discoverable I guess.

Olaf's CFormula class (I incorrectly called it CEvaluator above - that's the name of my wrapper for his class that does some extra processing) might be heavier than necessary since it supports custom functions and variables. I don't think those features would be necessary for PhotoDemon, but it might be worth asking him if you could take a look at the code or even use it?

IMHO handling addition, subtraction, multiplication, division, and order of operations would probably be enough for the use cases I'm imagining, but maybe you have a vision for usage beyond what I've described above.

jpbro commented 6 years ago

Thought I saw a thread about evaluating formulas at vbforums recently, and I just found it here:

http://www.vbforums.com/showthread.php?860225-simple-math-string-parser

Olaf, Diletantte, Wqweto and maybe others all took a shot at it, so might be worth having a look here too.

tannerhelland commented 6 years ago

Wow, great vbforums thread - thank you for the link. Lots to study and learn from!

(And I am doing well too, thanks. Always too busy, but well.)

jpbro commented 6 years ago

Happy to help, and glad you are doing well :)

There's certainly no rush on this, and I know you're busy so take your time! If you'd like I'm willing to take a kick at it for you. Unless you had other ideas, I'd probably just start with Olaf's code from the vbforums thread and re-purpose it a bit (after confirming the licensing - I'm pretty sure he's mentioned that his posted stuff is free to use before, but I'll check in with him to make sure).

tannerhelland commented 6 years ago

I'll never say "no" to outside help, if it sounds like something that would interest you!

I'm happy to do the messy bit of integrating it into the pdSpinner control (PD's user controls are a mess of interlocking systems that no sane person should have to deal with, ha), but if someone already has a reliable parser+evaluator that's resilient across locales, that would be a huge help.

The only wonky PD-specific thing I need to deal with involves the portable nature of the app. Users in countries with multiple official languages previously ran into trouble when running PD from a thumb drive, because the same PD instance may encounter "." as a decimal separator, then an hour later, encounter "," as a decimal separator. (For example, a work PC is set to "." but a home PC is set to ",".)

To make things easier for these users, PD treats both symbols as valid and interchangeable, which typically requires me to modify 3rd-party code to allow both "." and "," as decimal separators, regardless of current locale settings. This is usually as trivial as replacing "," with "." prior to processing, then using locale-invariant string-to-number functions.

I think that's the only PD-specific thing I'd need to address; all other evaluator rules would be the same as any other Windows app, hypothetically...?

(And licensing is no problem, as long as the code's not GPL; that's the primary reason I can't incorporate certain code libraries into PD, and why I've been hesitant to attach vbRichClient to PD.)

jpbro commented 6 years ago

Sure, I'll give it a go :)

The locale stuff is something I've encountered before because in the main app that I work on users can define arbitrary locales for their projects that they're working on (on a project by project basis). The one benefit there is that I know what locale they are working in and which locale their project is "living" in, so it's easy to convert things like decimal places, thousands separators, currency symbols, etc... before performing math operations.

Your approach of converting "," to "." works well provided people aren't entering "," as a thousands separator. I would expect that this would be exceedingly rare in an imaging app (vs. an app that handles currencies as it's most common data type). So if that sole transformation step is satisfactory in your mind, then I'm happy to do only that. The other option would be to get the current locale thousands & decimal separators and then do transformations to the invariant locale before performing math.

I'm not sure if that necessary, nor do I know what users would expect to be the best approach in the situation where they're moving between locales like you've described - do they tend to "switch" their entry to the new locale, or do they tend to stick with one fixed locale in their mind? It's a bit thorny I guess, and probably changes from individual to individual. Maybe there's no perfect approach, only a "as good as it gets" one (as seems to be the case with most locale related stuff!).

I've pared down Olaf's example (to get rid of various functions and unnecessary operators) and performed a "," to "." conversion and it looks like it works well with one issue - there's no real testing for "bad"/non-evaluate-able formulas. I think that would be a handy thing to have to avoid getting "weird" calculations coming out when the user enters a formula that doesn't make sense like "123+*/344" - right now Olaf's evaluator returns 123 for the above, but I think it would be better just to error out (and skip the evaluation/replacement of the user's entry in cases like that).

tannerhelland commented 6 years ago

Your approach of converting "," to "." works well provided people aren't entering "," as a thousands separator. I would expect that this would be exceedingly rare in an imaging app (vs. an app that handles currencies as it's most common data type).

This was my hope, as well. I've never received a complaint from a user who wants thousands separators to be available, which obviously isn't a guaranteed sign of "good design", but perhaps "good enough design", as you say. ;)

In a perfect world, I'd probably prefer to use heuristics to parse a number - e.g. if multiple periods are present, try to parse them as thousands separators, or if mixed commas and decimals are present, see if either thousands/decimal system would result in a valid number - but I haven't sat down and thought this through.

(And of course, my current approach is entirely broken in some Arab countries, which use entirely different Unicode decimal separator characters - something I should probably deal with at some point!)

The other option would be to get the current locale thousands & decimal separators and then do transformations to the invariant locale before performing math.

I'm not sure if that necessary, nor do I know what users would expect to be the best approach in the situation where they're moving between locales like you've described - do they tend to "switch" their entry to the new locale, or do they tend to stick with one fixed locale in their mind?

As it was described to me, a recurring problem with portable apps and locales is perhaps best summed up by traveling - where it's not the user consciously switching entries between locales, but rather, having the app on a USB drive that gets plugged into different PCs with different locale settings.

So if e.g. an American travels to another country and plugs their USB drive (with PhotoDemon on it) into a public PC at a hotel or internet cafe, there is no guarantee how locale-specific system APIs will behave with either 1) text they enter during that session, or 2) text saved from previous sessions (which PhotoDemon needs to read from file and display back to the user, e.g. last-used settings).

It is possibly a bad design decision, but for last-used settings, PhotoDemon saves all user input as bare text. When loading last-used settings, it just dumps previously entered text back into text boxes as-is, regardless of whether the text is numeric or non-numeric in nature. The same goes for macros and other saved-as-XML data in the program.

Because of this, I can't ever rely on system or user account locale settings to convert text into numbers, because there is a potential for mismatch between the saved text format (e.g. the user saved a "fix camera lighting" macro on their home PC in Germany, and during a trip in America, they plug-in to a hotel PC, run PD, and execute that saved macro).

This is why whenever it comes time to actually convert text into a number (e.g. to apply a photo adjustment), PD just forcibly replaces commas with periods, then uses a locale-invariant "text-to-number" transforms. It's ugly, and as I mentioned, probably doesn't play nicely with things like Arabic locales, but I have yet to sit down and implement a better method. :/

(Ideally, I should probably allow the user to specify a locale for all numeric behavior, one that gets embedded in things like macro files so they can share the file with other locales without problem, but again, I haven't sat down and thought through the implications of this, tbh.)

So I guess I am 100% open to suggestions for a better method, lol. Like many open-source projects, much of PD was built with a "good enough for the moment, I'll figure out something better later" mentality - and now that I've been at it for quite a few years, the "later" part is finally catching up to me!

I've pared down Olaf's example (to get rid of various functions and unnecessary operators) and performed a "," to "." conversion and it looks like it works well with one issue - there's no real testing for "bad"/non-evaluate-able formulas.

You work quickly! Thank you for investigating this.

I agree that it would be great to identify invalid formulas so PD can mark them in the UI the same way it currently does (with a red box around bad entries). I have no idea how cumbersome that would be to implement, or if there's even a foolproof way to do it - but I imagine Olaf, as always, may have an elegant solution. (I hope... I probably shouldn't always expect that of him, but he's done it to himself by being so talented! :)

jpbro commented 6 years ago

This was my hope, as well. I've never received a complaint from a user who wants thousands separators to be available, which obviously isn't a guaranteed sign of "good design", but perhaps "good enough design", as you say. ;)

I think it probably is "good enough" since I don't think most people bother to type the thousands separators for data entry, especially for units of measurement like "pixels" and "inches", etc... It might be an issue with copy & paste from outside sources, but really I think the "problem" will be rarely encountered. I thought of some kind of heuristic detection code too, but I do think the efforts would be better spent elsewhere TBH. The arabic unicode replacement is probably worth doing though (&H066B if my research is correct?).

As it was described to me, a recurring problem with portable apps and locales is perhaps best summed up by traveling - where it's not the user consciously switching entries between locales, but rather, having the app on a USB drive that gets plugged into different PCs with different locale settings.

Ahh, I see - so user's tend to stay in a single mental locale, but their PCs can change under them. In that case I agree that using the locale settings won't be a good idea.

(Ideally, I should probably allow the user to specify a locale for all numeric behavior, one that gets embedded in things like macro files so they can share the file with other locales without problem, but again, I haven't sat down and thought through the implications of this, tbh.)

That's probably the only way to do things 100% reliably, but then you have to stuff a setting somewhere and hope users find it :) Maybe if it defaults to the current locale on first start and then uses that locale going forward unless changed manually that would work? The problem might be with existing users suddenly getting locked into a locale without expecting it and then behaviour changes for them.

So I guess I am 100% open to suggestions for a better method, lol. Like many open-source projects, much of PD was built with a "good enough for the moment, I'll figure out something better later" mentality - and now that I've been at it for quite a few years, the "later" part is finally catching up to me!

Even some closed source projects have this mentality at times - looks around sheepishly. If I think of anything better I'll put my two cents in, but "good enough" is often good enough for a long time..."Perfect is the enemy of good" after all :)

You work quickly! Thank you for investigating this.

My pleasure! I have something ready for you to try out but I'm just waiting on Olaf to clarify the licensing/give permission to use his part of the code.

I agree that it would be great to identify invalid formulas so PD can mark them in the UI the same way it currently does (with a red box around bad entries). I have no idea how cumbersome that would be to implement, or if there's even a foolproof way to do it

I've added a "CanEvaluate" method that returns True if the expression can be evaluated. It does this "heavily" by just running the "Evaluate" method on the passed expression and trapping any errors (returning True if there are none). It's a convenience function really, it doesn't do any guessing/checking as to whether the passed expression only looks valid. Since I don't think you'll be running thousands of tests in a loop or anything, I don't think the heavy approach should matter.

If there's a better way, I don't know it unfortunately!

but I imagine Olaf, as always, may have an elegant solution. (I hope... I probably shouldn't always expect that of him, but he's done it to himself by being so talented! :)

That wouldn't surprise me either - I've pointed him to this issue so maybe he'll swoop in and give you a killer solution but if not I'll pass on my modified version of his vbForums published evaluator as soon as I've been given permission.

tannerhelland commented 6 years ago

Thank you for the conversation, @jpbro! I agree on all counts, and I appreciate your help and input. This is going to be very helpful in the future if I can find some time to refine some of my text validation mechanics.

For now, I'll keep plugging away on some other things while I await your class with bated breath. ;) Thanks again, and please pass along my thanks to Olaf, regardless of the outcome of your conversation with him.

jpbro commented 6 years ago

My pleasure, thank you for the same! I haven't heard back from Olaf yet, but in the meantime one last question for you - since the effort on my end won't be a completely integrated piece of drop-in code PD, I'm not sure whether it makes sense to issue a pull request for it, or just to post it here as a comment, or email it to you? Essentially what I've done is create a new standard module with a single Evaluate(Expression) method (in case that helps you decide how you'd prefer to receive it). No rush to get back to me!

jpbro commented 6 years ago

I've heard back from Olaf re: using his code (adapted) and everything is OK with him. I'd still like to know how to get this to you since it's not code that is fully integrated with the PD UI, just the evaluator portion. Because of this I'm not sure it's worth doing a Pull request for it. Should I just post it here? Perhaps email it to you?

tannerhelland commented 6 years ago

Awesome, thank you for following up!

If you can, feel free to just attach the file to this issue. I like it when people have a chance to see outside contributions in their "virgin" state, before I mangle them with code style changes and deeper integration into PD.

(And if you haven't already, if you could stick Olaf's and/or your copyright and license details somewhere in the file, I would greatly appreciate it. That way, people who stumble across the attachment will still know who deserves credit for the work, even if they don't take the time to read this thread.)

jpbro commented 6 years ago

OK, here goes! As far as copyright/licensing goes, I'm happy to give my part away to the public domain, and Olaf has said the same to me about his part:

As for the Eval-Code - much of it (if you mean the latest speed-optimized version in that thread) ... all code I post in the forum is "free for the taking"

Here's the code:

Option Explicit

' This code originally by Olaf Schmidt http://www.vbforums.com/showthread.php?860225-simple-math-string-parser&p=5271805&viewfull=1#post5271805
' Modified and used with permission by Jason Peter Brown https://www.github.com/jpbro on August 10, 2018

' Changes:
'    Added Evaluate wrapper function to pre-evaluation processing to passed expressions
'    Simplified by removing "Function" support
'    Simplified by remove logical/boolean operator support
'    Raise errors for expressions that can't be evaluated.
'    Added "CanEvaluate" method that returns true if passed expression can be evaluated

Public Function CanEvaluate(ByVal Expr As String) As Boolean
   Dim l_Eval As Variant

   On Error Resume Next
   Err.Clear
   l_Eval = Evaluate(Expr)
   CanEvaluate = (Err.Number = 0)
   On Error GoTo 0
End Function

Public Function Evaluate(ByVal Expr As String) As Variant
   Dim P As Long

   If IsNumeric(Expr) Then
      ' Return the passed expression since it is just a number
      Evaluate = Expr

   Else
      ' Try to evaluate the passed expression

      ' Remove spaces, convert "," and arabic decimal separator to ".", and convert "--" to "+"
      If InStr(1, Expr, " ") Then Expr = Replace$(Expr, " ", "")
      If InStr(1, Expr, ",") Then Expr = Replace$(Expr, ",", ".")
      If InStr(1, Expr, ChrW$(&H66B)) Then Expr = Replace$(Expr, ChrW$(&H66B), ".")

      If InStr(1, Expr, "--") Then Expr = Replace$(Expr, "--", "+")

      ' Find ( with preceding numeric character and insert a * operator between
      Do
         P = InStr(P + 1, Expr, "(")
         If P > 0 Then
            Select Case Mid$(Expr, P - 1, 1)
            Case "0" To "9", "."
               Expr = Left$(Expr, P - 1) & "*" & Mid$(Expr, P)
               P = P + 1
            End Select
         End If
      Loop While P > 0

      ' Evaluate the expression
      Evaluate = Eval(Expr)
   End If
End Function

Private Function Eval(Expr As String)
   Do While HandleParentheses(Expr): Loop

   Dim L As String, R As String

   ' Check for evaluatable chunk and perform evaluation
   If Spl(Expr, "-", L, R) Then:   Eval = Eval(L) - Eval(R): Exit Function
   If Spl(Expr, "+", L, R) Then:   Eval = Eval(L) + Eval(R): Exit Function
   If Spl(Expr, "\", L, R) Then:   Eval = Eval(L) \ Eval(R): Exit Function
   If Spl(Expr, "*", L, R) Then:   Eval = Eval(L) * Eval(R): Exit Function
   If Spl(Expr, "/", L, R) Then:   Eval = Eval(L) / Eval(R): Exit Function
   If Spl(Expr, "^", L, R) Then:   Eval = Eval(L) ^ Eval(R): Exit Function

   ' Check for invalid alpha characters
   If Expr >= "A" Then: Err.Raise 5, , "Invalid expression.": Exit Function

   If Len(Expr) Then
      ' Test for some unevaluatable conditions
      Select Case Left$(Expr, 1)
      Case "-", "+"
         ' Check for non-numeric character after + or -
         Select Case Mid$(Expr, 2, 1)
         Case "0" To "9", "."
         Case Else
            Err.Raise 5, , "Invalid expression: " & Expr
         End Select

      Case "0" To "9", "."
         ' Numeric and "." characters are OK

      Case Else
         ' Unacceptable starting character (non-numeric, "+", "-", or ".")
         Err.Raise 5, , "Invalid expression: " & Expr

      End Select

      Eval = Val(Expr)
   End If
End Function

Private Function HandleParentheses(Expr As String) As Boolean
   Dim P As Long, i As Long, C As Long

   P = InStr(Expr, "(")

   If P < 1 Then
      If InStr(Expr, ")") Then Err.Raise 5, , "Invalid expression: " & Expr   ' Unmatched ) with no (
      Exit Function
   End If

   If InStrRev(Expr, ")", P) Then Err.Raise 5, , "Invalid expression: " & Expr   ' Unmatched ) before (

   HandleParentheses = True

   For i = P To Len(Expr)
      If Mid$(Expr, i, 1) = "(" Then C = C + 1
      If Mid$(Expr, i, 1) = ")" Then C = C - 1
      If C = 0 Then Exit For
   Next i

   If C > 0 Then Err.Raise 5, , "Invalid expression: " & Expr   ' Unbalanced parentheses

   Expr = Left$(Expr, P - 1) & Trim$(Str(Eval(Mid$(Expr, P + 1, i - P - 1)))) & Mid$(Expr, i + 1)
End Function

Private Function Spl(Expr As String, Op$, L$, R$) As Boolean
   Dim P As Long
   P = InStrRev(Expr, Op, , 1)
   If P Then Spl = True Else Exit Function
   If P < InStrRev(Expr, "'") And InStr("*-", Op) Then P = InStrRev(Expr, "'", P) - 1

   L = Left$(Expr, P - 1): R = Mid$(Expr, P + Len(Op))

   Do
      Select Case Right$(L, 1)
      Case "A" To "z"
         Err.Raise 5, , "Invalid expression: " & Expr

      Case "", "+", "*", "/"
         If Op <> "-" Then
            ' Expression contains an operator after another operator,
            ' and the operator is not a "-" which would indicate a negative number
            Err.Raise 5, , "Invalid expression: " & Expr
         End If

         Spl = False

         Exit Do

      Case "-"
         L = Left$(L, Len(L) - 1)
         R = "-" & R

      Case Else
         Exit Do
      End Select
   Loop
End Function
jpbro commented 6 years ago

Usage is very simple - call Evalute(AnyStringExpression) and the result will be evaluated and returned. Call CanEvalute(AnyStringExpression) to return True if the passed expression can be evaluated, or False if not.

tannerhelland commented 6 years ago

Thank you, @jpbro! The code is amazingly short for what it can do. The CanEvaluate is also extremely helpful, and I love your implementation of it - nice and elegant.

I've dropped the code into PD and tied it up to all numeric edit boxes. I have a few little enhancements I want to add to the UI, but the actual evaluator seems to be working perfectly as-is. I made a few tweaks to the code (hopefully that's okay) but the bulk of it is taken as-is from your work. One note on a bug I found, as you mentioned that you're using something similar in your code...

Inside the Evaluate() function, there's this chunk of code:

' Find ( with preceding numeric character and insert a * operator between
Do
    P = InStr(P + 1, Expr, "(")
    If P > 0 Then
       Select Case Mid$(Expr, P - 1, 1)`

The Mid$() expression throws an error if a parenthesis is the first character in the expression, e.g. "(1 + 2)". Changing the If statement above it to If P > 1 Then seemed to fix things up.

I'm excited to get this into user's hands! You're on a roll with great code/ideas lately. Maybe time to buy a lottery ticket?? :laughing:

jpbro commented 6 years ago

Good catch on the first-char parenthesis bug, and great work on the integration! I'm looking forward to having this feature in PhotoDemon, and glad I could play a small part in bringing it to fruition 😄

Lottery ticket might be a good idea...no doubt I could use the winnings ;)

PS: Your tweaks/changes to any code I submit are always OK by me 👍