Skip to content

Conversation

@Sigma1912
Copy link
Contributor

  • inputs and deletions in the entry box are now respecting cursor position instead of being always at the end
  • a bit of clean up

@Sigma1912
Copy link
Contributor Author

Sigma1912 commented Jun 15, 2025

Is there any reason for not using the unicode '\u03C0' for the small greek letter "pi" instead of the string "Pi"?

π

That would make almost all the special handling of the two character string 'Pi' in the code obsolete.

Maybe we could also replace the 'DEL' with the more correct '\u232B':

Like this:
calc

@hansu
Copy link
Member

hansu commented Jun 15, 2025

Is there any reason for not using the unicode '\u03C0' for the small greek letter "pi" instead of the string "Pi"?

π

I guess it's because your keyboard doesn't have a π-key, but you can enter 'Pi' with your keyboard....

Maybe we could also replace the 'DEL' with the more correct '\u232B':

Yes, we can change that.

@Sigma1912
Copy link
Contributor Author

Looks like this now:

calc_widget

@hansu
Copy link
Member

hansu commented Jun 16, 2025

I prefer the small font size (like in the rest of the GUI).
Squeezing the text with the large font size on the buttons just makes them look ugly using the standard theme.
And also I have duplicated icons here which you seem not to have 🤔

Before:
grafik
After:
grafik

@andypugh
Copy link
Collaborator

Are there any objections to this? I don't use Gmoccapy so feel unqualified to do the merge.

@hansu
Copy link
Member

hansu commented Jun 19, 2025

I don't agree to the graphical changes except that one with the backspace-symbol.

And also I have duplicated icons here which you seem not to have 🤔

I was talking about duplicated icons - text and icons are fine. Maybe you misunderstood me...

And I don't really get the point with the cursor. Can you give an example of the behavior before and after?

@hansu
Copy link
Member

hansu commented Jun 24, 2025

And I don't really get the point with the cursor. Can you give an example of the behavior before and after?

@Sigma1912 ?

@Sigma1912
Copy link
Contributor Author

I don't have access to a linuxcnc installation to record the behavior but the issue is easily reproduced. Note that the issue does not appear if we use the keyboard to type, so make sure you use the mouse or touchscreen to press the buttons on the calculator widget:

Example 1

  1. open the calculator
  2. type '12345'
  3. try to use the mouse/touch to delete the '3' (ie but the cursor between the '3' and the '4' and press 'DEL')
  4. result is that the '5' is deleted.

Example 2

  1. open the calculator
  2. type '12345'
  3. try to use the mouse/touch to make the number negative (ie but the cursor to the left of the '1' and press the '-')
  4. result is that the '-' is added to the end '12345-'.

@hansu
Copy link
Member

hansu commented Jun 24, 2025

Ah good catch! I never noticed (used) that.

@hansu
Copy link
Member

hansu commented Jun 24, 2025

Your commit Allow using calculation result in new operations is a bit contrary to my change 4 years ago: 450dabe

I don't remember why I did it that way. @ others: opinions?

@Sigma1912
Copy link
Contributor Author

I don't agree to the graphical changes except that one with the backspace-symbol.

Generally I find the mix of icons and text labels somewhat unfortunate as it breaks the styling of the buttons. I can kind of understand the use of icons for buttons like 'OK' and 'Cancel' but I don't really see a point for any of the others.
In the end it is your decision.
Note that when using material icon themes apparently only the 'Pi' icon is shown.

@hansu
Copy link
Member

hansu commented Jun 24, 2025

It's just based on the old-style Gtk dialog buttons.

grafik

Yeah maybe the labels "Ok" and "Cancel" are superfluous as the icons should be clear enough.

@hansu
Copy link
Member

hansu commented Jun 24, 2025

Compromise?

grafik

@Sigma1912
Copy link
Contributor Author

sure, note though that the 'Pi' icon does not seem to handle the dark theme all that well:
pi

Unfortunately using the unicode '\u03C0' for the small greek letter 'pi' seems to be translated to the capital letter Pi.

@Sigma1912
Copy link
Contributor Author

Sigma1912 commented Jun 24, 2025

Seems that using a different font 'font-family: Arial, sans-serif;' solves the pi unicode issue:
arial

This way we could do without the pi icon, what do you think?

@Sigma1912
Copy link
Contributor Author

        self.wTree.get_object( "Pi" ).set_label('\u03c0')
        css = b"#calc_widget {font-family: Arial, sans-serif;}"

@hansu
Copy link
Member

hansu commented Jun 24, 2025

This way we could do without the pi icon, what do you think?

Yeah that's fine.

I just increased the font size for that symbol a bit (because it is a small letter)

#pi-symbol {font-size: 18px; font-family: sans-serif;}

https://github.com/hansu/linuxcnc/commits/calculatorwidget-respect-cursor-position

grafik

Note that when using material icon themes apparently only the 'Pi' icon is shown.

Hmm I get also the OK and Cancel button. But yes they should be added to the icon theme to be able to adapt to the material theme.
grafik

@hansu
Copy link
Member

hansu commented Jun 26, 2025

I have done the icon changes and added a prefix in the commit messages in this branch https://github.com/linuxcnc/linuxcnc/commits/calculatorwidget-respect-cursor-position
If you are fine with the changes, I suggest to force-push that branch into your branch so that we can merge it via GitHub.

@Sigma1912 Sigma1912 force-pushed the Gladevcp_calculatorwidget-respect-cursor-position branch from 720054a to 78ae6b5 Compare June 26, 2025 12:11
@hansu
Copy link
Member

hansu commented Jun 27, 2025

Weird that I am still the Committer of the commits that I rebased and you force pushed. However...
Thanks!

@hansu hansu merged commit 9c2ab7e into LinuxCNC:master Jun 27, 2025
16 checks passed
@Sigma1912 Sigma1912 deleted the Gladevcp_calculatorwidget-respect-cursor-position branch June 27, 2025 14:29
@hansu hansu mentioned this pull request Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants