-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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: Make the font used by the application configurable and other UI fixes #20933
Conversation
Also, improve code style a bit by adding blank lines here and there.
Changes to that font require a restart, so this also adds the ability to register one when creating font groups.
This is not necessary anymore
Also, set font for restarter app.
The problem is Qt adjusts em quantities according to the font size used in the app.
- Correctly align all widgets to the left. - Fix uneven spacing. - Make right and left margins to match. - Increase width a bit and set it to be the min width so that the layout is not affected when resizing the widget.
Also, restore previous cell icon to be used as such in the Outline plugin.
Thanks for the work here @ccordoba12 ! Quick question, could this be related with spyder-ide/ux-improvements#31 ? |
Good point @dalthviz! I think it is. I mean, that issue was about packaging in our installers and setting by default a nice monospaced font for the Editor and IPython console. With the work I did here, it will be possible to do the same but for the entire application. That way Spyder will look exactly the same in all operating systems, regardless of the font set by users for the other apps. |
Another important point I forgot to mention is that this will be useful for users of languages like Chinese, Japanese or Korean on Windows because they could select the font they want for the app, instead of being forced to use some ugly Latin font. I've seen that in a lot of screenshots (e.g. the first one on issue #5942) when users want to use Spyder in English but the OS is in one of those languages. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ccordoba12 ! Left a couple of comments and I'm missing to test this locally but overall LGTM 👍
That's not the case for me on Linux, i.e. all elements change their font. So, I'll have to check on Windows. But did you restart Spyder? Perhaps Spyder didn't ask you to do it, which could be the problem in your case. |
Applying the preference showed me the dialog to restart and Spyder was restarted succesfully. Checked on macOS and seems like there are elements which font is still different from the one set. Seems like more elements are affected like labels, buttons and the status bar with macOS 🤔: Also, I closed and reopened Spyder but the partial font change behavior is still there. Maybe some Qt setting needs to be enabled for Windows/macOS? |
This allows us to simplify the code in several places.
This seems to be a problem on Windows and Mac, but not on Linux.
Also remove footer on main page because it doesn't look good
- Remove vertical header in collections editor because it's not necessary and automatically resize columns when the widget is shown. - Remove some hard-coded styles in dataframe editor.
874baed
to
4451759
Compare
I think I fixed that in my last commit. Please check again.
It was missing but it should be fixed now too.
It seems that making this work on Mac is either really difficult (I was not expecting that the issues you found were still present after my last changes) or not possible at all (I'm not sure how to fix them). So, I'd say for now we should disable this feature on Mac, if that's ok for you (that would be helpful for me too because I've invested a lot of time on this and I'd like to move on). In addition, I think this would be mainly useful for Chinese, Japanese and Korean users on Windows (they requested it), and Linux ones (for testing and also because it works almost without issues there). |
Checked again and seems like there is still a thing regarding the size of the Variable Explorer header not expanding as stayed in the OP: But other than that seems like things are working on Windows 👍 Regarding macOS, I guess that is an option, but then on macOS things will work as before regarding the rich text font? And then users will not see the new preference regarding an |
Also, cache font used in ArrayModel for performance.
That's odd because it works for me with However, the solution I implemented seems to work for some fonts and fonts sizes, but not for others (this is with the same font at two different sizes): So, I guess this depends on the algorithm used by Qt to adjust the columns width to their headers' contents, which should use QFontMetrics to compute the length of each header. And that probably doesn't work very well for some fonts and sizes. However, if we'd try to implement the algorithm ourselves, I think we'd arrive at something very similar to what Qt already does. So, I guess what I did in this PR is the best we can do to improve the situation, although it's not perfect (and it'll probably never be).
Great! That's really good news.
Nop, we'd simply won't allow users to uncheck the I pushed a commit that implements that solution, so let me know if it works for you after resetting Spyder options. |
Probably even further it depends on the display dpi or something like that. Following that maybe adding some link to the discussion here around the code that parcially fixes the headers could be nice so there is a reference indicating that there is not much else to do about the header situation.
Seems like that is working 👍 Not sure if there is something else that could be done to improve the UX to handle the application font option in a cross platform way. Maybe @conradolandia could assess if what we have here is okay/makes sense? Or maybe you have already discussed this PR during the UX/UI meetings @ccordoba12 ? Also, a CI job was failing so I re-runned it, hopefully it will pass 🤞🏼 |
I've been reading QFontMetrics documentation, and this called my attention, regarding this issue (emphasis mine):
Maybe the font metric object needs to be updated after a font change? |
I think it makes sense. It's a very cool feature to be able to choose how Spyder can be themed, and this is a significant feature towards that. It seems strange some elements don't take the change into account, tho. I think unifying Font sources is important. |
Ok, I'll add a comment about it.
Great!
Yep, we already do that. But it seems some widgets on Mac don't pick the font selected for the rest of Spyder, and I don't know why. In addition, I don't have a Mac to try to fix that.
Thanks @conradolandia for your feedback! I totally agree with you. However, I'd prefer to be a bit more pragmatic here to keep things moving. If later Mac users request this feature loudly, then I could spend more time to make it work there. But for now, I think having it on Windows and Linux is a huge improvement for the project, as you said. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ccordoba12 ! Left some comments and a suggestion but other than that this LGTM 👍
This is not necessary because we already have a font cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @ccordoba12 ! Gave another check with the latest changes and this LGTM
However, seems like changing the font makes some errors regarding cache appear in the console that I used to launch Spyder:
[6120:15964:0626/163529.150:ERROR:cache_util_win.cc(20)] Unable to move the cache: Acceso denegado. (0x5)
[6120:15964:0626/163529.151:ERROR:cache_util.cc(140)] Unable to move cache folder C:\Users\dalth\AppData\Local\Spyder\cache\QtWebEngine\Default\Cache to C:\Users\dalth\AppData\Local\Spyder\cache\QtWebEngine\Default\old_Cache_000
[6120:15964:0626/163529.152:ERROR:disk_cache.cc(184)] Unable to create cache
qt.gui.icc: fromIccProfile: failed minimal tag size sanity
Also, when you select again the Use the system default interface font
after changing the values is like those values reset (font and size) and get replaced with the default values. So seems like you can't have a previous selection for the application font and size while using the default system font, right?
I don't have any strong opinion about the font and size selection, and not totally sure if the cache error messages are something to address or just something transient or that can be ignore but letting you know just in case.
Anyhow, I'm approving so if you are good with the things above feel free to merge or let me know and I can click the merge button 👍
Those errors seem unrelated to this and I didn't see them myself when testing on Windows. I'd say they are probably caused because at some point you launched Spyder as admin in your user account.
Yeah, that's the behavior I decided to implement for several reasons:
Thanks @dalthviz! I'll merge then to keep things moving. |
Description of Changes
Font related changes
Rich text
entry in Preferences byInterface
and renamePlain text
toMonospace
.SpyderFontType
enum to access the different fonts we have in Spyder.SpyderFontsMixin
to get those fonts as QFont objects to be used in Qt widgets or other objects.Online Help
use the font size of the rest of the app (this was not working).Index
column in our dataframe editor. This was reported by a user to be a UI inconsistency and I agree with them.Other changes
new_cell
.Plots
is shown (the previous hack was not working anymore).Visual changes:
Replace the previous
Rich text
entry in Preferences with anInterface
one:Fonts and layout of the "Report error" dialog were not the best
Before
After
Restore cell icon in the Outline pane
Before
After
Resize Variable Explorer columns at startup
Before
After
Hide vertical header in Variable Explorer because it's not necessary.
Before
After
Use the monospace font for the
Index
column in our Dataframe editorBefore
After
Scale size of monospace font to match the interface one when the latter is changed. That's applied to plugins that are not directly related to code, like the Variable Explorer.
Issue(s) Resolved
Fixes #20960
Fixes #5942
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: