Fix spinner lower bound

[ParticularlyPotent]

To fix issue #1777

Cm.to.Px.conversion.fix.webm

[Kneemund]

Hi, thanks for reporting the issue and creating this PR!

MeasureUnit::convert_measurement already correctly converts the minimal bound for PX into other units. Your changes only make the bounds smaller for MM and CM, that's why the bug seems to disappear.

The root cause of this bug are the following lines though:

rnote/crates/rnote-ui/src/unitentry.rs

Lines 156 to 164 in 99fd77f

	self.configure_spinner(unit, self.dpi.get());
	obj.set_value(MeasureUnit::convert_measurement(
	self.value.get(),
	self.unit.get(),
	self.dpi.get(),
	unit,
	self.dpi.get(),
	));
	self.unit.replace(unit);

• configure_spinner updates the widget and applies the new minimum and maximum values, which clamps the value to the new minimum.
• set_value then converts the value to the new unit, but uses the clamped value of the spinner instead of the original one.

So a proper fix would be to either calculate the new value before calling configure_spinner, or storing the original old value in a variable before calling configure_spinner. The same issue occurs when setting the DPI, that can be fixed in the same way.

Do you want to make these changes yourself? Or do you want me to take over?

[ParticularlyPotent]

Thank you for the response. You're right that it's better to instead use the pre-clamped value and leave configure_spinner alone. Let me post my change in a second...
edit: I am trying to figure out git. I am a beginner. Sorry

[ParticularlyPotent]

Sorry, I had a bit of a mix-up with another account. Is this the kind of change you were looking for?

[Kneemund]

Yes. Could you add the same fix to the DPI section too (directly below)? In theory, the same bug could occur there.

[ParticularlyPotent]

Great catch! Although I do wonder if there should be a comment placed here briefly saying why we are making copies of these floating point numbers before we run configure_spinner. I am not extremely familiar with Rnote's codebase, and I don't have nearly as much Rust experience as y'all, but to me it seems unintuitive that running configure_spinner(), which is meant to set the spinner's properties will clamp values like these and ruin your conversions. Do you think this warrants a comment?

[Doublonmousse]

Well, if you feel this warrant comments, feel free to add them (and refer to the github issue number).

The crux of the issue is keeping the UI state (displayed in the UI in the gtk4 element where the spinner bounds apply) and the internal state (the value/unit/dpi properties) coherent when updating. So there's some precautions to take here to update things in a coherent manner (all the more so when updating a property updates more than one like that is the case for unit/dpi).

The reason the update on the value occurs is probably

rnote/crates/rnote-ui/src/unitentry.rs

Lines 80 to 85 in 99fd77f

	obj.bind_property("value", &self.value_spinner.get(), "value")
	.transform_to(|_, val: f64| Some(val))
	.transform_from(|_, val: f64| Some(val))
	.sync_create()
	.bidirectional()
	.build();

So setting the bounds on the unitentry will trigger a change of the self.value in the case where clamping moves the value to the clamped one

NB : there's other issues with this component, like #1773.

[Kneemund]

I actually meant this section with "DPI section". We need the same fix here. But I don't think we need to store the DPI (what your latest commit did).

Maybe it would be best to simply calculate the new value altogether before calling the configure function (instead of doing it inline in the function argument)?

[ParticularlyPotent]

I hope that's what was intended. Anything else should be done?