tooledit: Fix rounding issue

[d2inventory]

A tool length of 123.4567 would be rounded to 123.457
This commit allows for 9 digits instead of the default 6 digits.

Fixes issue #2593

[petterreinholdtsen]

Is it possible to add some script in tests/ verifying this change work as intended?

[d2inventory]

I added a test.

Test:

puts [ format "%.9g" 123]
puts [ format "%.9g" 123.0]
puts [ format "%.9g" 123.000]
puts [ format "%.9g" 123.456]
puts [ format "%.9g" 0.1]
puts [ format "%.9g" 0.001]
puts [ format "%.9g" 0.0001]
puts [ format "%.9g" 0.00001]
puts [ format "%.9g" 0.000001]
puts [ format "%.9g" 12345.6789]
puts [ format "%.9g" 12345.0]
puts [ format "%.9g" 1234.123456]
puts [ format "%.9g" 12345.123456]
puts [ format "%.9g" 123456.123456]
puts [ format "%.9g" 1234567.123456]
puts [ format "%.9g" 12345678.123456]
puts [ format "%.9g" 123456789.123456]

Result:

123
123
123
123.456
0.1
0.001
0.0001
1e-05           <---- issue I mentioned
1e-06           <---- issue I mentioned
12345.6789
12345
1234.12346      <---- rounded
12345.1235      <---- rounded
123456.123      <---- rounded
1234567.12      <---- rounded
12345678.1      <---- rounded
123456789

[petterreinholdtsen]

Why did you change sim_mm.tbl? The test seem to test tcl more than tooledit. Is there no way to test the tooledit output non-interactively?

[d2inventory]

I changed it because I tested it locally by running linuxcnc and edititing the tool lengths and then fucked up when commiting to git.

I guess we could start tooledit with a tool table that has length with lots of digits and then make it save somehow but I don't know.

But I think this is all a bit of an overkill. This is simply changing the default 6 digits of %g to 9 digits.

[smoe]

@petterreinholdtsen recently introduced the extra automated testing and I agree that this looks a bit like an opportunity for it. My hunch is that he is looking after something like

source ./tcl/tooledit.tcl

set ::te(fmt,dummy) real
set ::te(dummy) 0.12345678901234567890
puts $::te(dummy)

exit

Just hat I am somewhat not capable to get this right. :)

[d2inventory]

I updated the test to use a test.tbl that is loaded with tooledit.tcl and then saved again.

While doing this I ran into an issue with the comments, which were converted in a strange way.
(the ä in schaftfräser was always different on saving)
This made me realise that tooledit always converts every line to lower case, which I think is bad as comments should be left as is.
So I made a slight change to tooledit.tcl to fix this issue and convert the lines to lower after the comment has been extracted.
This commit also contains a small fix that messed up syntax highlighting and was an error anyway (but I guess this code path is just never hit so nobody ever encountered the error).

[andypugh]

It looks like the tooledit test is now failing?

[d2inventory]

Yeah, I have to figure out why because it works locally for me.

Maybe because there is no display available on the server side testing?

[petterreinholdtsen]

[d2inventory]

Yeah, I have to figure out why because it works locally for me. Maybe because there is no display available on the server side testing?

There is no display server running. If you need one during testing, you will have to start it using for example Xvfb.

…

-- Happy hacking Petter Reinholdtsen

[andypugh]

I am starting to think that the amount of work being demanded for a test of this simple change is rather disproportionate.

[petterreinholdtsen]

[andypugh]

I am starting to think that the amount of work being demanded for a _test_ of this simple change is rather disproportionate.

Not quite sure who other than the release manager who can make demands here. Personally I have tried to only make recommendations. In any case, running a program under X from the command line when there is no X server available is as simple as 'xvfb-run program-name'. Here is an example from my machine: % xvfb-run glxgears 7044 frames in 5.0 seconds = 1408.685 FPS 7507 frames in 5.0 seconds = 1501.208 FPS ^C % No window showed up on my screen, and running glxgears on my X server show "Running synchronized to the vertical refresh. The framerate should be approximately the same as the monitor refresh rate." and a frame rate around 60.

…

-- Happy hacking Petter Reinholdtsen

[d2inventory]

@petterreinholdtsen

I highly doubt that xvfb is installed on the build servers. I set up a test environment locally with a minimal debian12 install and can reproduce the error.
If I install xvfb and use it as you suggested the test runs through successfully.

Let's see if it works if I just use xvfb-run ...

[d2inventory]

Well, looks like I was very wrong and using xvfb-run solved the problem 😄

EDIT:
nvm, seems like some have xvfb installed and others dont :-/

[d2inventory]

@petterreinholdtsen

How should I proceed now?

[petterreinholdtsen]

[d2inventory]

How should I proceed now?

The test look good, but we need it running on all archs, both those running on github and those running on buildbot. I see the test fail on some platforms on github, on buster like this: + export DISPLAY=:0 + DISPLAY=:0 + xvfb-run /usr/bin/tclsh8.6 test.tcl test.sh: line 8: xvfb-run: command not found + cat result.tbl cat: result.tbl: No such file or directory + rm result.tbl rm: cannot remove 'result.tbl': No such file or directory + exit 0 I suggest dropping the 'export DISPLAY=:0' line as it should be useless, as xvfb-run will set it automatically. You probably also need to add 'xvfb' as a build dependency in debian/control.top.in , or if this do not provide xvfb-run on all archs, only run the test if 'test xvfb-run' succeeds.

…

-- Happy hacking Petter Reinholdtsen

[petterreinholdtsen]

[d2inventory]

b0589b0 add xvfb as a dependency and check if it exists in tooledit test

For some reason xvfb is not installed in the github buster build test. I have no idea why.

…

-- Happy hacking Petter Reinholdtsen

[d2inventory]

@petterreinholdtsen

it's passing all the tests now.

[petterreinholdtsen]

[d2inventory]

it's passing all the tests now.

Nice. Code and test set look good to me. Do you want to squash the commits into one? @andupugh, ok to merge?

…

-- Happy hacking Petter Reinholdtsen

[d2inventory]

@petterreinholdtsen

I squashed it all into one commit.

[petterreinholdtsen]

I cherry-picked this commit into the 2.9 branch and merged 2.9 into master.