Replace QDoubleSpinBox by QLineEdit

[svengoldberg]

This is a draft PR, do not merge!

General information

This PR's base branch is branch that should be merged into main, as well. I started working there and did not see any advantage to move to another one for this example case...
Only one input field is changed, namely the "concentration" input of the add-lightsource-dialog. The window can be found under "View->Display->Add a spotlight". Here, the direct contrast can be seen to the currently used QDoubleSpinBox.

Now, to the

Implementation:

This is a suggestion for the replacement of the QDoubleSpinBox by a customized version of the QLineEdit for every occurrence within the TiGLCreator. We discussed internally that we want to find another solution for user input of double values

Since the QLineEdit is for text in general and not specified for number input, some restrictions and special calls are needed. For convenience, they are put into the derived class TIGLDoubleLineEdit. This allows easier access of this object when it is used for double input.

Moreoever, the QLineEdit (or its derived class) needs a validator object to only allow wanted input. This object, e.g., automatically blocks input that cannot be converted into a double. For some reasons, the implemented value-range control did not work for me. So, I implemented a customized validate() function. This is used by the Qt object QDoubleValidator. In order to customize the function, I derived the class TIGLDoubleValidator and overwrote the function to only allow string input that can be converted into double within a developer-defined value range.

What do you all think about the suggestion @joergbrech @merakulix @ole-alb @AntonReiswich ?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.75%. Comparing base (ae19c7c) to head (d521b8d).

Additional details and impacted files

@@                Coverage Diff                @@
##           add-light-source    #1252   +/-   ##
=================================================
  Coverage             72.75%   72.75%           
=================================================
  Files                   308      308           
  Lines                 26993    26993           
=================================================
  Hits                  19638    19638           
  Misses                 7355     7355           

Flag	Coverage Δ
unittests	72.75% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[merakulix]

Hi, I tested it under Windows. I guess my opinion on the spinboxes shifted a little.
Basically: I'd say it works on the outside as we talked about, but I realize, that I don't feel the behaviour changed for me. And on the inside, I'm not sure.

This is extra code to maintain ourselves and as I understand it rewrites functionality the spinboxes already had, also, the write-in-the boxes behaviour didn't increase (As I see it, I hope I didn't miss anything you added?).

Upside of the changes:

• No unintentional changes of numbers when hovering over the spinboxes - Alternative: I believe we could just not allow the 'editable on hover'- behaviour, and still have spinboxes, if we "decide not to like" this error prone functionality
• no more seemingly "ramdom" predefined steps - Alternative 1 : Add an option (e.g. spinbox) to adjust the steps, for me, this seems intuitive - Alternative 2: The option to write into the boxes is already there.

To sum up/the difference I see:

