Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: Ensure QFont.setPointSize receives an int argument (Shortcuts) #17646

Merged
merged 2 commits into from
Apr 13, 2022

Conversation

juliangilbey
Copy link
Contributor

Description of Changes

Thanks for your work on this version! I'm close to getting it working for Debian....

The test spyder/plugins/shortcuts/widgets/tests/test_summary.py::test_shortcutssummary fails with Python 3.10 because QFont().setPointSize() requires an integer argument, and Python no longer performs implicit float -> integer conversions. This patch fixes the cause.

Affirmation

By submitting this Pull Request or typing my (user)name below,
I affirm the Developer Certificate of Origin
with respect to all commits and content included in this PR,
and understand I am releasing the same under Spyder's MIT (Expat) license.

I certify the above statement is true and correct: juliangilbey

@juliangilbey juliangilbey changed the base branch from master to 5.x April 8, 2022 12:46
@dalthviz dalthviz changed the title Ensure QFont().setPointSize() receives an int argument PR: Ensure QFont().setPointSize() receives an int argument Apr 8, 2022
@dalthviz dalthviz added this to the v5.3.1 milestone Apr 8, 2022
@dalthviz dalthviz closed this Apr 8, 2022
@dalthviz dalthviz reopened this Apr 8, 2022
@dalthviz
Copy link
Member

dalthviz commented Apr 8, 2022

Note: This depends on #17641 for the tests to pass

@dalthviz
Copy link
Member

Hi @juliangilbey thank you for the fix! Now you can do a rebase to the latests 5.x to fix the tests. Let us know if you need help with that

@juliangilbey
Copy link
Contributor Author

Ooh, I have done a rebase on a pull request before, but I don't remember exactly how; a quick reminder would be helpful, so that I don't mess this up!

@dalthviz
Copy link
Member

Sure! So you will need to have/set the original Spyder repo as a new remote like for example upstream and fetch it. So something like:

git remote add upstream https://github.com/spyder-ide/spyder.git
git fetch upstream

Then be sure to have your branch checkout and rebase using the 5.x from the new upstream remote. So something like this:

git checkout make_point_size_integer
git rebase upstream/5.x

Finally force push the changes with something like:

git push -f

@juliangilbey
Copy link
Contributor Author

OK, thanks! I thought it was as simple as that, but didn't want to force-push without being sure 😄 Have done it now - all being well, the tests will pass!

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your help with this @juliangilbey! I left a small comment for you, otherwise looks good for me.

@@ -45,7 +45,7 @@ def __init__(self, parent=None):
# Calculate font and amount of elements in each column
# according screen size
width, height = self.get_screen_resolution()
font_size = height / 80
font_size = height // 80
Copy link
Member

@ccordoba12 ccordoba12 Apr 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
font_size = height // 80
font_size = int(round(height / 80))

I think this is the right fix. Otherwise we could end up using the wrong font size below.

Copy link
Contributor Author

@juliangilbey juliangilbey Apr 12, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand what you mean. height // 80 is equivalent to math.floor(height / 80), but int(height / 80) might do rounding or truncation, depending on the behaviour of the C library (see note (3) to the table in https://docs.python.org/3/library/stdtypes.html#numeric-types-int-float-complex). (Note (1) of that table says that x // y is an integer value but not necessarily of type int; in the case that both operands are of type int, as in the situation here, the result is of type int.) It seems to make sense here to be explicit as to what is wanted: if truncation/floor is wanted, then height // 80 does the job, as would math.floor(height / 80); if rounding is preferred, then int(round(height / 80)) would be needed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if rounding is preferred, then int(round(height / 80)) would be needed

Ok, I changed my suggestion accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It depends whether we want 80 * font_size <= height, in which case the correct thing to do is height // 80, or whether it is fine to have 80 * font_size > height, in which case int(round(height / 80)) works better. For example, using rounding, heights between 760 and 839 will give a font_size of 10, while using integer division will give a font_size of 9 for heights of 720 up to 799 and a font_size of 10 for 800 up to 879.
I'm happy to go with whichever you wish!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go with int(round(height / 80)).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, just pushed a commit making this change.

@ccordoba12 ccordoba12 changed the title PR: Ensure QFont().setPointSize() receives an int argument PR: Ensure QFont.setPointSize receives an int argument (Shortcuts) Apr 11, 2022
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for you help with this @juliangilbey!

@ccordoba12 ccordoba12 merged commit 48a9574 into spyder-ide:5.x Apr 13, 2022
ccordoba12 added a commit that referenced this pull request Apr 13, 2022
@juliangilbey juliangilbey deleted the make_point_size_integer branch April 14, 2022 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants