Numerical input boxes for canvas resize
ClosedPublic

Authored by jospin on Jun 14 2016, 5:22 PM.

Details

Summary

First integration of numeric parser spinboxes, for the moment just for the canvas resize function.

Test Plan

Open Krita, open an image and goto Image->Resize Canvas... You can then play around with the boxes, make them become red if you write a wrong expression (everytime the expression is valid you will see the size preview update, the corresponding offset and, if keep ratio is the to true, the other box and offset).

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped
jospin updated this revision to Diff 4469.Jun 14 2016, 5:22 PM
jospin retitled this revision from to Numerical input boxes for canvas resize.
jospin updated this object.
jospin edited the test plan for this revision. (Show Details)
jospin added a reviewer: Krita.
jospin set the repository for this revision to R37 Krita.
jospin added a subscriber: jospin.
woltherav rescinded a token.
woltherav awarded a token.
woltherav rescinded a token.
woltherav awarded a token.
woltherav added subscribers: dkazakov, rempt, woltherav.EditedJun 14 2016, 5:43 PM

Okay, the code looks good to my eyes, however, do make it a bit more friendly to other people, so either give certain variables better names than 'ok', or add some more comments on how to understand these functions :)

Another thing is one of style, so functions should be:

void class::function()
{
    code;
}

and if/for/while statements should look like:

if (statement) {
    code;
} else {
    code;
}

(note the spaces)
(no seriously, you will save someone from attempting to fix all those brackets in the future :p )

For commenting your code, if you want doxygen to pickup your explanation nicely, something like this should work.

/**
 * This is a class :)
 * @Param ok This tells us whether the parsing is going ok.
 * @Returns a double.
 */

And I'll leave the rest to @rempt and @dkazakov :)

libs/global/kis_numparser.h
36

please add some info about what bool ok is supossed to do. Especially when giving it names like 'bool ok' and 'bool oke'

jospin updated this revision to Diff 4471.Jun 14 2016, 7:47 PM
jospin removed R37 Krita as the repository for this revision.

Done: Correct style, add comments and do some small corrections to numerical boxes.

jospin updated this revision to Diff 4472.Jun 14 2016, 7:49 PM

Oups, diff in the wrong direction ^^'

jospin set the repository for this revision to R37 Krita.Jun 16 2016, 9:04 AM
rempt added a comment.Jun 17 2016, 8:02 AM

Excellent stuff :-). I am not really qualified to discuss the parser itself, but I think there should be unittests for it. The next step after that, I guess, is figure out how to use these classes in all numeric input situations.

libs/global/CMakeLists.txt
16

I'd put this in libs/ui instead, since that's where it's used.

I also think we are going to need a unittest for this code: it's fortunately very testable :-)

libs/ui/CMakeLists.txt
250

Once we're confident the code is stable, we probably should review what to do with related classes, like KisDoubleWidget, KisSliderSpinBox, KisMultiDoubleFilterWidget, KisMultiIntegerFilterWidget and similar classes.

Gimp beeps: we can do that too. Gimp's only other warning is some console output like:

ERROR: Expected number or '(' at 'a*b'

:-)

libs/ui/widgets/kis_doubleparsespinbox.cpp
124

A stylesheet is fine here, but I think we also will need an icon for extra visibility.

libs/ui/widgets/kis_doubleparsespinbox.h
56

s/usefull/useful/g

jospin marked an inline comment as done.Jun 17 2016, 9:18 AM

The parser will be used essentially in the ui lib in a first time, but may have applications elsewhere. Anyway, moving it is not a problem so it won't be in the future. I will put it in ui for the moment.
I have unittests written for the parser, but only in the prof of concept yet (I will move them to krita).

What is gimp beep ? My gimp never beep when I do an error in a numerical input box !? But I suppose it would be easy to play a sound when a numerical widget detec a new error (And I don't think it's a good idea, it's far too much invasive, a widget that become red + a warning icon should be ok, or we should let the user have an option to disable the sounds).

Speaking about icons, I think I found an elegant way to have a warning icon just inside the parseBoxes using a subwidget and some stylesheet tweak. I will try it.

More news soon.

jospin updated this revision to Diff 4606.Jun 18 2016, 10:39 AM

Icons in case of error and unittests.

I will review the code itself a bit later, but as far as I remember, the main benefit of the lexical-aware input boxes was the ability to type 'px' ar 'cm' or 'mm' dimensions in it. That is really a must. It is even more needed than the maths itself.

The dimensions can be implemented in two ways: 1) dimension may appear only in the end of the string (simple solution); 2) dimension may appear in any part of he string and will use the 'multiplication' priority rule (more complex solution). Both ways are acceptable.

