Gmoccapy Tool/Offset pages: Value verification and tool data checking by Sigma1912 · Pull Request #4436 · LinuxCNC/linuxcnc

Sigma1912 · 2026-08-21T09:05:03Z

Discussion has shown that silent manipulation to make the user entered data fit the data type required by the cell is not wanted and that a rejected value should trigger a message to the user.

Value entries by the user to the offset or tool tables are currently 'verified' and manipulated in several places:

Calculator widget
entry_dialog
gmoccapy
tooledit / offsetpage widgets

This PR moves value verification to the end recipient (ie the tooledit - and offsetpage widgets). The value is verified in the same place regardless whether the user uses the keyboard directly, the buttons in the calculator widget or the keyboard in the calculator widget. The cell and it's data range is known and a clear user message can be generated.

Fixes #4435

Also in this commit:

Tool table is checked for duplicate tool numbers on save.

The second commit:

The tool number for a new tool is preset to one above the highest number found in the table instead of '0'

The third commit:

Lines that contain only white space are ignored instead of creating a new row with all '0'
A message is shown on startup if the tool file contains
duplicated tool numbers
orientation values not in range 0...9
malformed lines (missing descriptors or values) these lines are copied to a file so they are not silently lost when the table is saved.

Example:

hansu · 2026-08-21T09:47:54Z

That was quick, thanks.

I haven't dug deep into this but I wonder if we should fix it better in tooledit (or additionally).
It fails in line int(new_text) with new text = "5.000" for example.

linuxcnc/lib/python/gladevcp/tooledit_widget.py

Lines 513 to 523 in bae5676

    
           if col in(1,2): 
        
               try: 
        
                   self.model[path][col] = int(new_text) 
        
               except: 
        
                   pass 
        
           # validate input for float columns 
        
           elif col in range(3,15): 
        
               try: 
        
                   self.model[path][col] = f"{float(new_text.replace(',', '.')):10.4f}" 
        
               except: 
        
                   pass

This would also fix it in this case. But it might be bad if this function gets accidentally a float value which is cut then.

    if col in(1,2):
        try:
            import re
            self.model[path][col] = int(re.split(r'[.,]', new_text)[0])

Sigma1912 · 2026-08-21T14:23:19Z

I noticed that the pocket number column currently allows negative integer values, which I don't think is correct?
So Tool and Pocket number columns will now truncate any float to the absolute integer value. I'm not sure we still need the dot,comma replacement but kept it just in case.

hansu · 2026-08-22T08:08:24Z

I noticed that the pocket number column currently allows negative integer values, which I don't think is correct?

I don't know. Maybe one want to disable a tool by setting pocket to -1?
I think the bigger problem is that duplicated tool numbers and pocket numbers are allowed.

grandixximo · 2026-08-22T08:37:32Z

I use duplicate pocket numbers, stacked tools

Sigma1912 · 2026-08-22T10:02:18Z

Just checked in Axis gui (if that is anything to go by):

allows duplicate pocket numbers
errors out on negative pocket numbers

And yes, there is definitely a use for duplicate pocket number.

hansu · 2026-08-22T17:56:39Z

Okay, but duplicated tool numbers? You would never know which one will be used.

Sigma1912 · 2026-08-22T18:10:21Z

Okay, but duplicated tool numbers?

Yes, those should indeed not be allowed.

grandixximo · 2026-08-22T23:39:44Z

I used programs where the tool ID has to be unique, it is tricky to do it without getting in the way when the user is renumbering tools for whatever reason, some thought need be put into that. Although at the final state there should be no duplicates, when user is wanting to do a renumbering, not having any hard blocks (current behavior) allows to have a moment where there are two tools with the same number; the trick is finding a reasonable way to allow renumbering without getting in the way of the renumbering cleanup flow, some kind of message confirmation, same number already in the table, want to swap number? cancel?
Also when adding a tool, it would be nice to have the next available ID be ready for the user.
Also there could be tool tables with duplicates in the wild, a warning that those need be fixed should show up somewhere somehow without nagging the user too much...

grandixximo · 2026-08-23T01:41:35Z

            except:
                pass
        # validate input for orientation: check if int and valid range
        elif col == 15:


This did not get the same float-tolerant treatment: it still uses plain int(new_text). Direct cell entry of 3.0 is now accepted for tool/pocket but still silently rejected for orientation. Same int(float(...)) pattern, keeping the existing range check, would unify it.

What are the lines you are referring to? I think the above lines not.

GitHub displays it weird because is not in the diff

hansu · 2026-08-23T08:12:29Z

I used programs where the tool ID has to be unique, it is tricky to do it without getting in the way when the user is renumbering tools for whatever reason, some thought need be put into that. Although at the final state there should be no duplicates, when user is wanting to do a renumbering, not having any hard blocks (current behavior) allows to have a moment where there are two tools with the same number; the trick is finding a reasonable way to allow renumbering without getting in the way of the renumbering cleanup flow, some kind of message confirmation, same number already in the table, want to swap number? cancel? Also when adding a tool, it would be nice to have the next available ID be ready for the user. Also there could be tool tables with duplicates in the wild, a warning that those need be fixed should show up somewhere somehow without nagging the user too much...

I could imagine to do the check on save. So it's easy to rename the IDs and have duplicate IDs in between.

Sigma1912 · 2026-08-23T09:03:11Z

Generally I cannot really comment since I don't use the tool table much.
Maybe we also shouldn't silently truncate a float entry to integer either.

Check on save seems the most straight forward to me but we would probably also have to check on load since a tool table can easily be edited outside the gui.

This widget is shared with axis, gscreen and qtdragon

I thought this was only shared by gscreen.

zz912 · 2026-08-25T10:25:59Z

As a tester, I don't think silently truncating a decimal value is the right behavior here.

If the Tool Number or Pocket Number field expects an integer and a user accidentally enters, for example, 5.7, I would expect the application to report a validation error rather than silently changing the value to 5.

From a testing/UX perspective, silently modifying the user's input can hide an input error and potentially lead to a different tool or pocket being used than the operator intended.

In my opinion, it would be safer and clearer to reject the value and tell the user that an integer is required. The user can then correct the input explicitly.

Sigma1912 · 2026-08-25T14:37:59Z

I agree that silent changes to the values entered is not the right thing to do so I made some deeper changes.

The receiver of the data (eg the offsetpage_widget or the tooledit_widget) is responsible for checking the entered values and will now inform the user if an entry is rejected.
The calculator no longer tries to shape the return values to what the receiver might want.
Offset editing using the calculator is now handled by the same code as when editing with the keyboard.

Testers welcome.

zz912 · 2026-08-25T15:26:19Z

            return None
+        if self.use_localization:
+            try:
+                value = locale.atof( value )


Just a small reminder regarding the use of locale.atof() — perhaps it simply got overlooked in the context of the other changes. 🙂

We have spent quite a bit of time dealing with atof() and decimal separator localization in LCNC, and we even have a recommendation about this in the documentation: https://linuxcnc.org/docs/devel/html/en/gui/gui-dev-reference.html#_localization_of_float_numbers_in_guis

atof() can of course work correctly in some situations, but in combination with locales I find it rather tricky and potentially a bit of a time bomb — especially because the problem may not show up under the usual EN locale.

If the intention here is to keep using locale.atof(), I would at least ask for it to be tested with the DE locale, where the decimal separator is a comma. That is where I would expect any potential problems to show up first.

Please don't take this as a blocker for the change, but rather as a reminder based on the experience we've already had with this in LCNC.

That is current code that got moved around not something new. That is there for backwards compatibility aimed at Gscreen only not something that Gmoccapy uses.

Yes, I understand that this is existing code being moved around and that it is only there for Gscreen backwards compatibility.

The reason I mentioned it is precisely because this is how we ended up with problems in the past. atof() had been working without any apparent issues for a long time, and then some surrounding code changed and suddenly the locale-related problems started showing up.

So my concern is not that atof() is being introduced here as something new, but rather that moving it around can potentially change the context in which it runs. That's why I would still consider a quick test with the DE locale worthwhile, just to make sure the existing behaviour remains safe after the change.

If that has already been covered by the existing Gscreen compatibility testing, then of course that's fine.

Sigma1912 · 2026-08-26T09:23:25Z

Also when adding a tool, it would be nice to have the next available ID be ready for the user.
Also there could be tool tables with duplicates in the wild, a warning that those need be fixed should show up somewhere somehow without nagging the user too much...

Both implemented.

gmoccapy · 2026-08-28T08:01:19Z

Sorry for jumping in late.
I was surprised reading that it was mentioned duplicate tool and pocket numbers may be needed.

I can not imagine a situation where the same tool or pocket numbers may be used in one tool table. The G-code does name the tool number, the "change code" looks in the tool table for that tool and gets that way the corresponding pocket number. The mechanical tool changer doesn't care about the tool number, as it just gets the tool out of the pocket and puts it in the spindle.

If you use a sister tool (i.e. 2 drills 5 mm diameter) in one tool changer the tools should have different tool and pocket numbers. If drill 1 is worn out an dyou want to use drill 2, you will need to edit the tool table. Otherwise, how will you know witch tool will be used?

So IMHO it should not be possible to have duplicate tool numbers and also not duplicate pocket numbers.

Norbert

hansu · 2026-08-28T08:13:18Z

I could image a use case for duplicate pocket numbers: If you want to use the same tool for two different g-code files where they have different tool numbers, you could just give them the same pocket number.

grandixximo · 2026-08-28T08:25:59Z

Multiple profiling wheels stacked on top of each other, on the same cone, all tools live in the same pocket, multiple tools with different height and diameters for different profiles, this is exactly how I use my edge profiling machines, when I call the tool change for a tool in the same pocket, the tool change routing is skipped, different tool same pocket is easy to use as conditional for skipping tool change, also simple to understand and manage, really don't see why it should be denied as a workflow, if you think is dangerous on some machines, might be worth an optional warning, I don't mind if you default it to warn about duplicate pockets, but removing the functionality altogether I don't think is wise.

hansu · 2026-08-28T15:04:54Z

+                    self.model[path][col] = int(value)
            except:
-                pass
+                msg = (_(f"\nMust be a positive whole number"))


Suggested change

msg = (_(f"\nMust be a positive whole number"))

msg = (_(f"\nMust be a positive integer"))

Sure, note though that 'integer' is a term out of computer science, not every machine operator might be as familiar with it as we are.

As far as I know is Integer just the english word for "Ganzzahl". I think only german speaking people can imagine what you mean with "whole number" ;-)

https://www.oxfordlearnersdictionaries.com/definition/english/whole-number

The average machinist will understand "positive whole number" much faster and more naturally than "positive integer."

"Integer" is an academic term, I'm with Sigma on this one ;-)

hansu · 2026-08-28T15:06:46Z

Also when adding a tool, it would be nice to have the next available ID be ready for the user.
Also there could be tool tables with duplicates in the wild, a warning that those need be fixed should show up somewhere somehow without nagging the user too much...

Both implemented.

Thanks.

It actually fills the gap of tool numbers. So if you have a tool no. 16 and 18, it creates a tool 17. Would be better if it creates a tool 19 in this case.
Can't we use the Gmoccapy dialog for the warnings?
To be consistent we should also display a warning when entering e.g. 16.0 for the tool number (16,0 gives a warning).

Sigma1912 · 2026-08-28T15:33:32Z

It actually fills the gap of tool numbers. So if you have a tool no. 16 and 18, it creates a tool 17. Would be better if it creates a tool 19 in this case.

Could you elaborate on the reasoning behind this?

Can't we use the Gmoccapy dialog for the warnings?

Possibly, but why? The point is that the recipient widget verifies and displays the message as to why the entry was rejected.

hansu · 2026-08-28T16:06:47Z

It actually fills the gap of tool numbers. So if you have a tool no. 16 and 18, it creates a tool 17. Would be better if it creates a tool 19 in this case.

Could you elaborate on the reasoning behind this?

First it messes up the order. Ok no big thing.
But I use to use not continuous tool numbers.
E.g. 30, 35, 40, 60 and so on. So adding a tool with number 31 would not be helpful.
But the old version with new tool number = 0 has the advantage that it's easier to see that this is the newly added tool.

Can't we use the Gmoccapy dialog for the warnings?

Possibly, but why? The point is that the recipient widget verifies and displays the message as to why the entry was rejected.

Because the Gmoccapy dialogs are touch optimized. And contribute to a consistent UI. For the exit of the tool table there is also the Gmoccapy dialog used.

Sigma1912 · 2026-08-29T01:19:43Z

First it messes up the order. Ok no big thing.

Clicking on the header 'Tool#' will sort the table.

But I use to use not continuous tool numbers.
E.g. 30, 35, 40, 60 and so on. So adding a tool with number 31 would not be helpful.

But in that case '61' would also not help, you would have to change the tool number either way.

But the old version with new tool number = 0 has the advantage that it's easier to see that this is the newly added tool.

The problem is that there might already be a tool '0' in the table. Also tool number '0' has different usage depending on random/non-random tool changers.

Sigma1912 · 2026-08-29T01:21:52Z

Can't we use the Gmoccapy dialog for the warnings?
To be consistent we should also display a warning when entering e.g. 16.0 for the tool number (16,0 gives a warning).

Ok, I'll have a look.

Lines containing whitespace only are skipped rather than creating a new line with all '0' A message is shown on startup if - duplicate tool numbers have been found in the tool table - orientation is not in range 0...9 - a line is malformed, these are saved to a separate file so they are not silently deleted on save

- value validation is done by the widgets the values are entered for - if the calculator is set to 'integer_only' it will reject any value or expression containing '.' or ',' - value editing is handled in the widget regardless whether directly with the keyboard or the calculator - On save the table is checked for duplicate tool numbers

The tool number for a new tool is preset to one above the highest nummber found in the table instead of '0'

Sigma1912 · 2026-08-29T12:52:09Z

Fixed the bug where an entry with a zero decimal (eg '16.0') in the calculator did not trigger an error.

Modified as requested:

Dialogs use the popups native to gmoccapy (the widgets can fall back to their own ).
Tool numbers on new tools are preset to one above the highest number found in the table.

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from b1369d0 to bb1801a Compare August 21, 2026 14:17

Sigma1912 changed the title ~~Gmoccapy: Fix calculator input for tool- and pocket-nr in tooltable~~ Gmoccapy: Fixes in calculator and tooledit_widget Aug 21, 2026

hansu reviewed Aug 22, 2026

View reviewed changes

Comment thread src/emc/usr_intf/gmoccapy/gmoccapy.py Outdated

grandixximo approved these changes Aug 23, 2026

View reviewed changes

Comment thread lib/python/gladevcp/tooledit_widget.py Outdated

grandixximo approved these changes Aug 23, 2026

View reviewed changes

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from 733754b to 361bc09 Compare August 25, 2026 14:22

Sigma1912 marked this pull request as draft August 25, 2026 14:22

zz912 reviewed Aug 25, 2026

View reviewed changes

Sigma1912 changed the title ~~Gmoccapy: Fixes in calculator and tooledit_widget~~ Gmoccapy: Move value verification to the widgets that recieve them Aug 25, 2026

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from 361bc09 to 4265bc8 Compare August 26, 2026 06:52

Sigma1912 marked this pull request as ready for review August 26, 2026 06:55

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from 4265bc8 to a27dfc9 Compare August 26, 2026 07:31

Sigma1912 changed the title ~~Gmoccapy: Move value verification to the widgets that recieve them~~ Gmoccapy Tool/Offset pages: Value verification and Tooln# handling Aug 26, 2026

Sigma1912 changed the title ~~Gmoccapy Tool/Offset pages: Value verification and Tooln# handling~~ Gmoccapy Tool/Offset pages: Value verification and Toolnr handling Aug 26, 2026

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from a27dfc9 to 4a9e629 Compare August 26, 2026 07:39

Sigma1912 marked this pull request as draft August 26, 2026 07:52

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from 4a9e629 to 4eac6cc Compare August 26, 2026 08:00

Sigma1912 marked this pull request as ready for review August 26, 2026 09:23

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch 4 times, most recently from 6de1203 to 6ee058f Compare August 26, 2026 13:27

Sigma1912 changed the title ~~Gmoccapy Tool/Offset pages: Value verification and Toolnr handling~~ Gmoccapy Tool/Offset pages: Value verification and tool data checking Aug 26, 2026

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from 6ee058f to 3399b5b Compare August 26, 2026 13:34

hansu reviewed Aug 28, 2026

View reviewed changes

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch 2 times, most recently from 8ffb330 to 1e404c6 Compare August 29, 2026 09:52

Sigma1912 marked this pull request as draft August 29, 2026 09:54

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch 3 times, most recently from d0b25b6 to ef085c4 Compare August 29, 2026 12:40

Sigma1912 added 3 commits August 29, 2026 14:41

Gmoccapy tooledit_widget: new tool gets next available toolnr

2223d77

The tool number for a new tool is preset to one above the highest nummber found in the table instead of '0'

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from ef085c4 to 2223d77 Compare August 29, 2026 12:45

	msg = (_(f"\nMust be a positive whole number"))
	msg = (_(f"\nMust be a positive integer"))

Uh oh!

Conversation

Sigma1912 commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansu commented Aug 21, 2026

Uh oh!

Sigma1912 commented Aug 21, 2026

Uh oh!

Uh oh!

hansu commented Aug 22, 2026

Uh oh!

grandixximo commented Aug 22, 2026

Uh oh!

Sigma1912 commented Aug 22, 2026

Uh oh!

hansu commented Aug 22, 2026

Uh oh!

Sigma1912 commented Aug 22, 2026

Uh oh!

grandixximo commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hansu Aug 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hansu commented Aug 23, 2026

Uh oh!

Sigma1912 commented Aug 23, 2026

Uh oh!

zz912 commented Aug 25, 2026

Uh oh!

Sigma1912 commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sigma1912 commented Aug 26, 2026

Uh oh!

gmoccapy commented Aug 28, 2026

Uh oh!

hansu commented Aug 28, 2026

Uh oh!

grandixximo commented Aug 28, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hansu Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grandixximo Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hansu commented Aug 28, 2026

Uh oh!

Sigma1912 commented Aug 28, 2026

Uh oh!

hansu commented Aug 28, 2026

Uh oh!

Sigma1912 commented Aug 29, 2026

Uh oh!

Sigma1912 commented Aug 29, 2026

Sigma1912 commented Aug 21, 2026 •

edited

Loading

grandixximo commented Aug 22, 2026 •

edited

Loading

hansu Aug 23, 2026 •

edited

Loading

Sigma1912 commented Aug 25, 2026 •

edited

Loading

hansu Aug 28, 2026 •

edited

Loading

grandixximo Aug 29, 2026 •

edited

Loading