rougier / svg-lib

Emacs SVG libraries for creatings tags, icons and bars
GNU General Public License v3.0
343 stars 31 forks source link

some screenshots of how the font doesn't play nice #14

Open japhir opened 2 years ago

japhir commented 2 years ago

In your reddit thread for svg-tag-mode you asked someone else to make file an issue for their font not working out nicely. I had the same issue and didn't see theirs yet so created this one here! :)

I created a nice clean new session with only svg-lib (and use-package) installed with the with-emacs.sh shell script by alphapapa.

Then I inserted a TODO tag as follows: 2021-12-29-16:10:01_region note that I also had some startup errors that I left in the screenshot

At first I thought it was the font in my own config, but the above image shows that on my system it also happens without making any changes from the emacs -Q config.

2021-12-29-16:11:25_region

vnckppl commented 2 years ago

Potentially related, so I am posting it here. I did set Roboto Mono as the default font, but my tags look off too. As @japhir the tags have that extra whitespace on the right, but also, the height of my tags are too big:

ss_20211229_122443

mskorzhinskiy commented 2 years ago

I'm using doom emacs & IBM Plex fonts. After some experimentation I've found that using a little bit bigger font size in the svg image fixes the issue for me. The patch:

1 file changed, 1 insertion(+), 1 deletion(-)
svg-lib.el | 2 +-

modified   svg-lib.el
@@ -280,7 +280,7 @@ and style elements ARGS."
                        (- tag-width stroke) (- tag-height stroke)
                        :fill background :rx (- radius (/ stroke 2.0)))
     (svg-text svg label
-              :font-family font-family :font-weight font-weight  :font-size font-size
+              :font-family font-family :font-weight font-weight  :font-size (+ 3.0 font-size)
               :fill foreground :x text-x :y  text-y)
     (svg-lib--image svg :ascent 'center)))

And before/after screenshot: svg_lib_before_after

I'd say this should be moved to the possible customization options. @rougier, should I prepare a patch to allow users to set a modifier to the font size?

pheraph commented 2 years ago

I (also doom emacs with IBM Plex) added a font-size modifier to correct the small font issue. But I changed it at line 245 so the modified size is used for the width an position calculations:

         (font-size   (+ 6.0 (plist-get style :font-size)))
Bildschirmfoto 2022-01-03 um 12 23 13

Don't know if this can be fixed in general, but a customization option would be definitely helpful for me, too.

rougier commented 2 years ago

Thanks for all the screenshots. It's a recurrent problem that for some unknown reason, the size is plainly wrong for some users. I suspect the os is "messing" with real cs virtual pixel but I'm not quite sure. Could each of test the following code:

(require 'svg)
(global-font-lock-mode 0)

(defun r-text (text family size)
    (propertize text
        'face `(:family ,family
                :height ,(* size 10)
                :weight regular
                :foreground "black"
                :box '(:linewidth -1 :color "black" :style nil))))

(defun s-text (text family size &optional)
  (let* ((font       (font-info (format "%s-%d" family size)))
         (font-size  (elt font 2))
         (descent    (elt font 9))
         (ascent     (elt font 8))
         (svg-height (elt font 3))
         (char-width (elt font 11))
         (svg-width  (* char-width (length text)))
         (svg        (svg-create svg-width svg-height)))
    (svg-rectangle svg 0 0 svg-width svg-height
           :fill "white" :stroke "black")
    (svg-text svg text
              :fill "black"
              :stroke-width 0
              :kerning 0
              :letter-spacing 0
              :font-family family
              :font-size font-size
              :x 0
              :y ascent)
    (propertize text 'display (svg-image svg :ascent 'center))))

(insert (r-text "Hello world!" "Roboto Mono" 8))
(insert (s-text "Hello world!" "Roboto Mono" 8))

(insert (r-text "Hello world!" "Roboto Mono" 12))
(insert (s-text "Hello world!" "Roboto Mono" 12))

(insert (r-text "Hello world!" "Roboto Mono" 24))
(insert (s-text "Hello world!" "Roboto Mono" 24))

(insert (r-text "Hello world!" "Roboto Mono" 14))
(insert (s-text "Hello world!" "Roboto Mono" 14))

This is what I get:

Screenshot 2022-01-03 at 17 36 37
mskorzhinskiy commented 2 years ago

Here are the result with IBM Plex Mono:

IBM Plex Mono

Another note would be that I'm running Ubuntu 21.10 and 18.10. The problem is the same on both OS.

rougier commented 2 years ago

@mskorzhinskiy Thanks. Do you know if you have a setting for "scaling" desktop on your system? See for example https://unix.stackexchange.com/questions/596887/how-to-scale-the-resolution-display-of-the-desktop-and-or-applications

mskorzhinskiy commented 2 years ago

I have tried to check the scaling, but I don't see if any of these settings are set to anything other then "1.0". I've tried to use different screens and different DPI settings and it didn't affect the bug.

However I see your point, I guess this is indeed some kind of OS problem, i.e. the problems somewhere out of the scope of this particular project. I'll try to research more about this problem on my machines.

mskorzhinskiy commented 2 years ago

... or may be not. I've fixed the code snippet you've provided, @rougier. In the (svg-text... I've changed ":font-size size" into ":font-size size" and I got the following picture:

IBM Plex With Fix

rougier commented 2 years ago

Oh nice. But I did not get the change you made (you wrote "I've changed :font-size size into :font-size size")

mskorzhinskiy commented 2 years ago

Sorry for my cumbersome language. Here is the diff:

.@@ -22,7 +22,7 @@
               :stroke-width 1
               :kerning 0
               :font-family family
-              :font-size size
+              :font-size font-size
               :x 0
               :y ascent)
     (propertize text 'display (svg-image svg :ascent 'center))))

