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

Handle changes to the "Xft/DPI" setting #2357

Closed
wants to merge 2 commits into from

Conversation

rkraats
Copy link

@rkraats rkraats commented Nov 20, 2017

GTK uses the XSETTINGS Xft/DPI value to determine the text size. A change
of this value causes e.g. the window title text to be adjusted. However,
the window's contents are currently not adjusted.

By connecting a callback handler to the "notify::gtk-xft-dpi" signal,
the gui.text_context and font are re-initialized on a change of this
setting, similar to how the "screen-changed" signal is processed.

A slight difference with that "screen-changed" handler is how the
desired font is chosen; in case of an empty p_guifont value, the default
font is forced. Otherwise in case no specific font is configured, the
initially selected (default) font is forgotten about.

This change was tested on Fedora 26 running Xfce, with command:
xfconf-query -c xsettings -p /Xft/DPI -s <dpi>
with <dpi> e.g. 96 or 144.

I hope my change fits the standards and doesn't get things completely
wrong. Please merge if applicable.

GTK uses the XSETTINGS Xft/DPI value to determine the text size. A change
of this value causes e.g. the window title text to be adjusted. However,
the window's contents are currently not adjusted.

By connecting a callback handler to the "notify::gtk-xft-dpi" signal,
the gui.text_context and font are re-initialized on a change of this
setting, similar to how the "screen-changed" signal is processed.

A slight difference with that "screen-changed" handler is how the
desired font is chosen; in case of an empty p_guifont value, the default
font is forced. Otherwise in case no specific font is configured, the
initially selected (default) font is forgotten about.
@nuko8
Copy link

nuko8 commented Nov 21, 2017

I can't seem to understand the point of the issue. I think the only purpose of changing the Xft/DPI value during a session is to find out its correct value in case what Xft automatically detected or an rc file set it to, turns out to be incorrect. Also, I think allowing us to do that is by no means meant for adjusting text size of applications to make them look nicer at runtime. If any correction is needed about it, that should be done in the course of windowing system initialization, and this should be easily done because DPI is a fixed value for a given display and its actual, true value must have been known to the system administrator once she/he found it went wrong with automatic detection. Also, it'd look clumsy to find out the correct value by switching between DPI's once a normal session has started. Of course, that's my personal view. So, say, If you could show me use cases where the proposed feature is indispensable, that would definitely help me understand the PR well.

@rkraats
Copy link
Author

rkraats commented Nov 21, 2017

Thanks for looking into my pull request, Kazunobu!

I can't seem to understand the point of the issue. I think the only purpose of changing the Xft/DPI value during a session is to find out its correct value in case what Xft automatically detected or an rc file set it to, turns out to be incorrect. Also, I think allowing us to do that is by no means meant for adjusting text size of applications to make them look nicer at runtime. If any correction is needed about it, that should be done in the course of windowing system initialization, and this should be easily done because DPI is a fixed value for a given display and its actual, true value must have been known to the system administrator once she/he found it went wrong with automatic detection. Also, it'd look clumsy to find out the correct value by switching between DPI's once a normal session has started. Of course, that's my personal view. So, say, If you could show me use cases where the proposed feature is indispensable, that would definitely help me understand the PR well.

My use case is that I (have to) change the DPI setting when I switch between my laptop's screen and an external display. I'm running Fedora 26 with Xfce in a virtual machine, and Xfce and/or X11 are not aware of a change of screen. Like most desktop environment, Xfce allows for changing the DPI value on the fly, and most applications directly respond to it, except gvim (and Firefox, but that's another story ;-) ).

Some googling for 'gtk-xft-dpi' shows that other project use the same approach. See e.g.
https://mail.gnome.org/archives/commits-list/2014-March/msg04097.html

@nuko8
Copy link

nuko8 commented Nov 21, 2017

Thanks, that definitely makes sense. I'll soon review the code...

@nuko8
Copy link

nuko8 commented Nov 21, 2017

@rkraats

The code block connecting gtk_settings_xft_dpi_changed_cb to notify::gtk-xft-dpi is guarded with #if GTK_CHECK_VERSION(3,0,0). But the functions used there are those introduced to GTK+ since 2.2, and the minimum version required for Vim to be built is just that version. So it looks the patch also works for 2.0 as well. Was there a reason to exclude 2.0? If not, it would be very nice if you would check that it works well with 2.0. Thanks.

@rkraats
Copy link
Author

rkraats commented Nov 21, 2017

Was there a reason to exclude 2.0?

Actually I just copied that #if from other g_signal_connect() calling places. I'll see if I can find a way to test it with GTK+ 2.

@nuko8
Copy link

nuko8 commented Nov 21, 2017

Great. Plus, to suppress compiler warnings regarding unused parameters, would you slightly modify the header part of the callback function to

gtk_settings_xft_dpi_changed_cb(GtkSettings *gtk_settings,
                                GParamSpec *pspec UNUSED,
                                gpointer data UNUSED)

where UNUSED is a macro which is to be expanded to __attribute__((__unused__)) if the compiler supports the attribute.

Xft/DPI change handling is also supported in GTK+ 2, so remove check for
GTK+ 3.
Mark unused function arguments with 'UNUSED'.
@rkraats
Copy link
Author

rkraats commented Nov 21, 2017

I tested with building and running against GTK+ 2 (gtk2-2.24.31-4 on Fedora 26) and since that works fine as well, I removed the check for GTK+ 3. I also added the 'UNUSED' attributes.

@rkraats
Copy link
Author

rkraats commented Nov 30, 2017

@nuko8, can you please indicate whether the latest version of this pull request is a candidate for inclusion? Thanks!

@nuko8
Copy link

nuko8 commented Nov 30, 2017

Personally I'd say "LGTM." Note, however, that the last word is always Bram's; any inclusions are done when he considers it appropriate, and that it often happens that a PR is listed in TODO (:help todo.txt) for further review or comments. So please be patient :) In the meantime, updates for further improvements/bug fixes, if any, are encouraged.

@nuko8
Copy link

nuko8 commented Nov 30, 2017

Sorry, I misspelled Bram's name. I already corrected my previous comment I posted to GitHub.

@rkraats
Copy link
Author

rkraats commented Nov 30, 2017

Clear, thanks for explaining the process, Kazunobu!

@brammool
Copy link
Contributor

brammool commented Nov 30, 2017 via email

@brammool brammool closed this in 7ebf4e1 Aug 7, 2018
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.

None yet

3 participants