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.

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.

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.

gmoccapy · 2026-08-30T08:37:51Z

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.

That is a valid usecase. I can imagine similar cases for some tools.

hansu · 2026-09-16T11:50:06Z

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

👍

Tool numbers on new tools are preset to one above the highest number found in the table.

👍

Dialogs use the popups native to gmoccapy (the widgets can fall back to their own ).

For this dialog it's not yet a gmoccapy dialog :

Sigma1912 · 2026-09-16T12:14:04Z

For this dialog it's not yet a gmoccapy dialog :

Should be fixed now. Thanks for testing.

hansu

And please rebase to master to resolve the conflicts.

hansu · 2026-09-16T11:54:13Z

+                if value != int(value):
+                    raise ValueError
+                value = int(value)
            if self.use_localization:


locale.atof( value ) doesn't make sense here because localization is already handled in compute() and value is already a float at this point (for non integer mode).

Git messed up the line numbers again, it is about this part

value = float(self.entry.get_text()) if self.integer_only: if value != int(value): raise ValueError value = int(value) if self.use_localization: value = locale.atof( value )

hansu · 2026-09-16T11:55:10Z

        qualified = ''
        temp = self.eval_string.strip( " " ).replace("Pi", "math.pi")
+        if self.integer_only and ( '.' in temp or ',' in temp):
+            self.entry.set_text( "Error" )


What speaks against raising the ValueError right here instead of later in get_value()?

Its about those lines:

def compute( self ): qualified = '' temp = self.eval_string.strip( " " ).replace("Pi", "math.pi") if self.integer_only and ( '.' in temp or ',' in temp): self.entry.set_text( "Error" ) return

The check for decimal symbols in compute() is required to disallow keyboard entry of, say, '6.0' in the calculator, which we disallow for direct keyboard entries into the table.

The check in get_value() is required to check the calculator result of 'b = str( eval( qualified ) )' because a user can enter an operation using integers into the calculator using the keyboard (eg '6/2' is allowed but '3/2' must raise an error).

hansu · 2026-09-16T12:10:15Z

+                value = float(new_text)
+                if value != int(value) or value not in range(10):


Why not

try : value = int(new_text)

like above?

If we want to accept inputs like this "1.000000" we could also use <float_type>.is_integer()

It is only a bit inaccurate:

>>> float("1.000000000000001").is_integer() False >>> float("1.0000000000000001").is_integer() True

I'll adopt the first suggestion to make it consistent with the code for columns 1 and 2.
I think we agreed that we don't want to silently change floats to integers and I have already changed the calculator widget to not accept keyboard input containing decimals.

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

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

- 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

Sigma1912 · 2026-09-16T14:40:25Z

Alas, there have been other changes pushed directly that actually break this PR. I don't have the time right now so I'm afraid this will have to wait.

hansu · 2026-09-16T14:48:15Z

After the rebase I get this:

Traceback (most recent call last):
  File "/home/cnc/linuxcnc-master/bin/gmoccapy", line 2221, in on_tool_col_edit_started
    self.widgets.tooledit1.validate_input(row, cell_text, col, captations)
  File "/home/cnc/linuxcnc-master/lib/python/gladevcp/tooledit_widget.py", line 606, in validate_input
    self.ext_dialog(msg,header)
  File "/home/cnc/linuxcnc-master/bin/gmoccapy", line 2137, in tooltable_dialog
    self.dialogs.warning_dialog(self, header, message)
TypeError: Dialogs.warning_dialog() takes 1 positional argument but 4 were given
 (gmoccapy:86)

The two calls self.dialogs.warning_dialog(self, header, message) :

    def tooltable_dialog(self, message, header=None):
        if header is None:
            header = _("Tool page error:")
        self.dialogs.warning_dialog(self, header, message)

    def offsetpage_dialog(self, message, header=None):
        if header is None:
            header = _("Offset page error:")
        self.dialogs.warning_dialog(self, header, message)

should probably be self.dialogs.show_warning_dialog(self, header, message)

hansu · 2026-09-16T18:13:28Z

Alas, there have been other changes pushed directly that actually break this PR. I don't have the time right now so I'm afraid this will have to wait.

It seems Chris did some commits on master accidentally :/

Sigma1912 · 2026-09-17T09:31:01Z

I'm sorry, couldn't recover. See new #4559

hansu · 2026-09-17T21:23:01Z

I'm sorry, couldn't recover. See new #4559

You could have force-pushed your new branch into this one, then this PR would have stayed alive.
But I don't care ;-)

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 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 4 times, most recently from ef085c4 to 2223d77 Compare August 29, 2026 12:45

Sigma1912 marked this pull request as ready for review August 30, 2026 05:59

Sigma1912 force-pushed the Gmoccapy_fix-calc-input-tool branch from 2223d77 to b488664 Compare September 16, 2026 12:10

hansu reviewed Sep 16, 2026

View reviewed changes

Sigma1912 added 3 commits September 16, 2026 14:32

Gmoccapy tooledit_widget: new tool gets next available toolnr

eaf3f79

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 b488664 to 4c5e603 Compare September 16, 2026 12:45

Sigma1912 marked this pull request as draft September 17, 2026 07:25

Sigma1912 mentioned this pull request Sep 17, 2026

Gmoccapy: value checks on table entries #4559

Open

Sigma1912 closed this Sep 17, 2026

		value = float(new_text)
		if value != int(value) or value not in range(10):

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!

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

Uh oh!

Sigma1912 commented Aug 29, 2026

Uh oh!

gmoccapy commented Aug 30, 2026

Uh oh!

hansu commented Sep 16, 2026

Uh oh!

Sigma1912 commented Sep 16, 2026

Uh oh!

hansu left a comment

Choose a reason for hiding this comment

Uh oh!

hansu Sep 16, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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 Sep 16, 2026 •

edited

Loading