Gmoccapy Tool/Offset pages: Value verification and tool data checking

[Sigma1912]

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 the lowest available 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]

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]

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]

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]

I use duplicate pocket numbers, stacked tools

[Sigma1912]

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]

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

[Sigma1912]

Okay, but duplicated tool numbers?

Yes, those should indeed not be allowed.

[grandixximo]

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]

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.

[hansu]

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

[grandixximo]

525-531

[grandixximo]

GitHub displays it weird because is not in the diff

[hansu]

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]

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]

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]

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

1. 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.
2. The calculator no longer tries to shape the return values to what the receiver might want.
3. Offset editing using the calculator is now handled by the same code as when editing with the keyboard.

Testers welcome.

[zz912]

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.

[Sigma1912]

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.

[zz912]

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]

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.