It might be a good idea to look into boost::spirit (especially if you decide to use the second way of working with demensions), though I guess it is possible to use any approach if it is stable enough.

I find math more important than unit conversion, but I suppose it's a question of preference and workflow. (But it's not useless, cf the calc function in css).

Unit at the end of the string are not that much usefull since we have unit combo boxes where it's needed.

So if we choose to work on it it must be within the expression but there is some problems like:

  • what does cos(2 cm) or exp(3 inch) mean ?
  • do we need to allow unit under a division operator ? 3cm / 2cm is unitless, so how do we convert it back to the current unit in use ?

Is there a solution ? Yes, we need to let our parser be aware if it should treat unit or not, and what is the current unit in use (by connecting the widget to the unit combobox for exemple). Then the parser will treat recursively the expression and disable units when treating a function other than abs() or () and when treating a denominator.

A switch to tell the parser that we enter a trigonometric function (cos, sin, tan) is also a good idea, so the parser will convert grad and rad to deg ! (to work with radian having pi as a constant is a good idea, and if we have pi, phi would be also a good idea since it's important for compositions).

But I think all of this can be implemented in a second time.

Concerning the use of boost::spirit I'm not convinced. Writing a math parser is not that hard, so I'm not sure if it's a good idea to add dependencies just for that. For the moment I use just a serie of recursing functions and Qt's regex system (since Qt is already a dependence of krita).

In D1875#36021, @jospin wrote:

I find math more important than unit conversion, but I suppose it's a question of preference and workflow. (But it's not useless, cf the calc function in css).

Unit at the end of the string are not that much usefull since we have unit combo boxes where it's needed.

They are extremely useful! The user can change the dimension without touching the mouse. That is really important among the professional users who have to do a lot of routine tasks.

So if we choose to work on it it must be within the expression but there is some problems like:

  • what does cos(2 cm) or exp(3 inch) mean ?
  • do we need to allow unit under a division operator ? 3cm / 2cm is unitless, so how do we convert it back to the current unit in use ?

    Is there a solution ? Yes, we need to let our parser be aware if it should treat unit or not, and what is the current unit in use (by connecting the widget to the unit combobox for exemple). Then the parser will treat recursively the expression and disable units when treating a function other than abs() or () and when treating a denominator.

Good point! I think you are right and this feature is not worth the complications we get due to inconsistent dimensions. The always-visible suffix in the end of the box is the best and the simplest choice.

A switch to tell the parser that we enter a trigonometric function (cos, sin, tan) is also a good idea, so the parser will convert grad and rad to deg ! (to work with radian having pi as a constant is a good idea, and if we have pi, phi would be also a good idea since it's important for compositions).

"rad" and "deg" suffixes are a good idea. But it should be available only as "a suffix to the sin/cos expressions". That is, it shouldn't be allowed to be mixed in the internal expression, but only as a suffix of the sin/cos param. Though I guess it would need a formal grammar for that...

But I think all of this can be implemented in a second time.

Well the dimensions suffix is really a must. To integrate the parser into our input boxes we will need to refactor a lot of code Basically, all the places where these input boxes are used. And the presence of this feature decides if we should also drop those dimensions combo-boxes or not.

Though the strategy might be the following:

  1. Write arithmetics-only patch for Canvas Size dialog only (what your patch does already). And push it into a branch.
  2. Write dimensions parsing, remove the dimensions combo-boxes and push it as a separate patch.
  3. Refactor all the places in Krita to use the new input box

The only question here is whether we do the 'rad'/'deg' stuff and whether this grammar can be implemented with a simple approach of QRegExp+recursion.

Concerning the use of boost::spirit I'm not convinced. Writing a math parser is not that hard, so I'm not sure if it's a good idea to add dependencies just for that. For the moment I use just a serie of recursing functions and Qt's regex system (since Qt is already a dependence of krita).

Well, we already depend on Boost, so using boost::spirit doesn't add any new dep for Krita. The question is whether it is possible/easy to implement the stuff like 'deg'/'rad'/'pi' without defining a formal grammar. If yes, I'm fine with not using it :)

They are extremely useful! The user can change the dimension without touching the mouse. That is really important among the professional users who have to do a lot of routine tasks.

I suppose a lot of krita users work with 1 hand on the keyboard and hold a digital tablet pen in the other. And combobox are useful in this situation.

But we can implement a signal/slots mechanism to allow some shortcuts to change the unit without using the mouse/pen.

Good point! I think you are right and this feature is not worth the complications we get due to inconsistent dimensions. The always-visible suffix in the end of the box is the best and the simplest choice.

One the other side it may be really convenient to be able to use syntaxes like "33% + 20px", like in the css calc function, especially when building compositions. For example if I know I have a margin of 1.5cm, then my comic box is one third of my page and I want to compute the another box to fill the rest of the page with respect to the margins I can write: "100% - (3*1.5cm + 33%)".

Also there would be some problems if we have only a suffix, for example if I have a spinbox with 120px and I want to get the value in inches. with the combobox I just need to select inches in the combobox but with a suffix, do I need to write "(120 px) inch" ?

Another advantages of combobox are that we need less refactoring and it's more comfortable for non-pro user who doen't know the list of units.

But as I said it's possible to have a mechanism to do all with the keyboard within the spinbox, for example If I end my formula with a unit separated by a space like "10 cm" then the spinbox send a signal to change the unit.

"rad" and "deg" suffixes are a good idea. But it should be available only as "a suffix to the sin/cos expressions". That is, it shouldn't be allowed to be mixed in the internal expression, but only as a suffix of the sin/cos param. Though I guess it would need a formal grammar for that...

From the code I imagine it would be easier to implement it within the function, It would be better to have distance unit outside functions to be able, for example, to write "(23 + 2*cos(45) ) cm" and get all the result converted from cm to the current unit.

NB: "(...)" is the identity function in my parser, this allow to treat parenthesis just like functions.

Well, we already depend on Boost, so using boost::spirit doesn't add any new dep for Krita. The question is whether it is possible/easy to implement the stuff like 'deg'/'rad'/'pi' without defining a formal grammar. If yes, I'm fine with not using it :)

I will take a look at boost::spirit, but I already have plenty of ideas to implement all of that without it :)

Just a few comments:

"33% + 20px" is a really nice idea. Not sure how to implement it without the grammar though.
If we have a grammar we can just disable putting the dimensions into the denominator and the parameters of sin/cos. So the user can only multiply by a dimension only.

"(23 + 2*cos(45) ) cm" is not a good idea, because it makes the user type too many "shifted" symbols. The ideal solution should work like that: "23 + 2*cos(45) cm". And only if we need to do an explicit unit conversion, then we should add a parenthesis: "(23 + 2*cos(45) cm) px + 100px"

Ok, I've thought a bit about it. I think the best solution would be that unit operators have a smaller precedence than multiplications and divisions but a greater precedence than addition and subtractions.

So one can write "33% + 20px" or "33% + 1.5*20 px" but not "33% / 20px"... Unitless numbers are considered to be of the active unit, for example if my box is in inch "23 + 2*cos(45) cm" mean "23 inch + (2*cos(45) )cm".

Then we can add global conversions operators that allow to change the active unit in the box. I was thinking about '>', '->' and '=>' (or maybe '#', '#:' and '#=').

'expression > unit' would mean "consider the active unit of the expression is unit but don't change the unit of the box", 'expression -> unit' would mean "Treat the expression with the active unit then change the active unit" and 'expression => unit' would mean "change the active unit then treat the expression".

So then "23 + 2*cos(45) => cm" would mean "(23 + 2*cos(45)) cm", "23 + 2*cos(45) -> cm" would mean "((23 + 2*cos(45) )inch ) cm" (both would change the active unit to cm) and "23 + 2*cos(45) > cm" would also mean "(23 + 2*cos(45)) cm" but wouldn't change the active unit.

It would then be possible to have a signal/slot mechanism to connect the spinbox to a unit combobox but also a switch to let the spinbox show the unit by itself (depending where in the code we use it).

On a longer therm we could develop a special widget that is sensitive to an option in the app that let the user switch between a keyboard friendly or a pen friendly mode. (it's just an idea, not sure it's a good one).

All of this may seem very complicated. We can also do like in scribus and configure the active unit for the whole document. Then we don't need to manage an active unit for the boxes and can use a simpler convention. But I have to admit I don't like the scribus way of doing things since I often need to change the units. (And it would imply a lot of refactoring which is not a good thing).