I have applied the same fix to the svg-lib and it worked for me too. I'll prepare a PR shortly.

vnckppl commented 2 years ago

By request: ss_20220103_130407

rougier commented 2 years ago

@mskorzhinskiy Ok, thanks, I've corrected the snippet. What is size / font-size in your case ? @vnckppl Thanks. We've just corrected the snippet (changing size in font-size). Can you check if the new version gives the same result?

vnckppl commented 2 years ago

Yes, that fixes it for me: ss_20220103_130817

rougier commented 2 years ago

Great!

mskorzhinskiy commented 2 years ago

@vnckppl, @pheraph, @japhir: could you please test the PR #15? If you're using doom emacs, you can do this with this package directive (and repo update of course):

(package! svg-lib
  :recipe (:host github :repo "mskorzhinskiy/svg-lib"))
rougier commented 2 years ago

@mskorzhinskiy I think you can submit a PR from the results we got so far. Make sure to sign Emacs Copyright assignment if you want to contribute further (for this simple fix, it is not formally necessary).

pheraph commented 2 years ago

Reverted my changes and tested the PR #15, but the problem remains. I am running macOS, so this may still fix the issue with Ubuntu.

Bildschirmfoto 2022-01-04 um 11 31 19
rougier commented 2 years ago

@pheraph Can you run the code I posted in this thread and post the results?

japhir commented 2 years ago

Perhaps (likely) I'm making a stupid mistake, but when I run the code above, I get the following warning when running any of the s-text function calls:

Debugger entered--Lisp error: (wrong-type-argument number-or-marker-p nil)
  *(nil 12)
  (let* ((font (font-info (format "%s-%d" family size))) (font-size (elt font 2)) (descent (elt font 9)) (ascent (elt font 8)) (svg-height (elt font 3)) (char-width (elt font 11)) (svg-width (* char-width (length text))) (svg (svg-create svg-width svg-height))) (svg-rectangle svg 0 0 svg-width svg-height :fill "white" :stroke "black") (svg-text svg text :fill "black" :stroke-width 0 :kerning 0 :letter-spacing 0 :font-family family :font-size font-size :x 0 :y ascent) (propertize text 'display (svg-image svg :ascent 'center)))
  s-text("Hello world!" "Roboto Mono" 14)
  (insert (s-text "Hello world!" "Roboto Mono" 14))
  (progn (insert (s-text "Hello world!" "Roboto Mono" 14)))
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  funcall-interactively(eval-last-sexp nil)
  command-execute(eval-last-sexp)

