After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 76616 - Size entry widgets could use some simple math
Size entry widgets could use some simple math
Status: RESOLVED FIXED
Product: GIMP
Classification: Other
Component: User Interface
git master
Other All
: High enhancement
: 2.8
Assigned To: GIMP Bugs
GIMP Bugs
: 574442 587460 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2002-03-27 09:24 UTC by Tuomas Kuosmanen
Modified: 2009-06-30 19:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Simple recursive descent parser / evaluator (9.70 KB, text/x-csrc)
2008-01-26 10:54 UTC, Fredrik Alströmer
  Details
Yacc file implementing a unit based measurement parser. (4.74 KB, text/plain)
2008-01-30 11:23 UTC, Simon Budig
  Details
Recursive descent parser / evaluator, gimpified and patched into libgimpmath (25.62 KB, patch)
2008-02-05 19:15 UTC, Fredrik Alströmer
none Details | Review
Patch to GimpSizeEntry allowing expressions in the spin-buttons. (6.50 KB, patch)
2008-02-05 19:18 UTC, Fredrik Alströmer
none Details | Review
fredriks-size-entry-parser.patch (33.16 KB, patch)
2008-04-12 07:35 UTC, Martin Nordholts
needs-work Details | Review
gimp-size-entry-expressions-parser-2008-12-04.patch (36.31 KB, patch)
2008-12-04 18:21 UTC, Martin Nordholts
needs-work Details | Review
revised and bugfixed (37.26 KB, patch)
2009-02-18 21:24 UTC, Fredrik Alströmer
none Details | Review
changed license to LGPL3, fixed indentation (10.49 KB, patch)
2009-02-18 21:54 UTC, Sven Neumann
none Details | Review
updated patch with license change, should have all files this time (36.97 KB, patch)
2009-03-08 14:10 UTC, Sven Neumann
committed Details | Review

Description Tuomas Kuosmanen 2002-03-27 09:24:06 UTC
Posted this to gimp-developer as well, but was told to file a wishlist bug
here as well. So here goes..

I have been thinking about this thing for a while already.. pretty often
I have a situation like this:

        * I will need to design a flyer or something, and try to fit it
          on a common papersize so it can be photocopied/color-lasered
          easily
        * But the size is too large, so I rather design a "cut A4 in
          three" (or US Letter depending where it will be printed)
        * A4 is 209.9028mm x 297.0389mm
        * My design would then be 209.9028mm x (297.0389mm / 3) which
          makes a nice flyer shape
        * It would be very very handy if I could type that into the New
          Image entry;  Height: [ 297/3 ] millimeters and Gimp would do
          the division for me and round it to the nearest pixel.
        * It would allow the very basic math like add, subtract,
          multiply and divide. This would be very useful for situations
          like the above.
Comment 1 Alan Horkan 2003-07-23 18:37:48 UTC
Changes at the request of Dave Neary on the developer mailing list.  
I am changing many of the bugzilla reports that have not specified a target
milestone to Future milestone.  Hope that is acceptable.  
Comment 2 Maurits Rijk 2003-07-24 08:48:30 UTC
Yeah, the size widgets should embed Gnumeric as a Bonobo component.
Just kidding :) I'm not sure how the user interaction would look in
this case. You have to consider things like users entering wrong
formulas and also the interaction with the spin-buttons. One option
would be to use the right mouse button (and maybe some shortcut) to
bring up a dialog in which you can enter the formula. The dialog can
only be closed if the formula is correct. Next, the calculated value
will be put into the entry.
Comment 3 Tuomas Kuosmanen 2003-07-24 09:22:30 UTC
Well, this should be really simple. Just basic math: plus, minus and
multiply and divide are the operations that are really useful for those.

So lets not over-engineer. Anything more complex does *really not*
belong there.

I think if you enter something invalid, Illustrator just cuts out the
invalid part, like if I type in "1234_234" when I mean "1234-234" it
just leaves the "1234" there. It does not really need a GUI for
entering any formulas since there does not need to be any support for
formulas. Just multiply/divide/add/subtract. 
Comment 4 Dave Neary 2003-07-24 10:35:14 UTC
Changing the milestone to 1.3.x for the time being to see if anyone
bites - I don't think this will get done for 2.0, though, it doesn't
seem to have a very high priority.

