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

Font scaling (mostly for Linux) #441

Closed
wants to merge 2 commits into from
Closed

Conversation

Necoro
Copy link
Contributor

@Necoro Necoro commented Feb 1, 2022

DO NOT MERGE BLINDLY

As stated in #437, currently no font scaling happens on Linux, but it seems to be needed. This PR wants to look into this issue a lil' bit...

Changes in this PR:

  • enable font scaling on Linux (and technically also Mac, but GetContentScale always returns 1 there)
  • Fixed: when scaling was used, extraFonts would not load, because the scaled size was written as a key.

Open points so far:

  • if I see this correctly, DPI settings are not updated. I.e. when moving the window to another monitor with other DPI settings, those do not apply. Shall this be fixed, or is this intended?
  • there is also a call to style.ScaleAllSizes in MasterWindow.go#setTheme, that is only executed on Windows. Enabling this also in Linux -- shows no difference. What should I expect here?

@macbutch, @HACKERALERT: As you were also dealing with DPI scaling issues (#350), do you have any input here? What platforms were you on?

The `fontInfo` element in the loop is local, changes to its size member
are not kept.
BUT: The call to `.String()` for entry in the `extraFonts` map uses that
size information -- yielding a key that is not reproducable.

Fix: Use a local variable for the scaled size.
@Necoro Necoro marked this pull request as draft February 1, 2022 22:13
@codecov-commenter
Copy link

Codecov Report

Merging #441 (af164d2) into master (eb9f90e) will increase coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #441   +/-   ##
======================================
  Coverage    2.91%   2.91%           
======================================
  Files          30      30           
  Lines        3122    3121    -1     
======================================
  Hits           91      91           
+ Misses       3031    3030    -1     
Impacted Files Coverage Δ
FontAtlasProsessor.go 0.00% <0.00%> (ø)
canvas.go 0.00% <0.00%> (ø)
msgbox.go 0.00% <0.00%> (ø)
markdown.go 0.00% <0.00%> (ø)
ExtraWidgets.go 0.00% <0.00%> (ø)
ImageWidgets.go 0.00% <0.00%> (ø)
utils.go 29.80% <0.00%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb9f90e...af164d2. Read the comment docs.

@HACKERALERT
Copy link
Contributor

I've really only tested on Windows. I compile giu programs on macOS and Linux, but since I do it via VirtualBox which doesn't support OpenGL, I can't see what the compiled program looks like. So I probably can't lend much wisdom here.

@gucio321
Copy link
Collaborator

@Necoro may I ask what is stat of this PR right now? It looks like it is draft since over a year...

@Necoro
Copy link
Contributor Author

Necoro commented May 10, 2023

@gucio321 I used the my changes in my private build on Linux and Windows and it worked. But it was along the lines of "I dont really know what I'm doing".

The projects I used it in where one-shot uses (more or less) and I do not use giu anymore. Therefore I've not that much motivation on porting the patch to the current HEAD.

If in doubt, close this PR. The changes are rather minor and can easily be re-done by any interested party.

@gucio321
Copy link
Collaborator

I see. So I think it should be marked as ready-for-review.
But @AllenDang could you take a look? I don't know what it does too 😄

@gucio321 gucio321 marked this pull request as ready for review May 11, 2023 13:13
@gucio321
Copy link
Collaborator

closing due to age and #628

@gucio321 gucio321 closed this Sep 22, 2023
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.

4 participants