at first I thought it was because I hadn't (require 'svg-lib)'d yet, but even after doing that and doing (require 'svg-tag-mode) the first function calls to r-text just print Hello World in normal text, and the second one always throws the above error.

I'm on GNU Emacs 28.0.60 (build 1, x86_64-pc-linux-gnu, GTK+ Version 3.24.30, cairo version 1.17.4) of 2021-11-01

rougier commented 2 years ago

Can you try (message "%s" (font-info "Roboto Mono-14")) and post the result?

pheraph commented 2 years ago

I apparently don't understand what I am doing and how it might be helping, but here are my results:

grafik
pheraph commented 2 years ago

Perhaps (likely) I'm making a stupid mistake, but when I run the code above, I get the following warning when running any of the s-text function calls:

I got the same error when the Roboto font wasn't installed. I think you have to replace the font name with the font you are using.

rougier commented 2 years ago

Yes, I forgot to specify to test the code with a font that is installed on your system. In the end, it seems to be working.

japhir commented 2 years ago

Ah whoops! Here it is in a brand new with-emacs.sh session with only these packages installed:

2022-01-04-15:25:23_region

rougier commented 2 years ago

@japhir. If you want to compare with pure text in the same buffer, disable font locking.

japhir commented 2 years ago

Ah great, here it is!

2022-01-04-16:00:10_region

and below all the code I had to evaluate to get this working in a clean with-emacs.sh -i svg-tag-mode -i use-package -i svg-lib session:

code to disable font-locking and run tests with Iosevka font ``` (require 'svg-lib) (global-font-lock-mode 0) (defun r-text (text family size) (propertize text 'face `(:family ,family :height ,(* size 10) :weight regular :foreground "black" :box '(:linewidth -1 :color "black" :style nil)))) (defun s-text (text family size &optional) (let* ((font (font-info (format "%s-%d" family size))) (font-size (elt font 2)) (descent (elt font 9)) (ascent (elt font 8)) (svg-height (elt font 3)) (char-width (elt font 11)) (svg-width (* char-width (length text))) (svg (svg-create svg-width svg-height))) (svg-rectangle svg 0 0 svg-width svg-height :fill "white" :stroke "black") (svg-text svg text :fill "black" :stroke-width 0 :kerning 0 :letter-spacing 0 :font-family family :font-size font-size :x 0 :y ascent) (propertize text 'display (svg-image svg :ascent 'center)))) (insert (r-text "Hello world!" "Iosevka" 8))Hello world! (insert (s-text "Hello world!" "Iosevka" 8))Hello world! (insert (r-text "Hello world!" "Iosevka" 12))Hello world! (insert (s-text "Hello world!" "Iosevka" 12))Hello world! (insert (r-text "Hello world!" "Iosevka" 24))Hello world! (insert (s-text "Hello world!" "Iosevka" 24))Hello world! (insert (r-text "Hello world!" "Iosevka" 14))Hello world! (insert (s-text "Hello world!" "Iosevka" 14))Hello world! ```
tam5 commented 1 year ago

I haven't fully tracked down the why/where this is happening, but it seemed at least for me (Doom Emacs) that (svg-lib-style-compute-default) was setting the font correctly, but then the font info was getting reset back to the default values. This resulted in svg-lib inserting svgs with a different font/size than the buffer, which resulted in the weird vertical alignment issues.

So, for me basically adding this fixed it:

(add-hook 'after-setting-font-hook (lambda () (setq svg-lib-style-default (svg-lib-style-compute-default))))

Could be there is a much more straightforward way to fix.

rougier commented 1 year ago

@tam5 Thanks, we're making progress. That might be linked to https://github.com/rougier/svg-tag-mode/issues/38 (see end of thread) where @ronisbr set the default font explicitely. Maybe we can merge the two approaches since only the default fon/size needs to be modified.