I think people use different units in different contexts, so it wouldn't be good to do it the scribus way. In fact, scribus simply infuriates me with that feature :-)

Oh, and I'd like you to apply for a dev account -- give my name as reference. Then you can push this code to a branch, making it easier for artists to test!

Commits have been pushed to the branch numericalboxes. All seem's to be correct (I hope). I extended the usage of numericalboxes to all the imagesize plugin. As we get confident the code work well I will extend it to other part of the code.

dkazakov requested changes to this revision.Aug 1 2016, 6:14 PM
dkazakov added a reviewer: dkazakov.

Hi, @jospin!

The patch seems to work fine. Except that I expected it also support auto-changing dimensions when put in the end of the line :)

Codewise, there are several problems with coding policy:

  1. #include "kis_intparsespinbox.h" In krita we use underscore split words, so the filenames should look like: #include "kis_int_parse_spin_box.h"
  2. In KDE we use 4-space code indentation, not tabs. See the KDE policy
  3. In some of the places you use python-style member function naming and in the others, c++-style. I mean
class KRITAUI_EXPORT KisDoubleParseSpinBox : public QDoubleSpinBox
{
...
		QMargins _oldMargins;
		bool _areOldMarginsSaved;
};
  1. I'm not sure if it is ok that you moved the classes like KoStrokeConfigWidget into kritaui. @rempt, what do you think about it?

Please fix the styling issues and update the patch in the review by doing in your branch:

arc diff --update D1875 origin/master
This revision now requires changes to proceed.Aug 1 2016, 6:14 PM
jospin added a comment.Aug 2 2016, 7:39 AM

Concerning changing dimensions, as I said to Rempt in an email, it's a work in progress. The basis is there (the KisDoubleParseUnitSpinBox) but it won't be able to change the unit on the fly for 3.0.1 but for the next version.

Concerning the classes that were moved to kritaui the other solution is to move all the math parsing classes to kritawidgets. (Or to accept some widget won't have the math parsing abilities, but this is not a good idea since thoses widgets are used in the vector tools... so basically where it's the most usefull to do math).

rempt added a comment.Aug 2 2016, 8:34 AM

Codewise, there are several problems with coding policy:

  1. #include "kis_intparsespinbox.h" In krita we use underscore split words, so the filenames should look like: #include "kis_int_parse_spin_box.h"

That, or new-style CamelCaps.h

  1. I'm not sure if it is ok that you moved the classes like KoStrokeConfigWidget into kritaui. @rempt, what do you think about it?

That is a problem, since it's used libs/basicflakes, which should not link to krita/ui -- it would be better to move the new widgets to libs/widgets.

jospin added a comment.Aug 2 2016, 9:24 AM
  1. I'm not sure if it is ok that you moved the classes like KoStrokeConfigWidget into kritaui. @rempt, what do you think about it?

That is a problem, since it's used libs/basicflakes, which should not link to krita/ui -- it would be better to move the new widgets to libs/widgets.

Should I move the numericparser in lib/widgetutils or in libs/global ?

rempt added a comment.Aug 2 2016, 10:00 AM
Should I move the numericparser in lib/widgetutils or in libs/global ?

lib/widgetutils would be better.

jospin added a comment.Aug 2 2016, 9:56 PM

@dkazakov What do you mean by python style ? You mean I shouldn't use the private member underscore indicator ?

To correct the identation problem do you think the command:

grep -irlP '\t' --exclude-dir=.git | grep -E '(\.h)|(\.cpp)$' | xargs -I @ sed -i 's/\t/ /g' @

Is safe ? Apparently I'm not the only one who has an IDE configured to use tab ! There are tabulations in a lot of files I did not touched.

rempt added a comment.Aug 3 2016, 8:26 AM

That also needs to run on .cmake, CMakeLists.txt and .cc files, but yes... We'd better do another round of indentation cleanups! I had wanted to do a big astyle type or clang-format cleanup when we moved out of calligra.git, but that never happened, but we have to get some kind of consistency!

rempt added a comment.Aug 3 2016, 8:27 AM

It might be best, though, to do that right after the merge window closes, on master, so the smallest amount of branches are active.

jospin added a comment.Aug 3 2016, 8:29 AM

Ok, so I'll only clean up my main files (spinboxes and numparser). If there was a tab in another file it will go in this cleanup.

