search

Quasars / orange-spectroscopy

Other
51 stars 59 forks source link

[FIX] Non-limited and typing-friendly spin-boxes #587

Closed markotoplak closed 2 years ago

markotoplak commented 3 years ago

I always disliked decimal limitations for input wavenumbers. I reviewed biolab/orange3#5647 today which used just what we need here.

Also, now spin boxes now do not call valueChanged after each edit which means that strange issues while typing are gone! So, fixes #564.

TODO:

markotoplak commented 3 years ago

@stuart-cls, could you perhaps try this out? Thanks!

codecov-commenter commented 3 years ago

Codecov Report

Merging #587 (94bc3ef) into master (12a9952) will increase coverage by 0.00%. The diff coverage is 81.81%.

@@           Coverage Diff           @@
##           master     #587   +/-   ##
=======================================
  Coverage   88.79%   88.80%           
=======================================
  Files          63       63           
  Lines       11245    11259   +14     
=======================================
+ Hits         9985     9998   +13     
- Misses       1260     1261    +1
stuart-cls commented 3 years ago

@markotoplak This is great! Two small things:

  1. Can we disable mouse scroll events resulting in number changes? In both Integrate and Peak Fit where these are used, the boxes are in lists of models that are almost always long and require scrolling. The natural mouse wheel to scroll this can end up changing the values by mistake, which is definitely not what you want
  2. On my system, the numbers are localized in a way that is not what I want: double-comma Specifically, the comma in the wavenumber value is weird.
markotoplak commented 3 years ago

@stuart-cls, thanks for your comment. I think I solved both of your issues. Could you check?

Apart for the stuff already in TODO, do you see anything else that should be fixed?

stuart-cls commented 3 years ago

Works nicely! I like how the vertical line precision changes with the view zoom.

I can't think of anything else. Re the last TODO, I found the preview fit updates after moving the vertical line already (try with 2 offset-center Gaussian)

markotoplak commented 3 years ago

Peak fitting updates do not work well. Especially in delta mode. I suspect it also did not work properly before, but now that changed notification events go out more rarely this became more apparent.

I tried to quickly fix it but I could not. The main issue is that the editors store the same settings in two places: in the __values and within the edit-boxes components. Furthermore, state is stored in two different formats (lmfit's and check boxes can not correspond perfectly, because of delta), and kudos for keeping them synchronized well enough that this was not visible before.

I was thinking of a rewrite, which would store the state at one spot, just in __values. I'd always store it in the form that makes it convenient for editing (that would simplify edit box lines) and only at the end "export" into lmfit hints. That would simplify it and perhaps even reduce some signal overhead I remember hearing about.

stuart-cls commented 2 years ago

The delta mode is indeed a hack. I'm not very happy with it actually: as you say keeping it in sync between the three layers (lmfit, __values, edit boxes) was hard and I probably should have taken that as sign to do something else.

My design goal for the settings was indeed to have __values be the source of truth and translate to the edit boxes / lmfit hints only when needed. My secondary goal was to have the saved setting be directly usable as param_hint dict but I see what you mean about making life easier on the GUI side.

I've been thinking about what to do about better initial guesses when placing new peaks and about storing fit results as new initial conditions and how those might influence the delta setting type. The use case is to set a center wavelength and allow it to vary only within a sensible range. If the initial conditions are changed by the user, they likely want the range to change to match. But if they are updated otherwise, then the initial restraints are still correct.

All of which is to say the delta mode adds a lot of complexity and I'd want to make sure we want to keep it long term before re-writing the settings to accommodate it.

But! If you think changing the __values to store the GUI state is better to make things less complicated regardless of all that, let's do that.

(I recall having trouble sharing the settings between the ModelEditor and the ParamHintBox, but perhaps you know a way around that?)

markotoplak commented 2 years ago

I'll merge this as is and continue working on Peak Fit in #588.