1. The ',' (depending on language) became a fixed '.' (wether this is improvment is subjective, I like the '.')
2. Increase/Decrease buttons on each spinbox yes or no? (My mouse-hand likes the buttons, my keyboard-hand doesn't feel disturbed by them, my brain maybe asks "why this stepsize?" )
   The stepsize is default and not based on any rules. But maybe somebody still thinks they are useful? I can't really say. Do those up/-down buttons really hurt anybody who doesn't like to use them?

My conclusion: The current functional behaviour of the spinboxes is not perfect but now I don't think it's worse than a lineedit, but already implemented-> no extra code, no replacing code.
There is also the possibility to optimize usage and behaviour.
The questions raised are very subjective and answers depend on usage.

[joergbrech]

I actually like the idea of replacing double spin boxes with LineEdit-like widgets. I am a little bit bothered by the arbitrary step sizes of the up and down buttons and the involuntary value changes when accidentally hovering a widget and using the mouse wheel.

There is some extra code involved, but that is just the price one has to pay for customization. And the code can directly be re-used everywhere where we have widgets for entering doubles, which now is all over the place.

@svengoldberg, we talked about it, but I forgot. What does the custom DoubleValidator do again?

I played around a bit here: 3fb1afb. This is a version, where a user directly sees a difference in edit-mode and in view-mode and values are rounded in view-mode. It is actually a QLabel that gets replaced by a QLineEdit when the widget gets focus. When we enter edit mode, we see the whole unrounded value and the value is selected, so that typing replaces the current value. When pressing escape, edit mode exists without overwriting the value. When pressing enter or loosing focus (for instance by pressing tab), the value is accepted. If it is outside of the valid range, the value gets clamped. What do you think?

[svengoldberg]

@merakulix

I don't feel the behaviour changed for me

I am confused about that. Wasn't it possible for you to add arbitrary many digits in the new user input field? This was the first intention we had at all that made us want to remove the QSpinBoxes. Especially the point that a pre-defined entry comming from the system did not let the user put in a decimal place (even at the start) when all of them were already in use.

Regarding the scrolling:
This indeed was one of the two (IIRC, maybe it was even more) pain points that we did not like. I personally find that unintended changing of values by scrolling very annoying, combined with a 'risk' from a user-perspective of loosing the values entered there.
I was not aware of the fact, that one can deactivate the "changing-values-by-scrolling"-feature! We should definitely get rid of that

Regarding default steps:
I personally cannot think of a use case in which one wants to customize a default step for these values. In that case, one had to change this value permanently to really make use of it. To me, it seems over-engineered. In my opinion we do not loose any functionality by avoiding QDoubleSpinBoxes.
When it comes to QSpinBoxes, which deal with int, this is differently. I think, here the SpinBoxes indeed can be kept and one can make use of the arrows (without scrolling...) !

Finally: I was not aware of that one can deactivate the "scrolling-issue". That was indeed one of the big downsides for me. Nevertheless, the user input becomes much more intuitive with the QLineEdits, since the number of decimal places are handled in a much nicer way. It is possible to define a default value with (e.g.) 2 decimal places. ANY other amount of decimal places is totally fine during the user input. That is currently not the case. And the most annoying part is also moved out of the way: With the QLineEdits, no decimal places at the end need to be deleted, before adding some significant digits in the first places of the decimals.

[svengoldberg]

@joergbrech
That looks really cool! The clamping does not seem to work fully correct (at least the "view-mode" value stays wrong). But that is surely something, we can fix.
To be more precise: When entering a value larger than 1 and leaving the field with tab, the value stays larger. After returning there by clicking in this area, the larger value still remains. When "iterating" over all boxes by tab and returning in the concentration box again, the value is then clamped. With tab it is even possible to leave the box with an empty input. But, that can also be just a difference between "frontend"- and "backend"-mode or -value (I did not have a real look into the implementation yet)

What does the custom DoubleValidator do again?

From my point of view, the QDoubleValidator did not work correctly and allowed wrong values within the box. Hence, I overrode the validate function to cut off values that are outside the range

[merakulix]

@svengoldberg

This was the first intention we had at all that made us want to remove the QSpinBoxes. Especially the point that a pre-defined entry comming from the system did not let the user put in a decimal place (even at the start) when all of them were already in use.

Yes, you're right, this is a big upside! And I didn't mention it.

But: With blocking unfitting values, and no rule of how many decimal places get to be displayed, the experience just didn't feel so much better for me.
But feeling is not quite the right word. For me the issue is accuracy:
I think about significant figures, they shouldn't just change because somebody forgets a '.' or '0's.

How exactly is is specified how many decimal places get to be displayed and how many get to be used in the calculation?
Even if this is not that important in case of a lightsource, it will be for measurements -> for many cases we like to apply QLineEdit. Even if measurements in tigl are relative to each other, it shouldn't feel random, we should be careful not to change accuracy.

Didn't put this into words in my comment, sorry, for some reason I assumed this was a downside of QlineEdit, but @joergbrech just put it into code - though I must admit, I'm not quite sure about the number of decimal places of the internally (saved) values? Do they match? If they do:
I feel like I can be very happy now about the much nicer editing of decimal places, now ;).

@joergbrech: 'tab' and 'enter' do not work equivalent, right?
As @svengoldberg mentioned, if you leave the box with tab, whatever value you entered stays there, this is confusing, I'd expect the same behaviour as when pressing 'enter'.

I really like a lot- and that also makes a huge difference- : That I can enter any high value, and it gets rounded to the upper boundary, and also displays zero decimals even if I didn't enter them.
Another upside: If I press 'enter' the widget doesn't close, which happens with the Spinboxes (and is annoying).
In sum, both changes feel to me like an improvement, which justifies adding code to me now.

But I'm not completely convinced to replace ALL the spinboxes yet:

The hovering funcionality is absolutely annoying, that's right.

But coming from the specific point of view: If we'd prefer to keep the spin boxes, because they already bring a lot of functionality, and users would love them (which I don't know) what could we do?

• I would have to look into that, but it seems to me there is away to deactivate the edit on hover, and still use Spinboxes.
• How close to our prefered behaviour can we get customizing them?
• Did we rule out, that there is no way to change the behaviour entering decimal places? Can we also change the behaviour on pressing 'enter'?

[joergbrech]

Ok to sum up our discussion that we had on the phone today:

• Let's not waste too much time on this.
• Let's take orientation on my implementation in the sepearte branch (see above). Instead of rounding in view mode, we have a consistent visualization that does not round, e.g. scientific notation. There are some issues still to be fixed, e.g. the value is discarded when tabbing through the values.