rempt added a comment.Aug 3 2016, 8:33 AM
Ok, so I'll only clean up my main files (spinboxes and numparser). If there was a tab in another file it will go in this cleanup.

Yes, please :-)

jospin updated this revision to Diff 5658.Aug 3 2016, 8:53 PM
jospin edited edge metadata.
  • [numerical boxes] first working version (only for the resize canva dialog for now).
  • [numerical boxes] Improve the behavior of parseSpinBoxes (tested with resize canva function).
  • [numerical boxes] Correct style, add comments and do some small corrections to numerical boxes.
  • [numerical boxes] Allow to use floating point in int calculationsbefore flooring.
  • [numerical boxes] Icons for warning in parseSpinBoxes.
  • [numerical boxes] Add a test for simpleMathExpressionsParser.
  • [numerical boxes] Implement a test for the parserSpinBoxes and correct what this test shown was wrong.
  • [numerical boxes] improve parser structure.
  • [numerical boxes] Use the numerical boxes in all the imagesize extension.
  • [numerical boxes] add license information to test files.
  • [numerical boxes] add parsespinbox to the offsetimage extension.
  • [numerical boxes] Manage prefix and suffix in error cases.
  • [numerical boxes] make use of parsespinboxes in the forms of the tools option widgets.
  • [numerical boxes] treat all the forms in the ui/form folder.
  • [numerical boxes] assert that all the functions from the image and layer menu use numerical boxes.
  • Let the slider spinboxes use the math expression parser.
  • Create a replacement for KoUnitSpinBox that is able to parse math expression.
  • Use the parsespinboxes on all the dockers.
  • Move the classes that use KoUnitSpinBox from lib/widget in lib/ui/widget.
  • Let the widgets previously in kritawidgets use KisDoubleParseUnitSpinBox.
  • Let all the filters use the math parser spinboxes.
  • Some little changes in kritaui regarding the use of KisIntParseSpinBox.
  • Let the karbon plugin use the math parsing abilities
  • Transform the spinboxes in parsespinboxes in the paintops plugin.
  • Fix an annoying bug whith the specific color selector and the parsespinBoxes.
  • Let a few last place of the code use the numercic parse spinboxes.
  • Display the warning sign when a parsespinbox is in error state only if there's enough place.
  • Correct code and coding conventions prior merging numericalboxes.
jospin updated this revision to Diff 5683.Aug 4 2016, 6:58 PM
jospin edited edge metadata.

Let krita parse simple math expressions in numerical fields.

The code provide two classes to replace QSpinBox and QDoubleSpinBox: KisIntParseSpinBox and KisDoubleParseSpinBox.
It also provide a classe to replace KoUnitSpinBox: KisDoubleParseUnitSpinBox. In the future this class should also allow to perform unit conversions on the fly.
It also gives some math parsing abilities to older classes lis KisSliderSpinBox and some others.
It provide unitests to quickly check the behavior of the parser and the spinboxes.
It let all parts of krita use thoses new classes when needed.

Ref T1621

jospin added a comment.Aug 4 2016, 7:01 PM

Sorry for the new revision, I spotted a problem a bit too late. It was 2 .ui files that were not back at their correct places. I've checked git history there shouldn't be another problem of this kind :S. (The trick is that it did not break unittests of the build)

If you see other problems say it to me, so I can solve them before merge window close (or we'll wait until next version).

dkazakov accepted this revision.Aug 5 2016, 9:28 AM
dkazakov edited edge metadata.

Hi, @jospin!

Your patch looks perfectly ok (except of a few left tabulations of course). I have pushed the tabulations fix into your branch and added a comment with the script how to fix it in the future.

The only concern I have is that commented-out line in KisAbstractSliderSpinBox (see the attached comment). Either delete this line or uncomment back :) And then just push your branch :)

Though is you are going to merge it with arcanist using my manual, please be careful, it might happen that you will have to first update the review to include my commit into the final merge.

libs/ui/widgets/kis_slider_spin_box.cpp
98 ↗(On Diff #5683)

Is this line ok? Looks like a forgotten debug

This revision is now accepted and ready to land.Aug 5 2016, 9:28 AM
jospin updated this revision to Diff 5692.Aug 5 2016, 12:20 PM
jospin edited edge metadata.

As said, @dkazakov commit and remove the useless line.

  • Fix whitespace/tabulations problem
  • Remove single commented line.
This revision was automatically updated to reflect the committed changes.