Dave.
Comment 5 Sven Neumann 2003-10-29 23:59:28 UTC
Bumping to milestone to 2.2.
Comment 6 Maurits Rijk 2004-05-24 07:34:22 UTC
We can either implement this ourselfs (this is more or less trivial) or use the
new GNU libmatheval (http://www.gnu.org/software/libmatheval/).
Comment 7 Sven Neumann 2004-05-24 07:45:41 UTC
That is a decision that has to be made by whoever writes the code. I doubt
however that the problem is trivial. The evaluation of the math is probably the
smallest problem. What needs to be figured out first is how to add this feature
to the widgets we use in a backward-compatible way or who's going to port all
users to new widgets.
Comment 8 Maurits Rijk 2004-05-24 08:34:55 UTC
Isn't this functionality going to be fully transparent in the size entry
widgets? Look to me that there won't be any api changes and/or new widgets.
Comment 9 Sven Neumann 2004-05-24 12:17:58 UTC
The size entry widgets use gtk spinbuttons and I am curious on how you want to
make GtkSpinbutton behave like this w/o breaking binary compatibility. Most
probably this isn't possible at all and a new widget needs to be introduced. I
don't think we should keep this bug on the 2.2 milestone. It should be bumped to
Future.
Comment 10 Maurits Rijk 2004-05-27 09:02:04 UTC
You're right, this should best be done by introducing a new widget
(GimpSpinButton). I'll change the target milestone.
Comment 11 Sven Neumann 2006-04-25 10:54:06 UTC
We should try to come up with a decent proposal for a widget that

 - is compact but good looking and functional
 - offers basic math functionality
   (addition, subtraction, multiplication and division)
 - handles units

Similar to GimpSizeEntry, it should be possible to combine several of these widgets.

Such a widget would be rather complex but we could use it all over the place and it would help us to make the GIMP user interface more functional and more compact. IMO this would be a good candidate for a Google SoC project.
Comment 12 Simon Budig 2006-04-25 12:31:11 UTC
Ok, some random thoughts off the top of my head:

- The widget should have a default unit that gets applied when the user specifies a number without qualifying it with a unit.

- Do we want to simplify the terms entered by the user? Or should the math expression stay at what the user entered? (Maybe normalized in some fashion, e.g. adding the default unit)

- Coming up with a grammar for math terms is not that hard, a recursive descent parser is not hard to implement if you know what you're doing. Also we could use lex + yacc, although this would mean another build dependency...

- what might be hard, is working out how the grammar would work with qualified numbers: "5mm + 3in" certainly is easy to interpret and "4 + 5 in" should be valid as well. But what does "5mm + 4" mean? Do we want to accept that? What about "5mm * 3in"?

Hmm. Maybe it is easy to put that in a grammar and allow only addition and subtraction on qualified terms.

- How would syntax errors be presented in the UI?
Comment 13 Kevin Cozens 2006-04-25 17:50:32 UTC
I've done this type of parsing before. You don't need the complexity of Lex/Yacc for these types of simple expressions.
Comment 14 Sven Neumann 2006-08-15 16:48:23 UTC
See also bug #346863. Perhaps the two reports should be combined.
Comment 15 Fredrik Alströmer 2008-01-26 10:54:23 UTC
Created attachment 103771 [details]
Simple recursive descent parser / evaluator

I thought I'd take a shot a this one, just created a very simple parser / evaluator, and I figured I'd try to integrate it into a widget this weekend (haven't coded gtk-widgets before, just used them). It does basic units, currently will accept 4 + 5in (in spite of the dimension error, the 4 will get the default unit, assuming the default unit is one dimensional, otherwise not), and it will also accept 5px*25px, but report the dimension as 2. The 4 + 5in is little bit more tricky than it seems, as dimensionless terms can't simply be assigned the default unit (consider the case (2+4)*5 in, which would give a 2-dimensional result if the default unit was just blindly assigned).

Feel free to point me in the right direction, which gtk-widget to work with, or even beat me to it.
Comment 16 Martin Nordholts 2008-01-26 11:30:27 UTC
Hi Fredrik

First of all, your efforts are greatly appreciated.

The problem however is not that we miss a parser for this, but rather that the whole unit system; widgets, core API and UI is quite crude, both from a user and developer point of view.

Rather than building a parser on top of the current unit code it would be preferable to basically rewrite the unit code from scratch. There have been some ideas floating around on the mailing list, but afaik no major work has been done yet.

Here is a link to a mail I posted to the maling list a while ago with some thoughts regarding this:
http://www.mail-archive.com/gimp-developer@lists.xcf.berkeley.edu/msg14814.html

You are of course free to finnish what you have started, but I think it would be better to redo the unit system rather than building on top of it. Also, it would be good to keep discussions regarding this on the mailing list to avoid bloating the bug report with discussions.
Comment 17 weskaggs 2008-01-26 17:59:10 UTC
Fredrik,

The main place where this would come into play is in the GimpSizeEntry widget, whose source code is in libgimpwidgets/gimpsizeentry.c, in the Gimp distribution.

Note that the goal here is, as far as possible, to give users the results they expect.  So, it seems to me that parsing 4 + 5in by using the default units for "4" would be wrong -- users writing such an expression would probably not expect the 4 to be interpreted as pixels or mm, for example.  It would be preferable to report a syntax error than to silently give results that users won't expect.  In short, don't aim for maximum generality, aim for maximum usability.
Comment 18 Sven Neumann 2008-01-27 10:12:31 UTC
Actually, the main goal is to get rid of the GimpSizeEntry widget.
Comment 19 Sven Neumann 2008-01-27 10:20:59 UTC
There are a few things to consider here:

(1) As far as I know Simon is also working on this so you two should coordinate.

(2) The code should take i18n into account and work for all locales supported by GIMP. I am not sure what exactly this means. But some of the assumptions made by the patch might have to be reconsidered.

(3) Redoing the unit system in a backward-compatible way may turn out to be quite difficult. So we should not block work on improving the existing widgets nor should we delay the development of new widgets based on the old unit system.

But let us please move this discussion to the gimp-developer mailing-list.
Comment 20 Simon Budig 2008-01-30 11:23:10 UTC
Created attachment 104016 [details]
Yacc file implementing a unit based measurement parser.

Yeah, I wrote a yacc based parser some time ago as well, I'll attach it for future reference.

I guess the ideas are quite similiar. Both do dimension tracking. There are slight differences though: My parser also accepts units without a number in front (assuming 1.0 then) which works quite natural. Also my parser tries to stick a unit to the value and return the result with a more-or-less-natural (mostly first used) chosen unit.

Of course not having a dependency on yacc is a nice thing to have IMHO and a clean recursive descent parser has a beauty that is hard to beat with yacc.

I have not done any work on the integration with the unit system of the gimp or a custom widget.
Comment 21 Simon Budig 2008-01-30 11:53:36 UTC
Hum, there actually was a problem with my grammar that caused 4-2+3 to be evaulated as 4-(2+3) which is not what one expects.

Gets fixed by swapping the two symbols in the yacc grammar:

term:     expr              { $$ = $1; }
        | term '+' expr     { $$ = unitsum ($1, $3); }
        | term '-' expr     { $$ = unitsum ($1, unitnegate ($3)); }
        ;

expr:     factor            { $$ = $1 ; }
        | expr '*' factor  { $$ = unitmul ($1, $3); }
        | expr '/' factor  { $$ = unitdiv ($1, $3); }
        ;

Comment 22 Fredrik Alströmer 2008-02-05 19:15:43 UTC
Created attachment 104499 [details] [review]
Recursive descent parser / evaluator, gimpified and patched into libgimpmath

As suggested, I gimpified the code, and patched it into the libgimpmath, I also created a patch for the GimpSizeEntry, however, since it's future appears to be unclear, I separated it into another patch.
Comment 23 Fredrik Alströmer 2008-02-05 19:18:08 UTC
Created attachment 104500 [details] [review]
Patch to GimpSizeEntry allowing expressions in the spin-buttons.

The patch for the GimpSizeEntry, what's really missing is perhaps different presentation (currently, on focus switch, the input validation runs and replaces the expression with the calculated value), and definitely user feedback in error situations (console printing is probably not the way to go...)
Comment 24 Martin Nordholts 2008-04-12 07:35:50 UTC
Created attachment 109105 [details] [review]
fredriks-size-entry-parser.patch

Fredrik I am sorry for taking so long to look at this.

I've looked at it now though and this seems to work very well. I definitely think this code deserves being commited to trunk so that we can get some broader input on it. Before we do though I have some feedback on the patch.

I have attached a patch that is a merge of the latest two from Fredrik. I have also made som minor changes like compile warning fixes, proper indentation of structs and making make check pass by properly updating the POTFILES-stuff. But the patch needs some more work.

 * Add a gimp_eevl_-prefix also to static functions (for better stack traces)
 * Collect static function prorotpyes in the top of the .c file and align them properly (look in other .c files for examples)

Also, I'm a bit skeptical about adding this to libgimpmath. May I ask who recommended it to put it there and why? I think we should keep this in the core. If we decide to make it public, I would like a more clear prefix than gimp_eevl, something like gimp_simple_number_expression_parser. Well that name sucked, but something better from an API point of view.

In any case that is some very nice code there Fredrik, thanks for the time you've put into this so far!
Comment 25 Martin Nordholts 2008-04-12 07:39:22 UTC
Um it just struck me that we can't keep the parser in the core since libgimpwidgets depend on the code. But I still think the parser should be internal and not be part of the public API.
Comment 26 Martin Nordholts 2008-05-29 16:51:42 UTC
We're close in getting this in for 2.6, but not close enough without a driving force. Setting milestone to 2.8.
Comment 27 Martin Nordholts 2008-12-04 18:21:09 UTC
Created attachment 123957 [details] [review]
gimp-size-entry-expressions-parser-2008-12-04.patch

Here is Fredrik's latest patch processed a bit. It is not finished for trunk but we are significantly closer.

I have:
* made a major cleanup including but not limited to renamings, restructuring, clarifications, simplifications, namespacing, interface documentation and purification.
* moved the parser away from libgimpmath and made it libgimpwidgets-internal instead.
* made the error strings non-translatable as I don't think it's proper to have textual error messages for parse problems.
* adapted the parser for Unicode.
* completely removed knowledge of units from the parser (that were ifdeffed away to start with) so that all unit resolution is handled through a callback.
* removed the variable substitution code as it creates a cleaner parser API and I doubt we will ever have any use of it in this context anyway. It is easy to add again if we need it.
* extended the test suite with more test cases.

But we still need to:
* Make % in the expression refer to image width/height just as % works in the unit combo box, i.e. typing 50% should evaluate to 50% of the width not to 0.5. This should be doable by only modifying GIMP's unit resolver.
* Make "(2 + 2/3)in" and "2 / 3 in" work. This requires adjusting the EBNF production rules and then adapt the parser accordingly.
* Add parse-error handling and feedback such as making the widget flash or beep or something.
Comment 28 Fredrik Alströmer 2009-01-09 12:07:46 UTC
Ok, so I've changed the EBNF (and adapted the code) to let arbitrary factors have units, which will allow (2+2/3)in.

However, I'm not able to figure out how 2/3 in could ever resolve to 0.666.. in (rather than 0.6666 in^-1), as the parser would have to have either some heuristic or other kind of context sensitivity. One possible solution would be to maintain a unit stack, and whenever we end up with -1, multiply with unit^2 to get the result. This would be a heuristic however, and will not always deliver the _intended_ result.

Allowing a textual user-feedback seems difficult, I added the gtk_widget_error_beep to flash the window and beep (system wide setting, I guess, it doesn't beep on my setup). It appears the standard way is to add a label and set the error message in there, but I don't think this is acceptable for the size entry, is it?

To have % resolve to percent of the width/height/whatever also doesn't seem feasible, as 1) the initial value is not actually known, and 2) it's not always available (new image dimensions for example).

My suggestion would be to not have 2/3in resolve to 0.666in, not handle %, and just let it beep/flash on error, it'll be more than sufficient for the original bug-report, anyone using it as before won't notice anything.

If anyone has got a better idea, please holler, otherwise I'll prepare the patch for this, with a few bug-fixes as well.
Comment 29 Fredrik Alströmer 2009-02-18 21:24:58 UTC
Created attachment 129014 [details] [review]
revised and bugfixed

Ok, so me again.. I did it pretty much as described above, except for two things. 

I found that GimpSizeEntryField->upper contains the '100%'-value (rather than an upper limit or something), so I made 100% to map that value, this means % is no longer dimensionless, and 50%+5cm evaluates to 50% of the size plus 5cm.

If the selected unit is percent, the process is reversed, 100% maps to 100 dimensionless, and 5cm is translated, given the current 'upper'-value and the current resolution, to the corresponding percentage. That means, if 100% = 50cm, 50%+5cm will evaluate to 50 + 10 = 60 (dimensionless).

Let the bashing commence...
Comment 30 Sven Neumann 2009-02-18 21:54:06 UTC
Created attachment 129017 [details] [review]
changed license to LGPL3, fixed indentation

As we updated the license to (L)GPL version 3 or newer in the meantime, I took the freedom to do that change in your patch as well. While I was on it, I've adjusted the indentation in gimpsizeentry.c.
Comment 31 Fredrik Alströmer 2009-02-19 05:51:40 UTC
Just one additional comment;

due to the transformations with percentages being done, you might end up in some weird behavior if you start multiplying. Like 5cm*50% will not give you 2.5cm, but rather one of two things 1) a two-dimensional value (50% gets mapped to a one dimensional value) which results in the expression not being accept, or 2) if the selected unit is percent, you'll end up with a dimensionless value (5cm being mapped to a percentage), which will be accepted and displayed, often giving a very large number.

Now the first case is not so bad (expression is simply not evaluated), the second case however might cause confusion.

Tried to post this comment yesterday, but bugzilla was having a rough night..
Comment 32 Martin Nordholts 2009-03-07 19:44:23 UTC
*** Bug 574442 has been marked as a duplicate of this bug. ***
Comment 33 Sven Neumann 2009-03-08 14:10:45 UTC
Created attachment 130279 [details] [review]
updated patch with license change, should have all files this time

Sorry, my last patch did not include the added file. Hopefully this one has everything we need.
Comment 34 Martin Nordholts 2009-05-17 11:54:03 UTC
So I've finally commited this now. What we have is very useful already. One thing that could be polished is propagating parse errors in a different way to the UI, but that's a minor thing we can handle outside of this bug report, so I'm closing this as FIXED now.


commit bcee243fa349c082e9405cf591291576bbf0581b
Author: Fredrik Alströmer <roe@excu.se>
Date:   Sun May 17 11:33:35 2009 +0200

    Bug 76616 – Size entry widgets could use some simple math

    Add a simple parser to the GimpSizeEntry widget so that one can write
    things such as "40in" and "50%" in a size entry widget and get that
    converted to the current unit.

    The parser also handles basic expresions such as "20cm + 20px" and
    "2 * 3.14in".

Comment 35 Sven Neumann 2009-05-17 21:37:04 UTC
Uh? But while this is very nice work indeed, it has serious usability problems. Please discuss this with Peter. I had talked to him and we came to the conclusion that an overhaul of all size entries and their unit combo-boxes is needed. I don't think we should keep this change without doing the other changes. So either we finish this for 2.8 or this change needs to be reverted.
Comment 36 Sven Neumann 2009-05-17 21:44:37 UTC
To explain this, the problem is that this change introduces the possibility to enter units into the size entries directly. This is great as this is actually where the unit belongs. But we still display it in a combo-box outside the size entry. That means that now there are two places where the unit is controlled. What we need to do is to move the unit into the size entries all over the place. That's a major change and it definitely needs careful planning. But it would be a major improvement. In particular because it has the potential to reduce the screen estate needed for size controls.
Comment 37 Martin Nordholts 2009-05-18 06:05:56 UTC
I agree that it would be very nice if the unit was in the text entries themselves, but this bug report is about improving the size entry widget, not to rewrite the unit system UI. That the size entry widget has serious usability problems is not news. We should not reuse this huge bug report for a separate huge issue so I'm closing this one again. What we need is a UI spec for this.
Comment 38 Michael Schumacher 2009-06-30 19:53:32 UTC
*** Bug 587460 has been marked as a duplicate of this bug. ***