Skip to content

qtplasmac: fix conversational circle M3 vs M03 bug - #4580

Merged
snowgoer540 merged 1 commit into
LinuxCNC:masterfrom
snowgoer540:gregc/conv-bug
Sep 24, 2026
Merged

snowgoer540 merged 1 commit into
LinuxCNC:masterfrom
snowgoer540:gregc/conv-bug

Conversation

@snowgoer540

Copy link
Copy Markdown
Contributor

For conversational circle, the torch on commands would be written as M3 instead of the M03 convention used by all of the other shapes. This caused errors with conversational block (specifically array in my testing but maybe others).

Fixed an objectname copy/paste error in convesrational.py that prevented proper resetting between screen changes, as well as prevented proper valid character checking.

For conversational circle, the torch on commands would be written as M3 instead of the M03 convention used by all of the other shapes.  This caused errors with conversational block (specifically array in my testing but maybe others).

Fixed an objectname copy/paste error in convesrational.py that prevented proper resetting between screen changes, as well as prevented proper valid character checking.
@grandixximo

Copy link
Copy Markdown
Contributor

M3 and M03 are both valid, doing the fix this way might bite again in the future, why not fix block.py and conversational.py instead?

@snowgoer540

Copy link
Copy Markdown
Contributor Author

Because this maintains the spirit of the person who wrote the code and requires one line to be changed, he just missed it when he refactored it.

@snowgoer540
snowgoer540 merged commit 61e56d7 into LinuxCNC:master Sep 24, 2026
17 checks passed
@grandixximo

Copy link
Copy Markdown
Contributor

Because this maintains the spirit of the person who wrote the code and requires one line to be changed, he just missed it when he refactored it.

That's reasonable as a quick fix, you didn't have to close the PR, was that a mistake?

@snowgoer540

Copy link
Copy Markdown
Contributor Author

Because this maintains the spirit of the person who wrote the code and requires one line to be changed, he just missed it when he refactored it.

That's reasonable as a quick fix, you didn't have to close the PR, was that a mistake?

No, I merged the PR.

I don’t agree with this unilateral branch protection decision that was made and put in place out of nowhere, and the reasons behind it, especially by those who have traditionally not followed the guidelines (rules for thee and not for me). I can cite some pretty specific examples of commits that caused issues, that were largely ignored, for well over a year.

I can understand Pull Requests and reviews for large changes that span multiple GUIs and/or affect core functionality, etc. I’m not interested in debating every change to a GUI that Phill (now retired from the project) and I spent many years writing, especially trivial ones.

@grandixximo

Copy link
Copy Markdown
Contributor

Sorry I saw closed, and not the merged, my bad, yeah there is discussion ongoing about that change, and already expressed the discussion should have happened first, the change is fine, just wondering why it was not done in block.py didn't mean to be argumentative...

@Sigma1912

Copy link
Copy Markdown
Contributor

I’m not interested in debating every change to a GUI that Phill (now retired from the project) and I spent many years writing, especially trivial ones.

A bit sad when somebody goes through the trouble of pointing out a possible gap in your code and all you have to say is basically 'get off my turf'.

@snowgoer540

Copy link
Copy Markdown
Contributor Author

A bit sad when somebody goes through the trouble of pointing out a possible gap in your code and all you have to say is basically 'get off my turf'.

Hi David, thanks for your new found interest in QtPlasmaC!

That's not what I said, nor does it represent my attitude toward either the project or code reviews.

Good luck with the agenda.

@grandixximo

grandixximo commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Text can come across as passive aggressive unintentionally; let's not make this a bigger deal than it is, the commit here is fine, and master lockdown is a bigger discussion.
The review was just food for thought, and the merge was fair, I was about to merge when I saw closed, that's what got me confused...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants