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

Fix calculations and hard-code less #14

Closed
wants to merge 7 commits into from

Conversation

tarsius
Copy link
Contributor

@tarsius tarsius commented Apr 30, 2021

Closes #12.

All but the last commit perform some cleanup. The last commit makes the big changes. I could have tried to split that into multiple commits, but don't think the work involved in that would be worth it.

There were four major issues:

  1. image-compute-scaling-factor was not taken into account. (While that says "image", the same factor applies to faces.)
  2. Calls to face-attribute did not use the inherit property, which lead to lots of duplication, complications and probably some bugs.
  3. Likely in order to deal with issues caused by (1) and (2) fonts were hard-coded, hiding issues and increasing the likelihood this breaks completely for many users (who don't have the font installed).
  4. The calculations seemed a bit obfuscated at times. ;)

I'll probably be adding svg-tag--make to forge (under a different name of course ;) and use it for labels. Or maybe even for refnames in magit. That's why I looked into svg-tag-mode in the first place!

@tarsius
Copy link
Contributor Author

tarsius commented Apr 30, 2021

Uh, I should not that I have not tested this much. I drafed this code and then let it sit back when I brought up the issue; and how I have cleaned it up a bit because of the activity in #12.

It seems like a good idea to abstract this complication
away so one doesn't have to worry about it when dealing
with the size calculations.
Fix bugs
- Fix the essential size calculation (see "(ceiling ...)")
- Use the INHERIT argument of `face-attribute'

Also cleanup
- Simplify calculations
- Use more descriptive variable names

Remove variable only needed to paper over fixed bugs
- `svg-tag-vertical-offset'
- `svg-tag-horizontal-offset'

Remove variable `svg-tag-default-line-width'.  Instead support
box-less tags.

Move the calculations and formatting to a new function
`svn-tag--make', while `svn-tag-make' now only takes care of
passing along values to the former.
@jamesnvc
Copy link

jamesnvc commented Apr 30, 2021

This seems to work for me!

EDIT: Although to make the examples work, I do need to manually set a different :height on the faces to make them be the correct size.

@jamesnvc
Copy link

I think I also have a fairly weird display set up...I have xorg conf file setting the DPI to 144, but also Xft.dpi: 180 in my ~/.Xresources; fair enough if Emacs is confused then, I certainly am.

@rougier
Copy link
Owner

rougier commented Apr 30, 2021

Thank you! Impressive, I've a lot to learn . I'll test it on my side but if no complaints, I'll merge.

@jamesnvc
Copy link

I'm very curious how the drawing works internally...I continue to see that omitting the :font-size in the call to svg-text makes the text show as the correct size (in conjunction with changing the text-y to (- txt-height 5) instead of font-size), while normally it's way too small and in the top-left corner. I don't know if it's worth trying to figure out what calculation is needed -- maybe it's just my weird setup that necessitates this -- or just set a much-larger font size for the svg face.

@rougier
Copy link
Owner

rougier commented Apr 30, 2021

Could post some screenshots showing the problem?

@jamesnvc
Copy link

Opening example-2.el & eval-ing with emacs -Q:
screenshot-2021-04-30_15-50-49

Commenting out the :font-size line in svg-tag--make and repeating:
screenshot-2021-04-30_15-59-25

Commenting out :font-size and changing (text-y font-size) to (text-y (- txt-height 5)) in svg-tag--make:
screenshot-2021-04-30_16-00-24

@rougier
Copy link
Owner

rougier commented May 1, 2021

What does (frame-monitor-attributes) returns ?

@tarsius
Copy link
Contributor Author

tarsius commented May 2, 2021

Well, it seems my calculations and/or there are values I am not taking into account are off too. :/

So I very much welcome it if other people also try to figure out why it doesn't work for them.

For debugging purposes I recommend editing svg-tag--make to return svg instead of (svg-image svg ...) and pretty-print the value.

@jamesnvc
Copy link

jamesnvc commented May 2, 2021

What does (frame-monitor-attributes) returns ?

((name . "DP-1") (geometry 0 0 3840 2160) (workarea 0 0 3840 2160)
 (mm-size 610 350) 
 (frames #<frame *scratch* - GNU Emacs at gonk 0x56359771c210>) (source . "Gdk"))

Well, it seems my calculations and/or there are values I am not taking into account are off too. :/

It is entirely possible I've just done something so bizarre with my DPI settings that Emacs is doing the wrong thing somewhere...although I do find it suspicious that not passing :font-size to svg-text does the right thing.

The value of svg seems reasonable enough; for example, this is svg-tag-org-todo from example-2.el:

(svg 
 ((width . 60) (height . 23) 
  (version . "1.1") (xmlns . "http://www.w3.org/2000/svg")
  (xmlns:xlink . "http://www.w3.org/1999/xlink"))
 (rect
  ((width . 50) (height . 21)
   (x . 5.0) (y . 0) (rx . 2)
   (fill . "#FFAB91")))
 (rect
  ((width . 49.0) (height . 20.0)
   (x . 5.5) (y . 0.5)
   (rx . 1.5) (fill . "#FFAB91")))
 (text
  ((y . 10.040268456375841) (x . 10.0)
   (fill . "#ffffff") (font-size . 10.040268456375841)
   (font-weight) (font-family . "PragmataPro Liga"))
  "TODO"))

@rougier
Copy link
Owner

rougier commented May 3, 2021

From https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/font-size, it seems the default font-size is medium. Can you try specifying this font size and check if you get the same result as when not specifying the font size?

@jamesnvc
Copy link

jamesnvc commented May 3, 2021

Hm, "medium" seems to make the text too big:

screenshot-2021-05-03_09-18-10

I did note that setting the font-size to be "1em" appears to be the correct size, albeit misaligned vertically:

screenshot-2021-05-03_09-19-49

@rougier
Copy link
Owner

rougier commented May 3, 2021

Interesting. I'm not sure how Emacs or librsvg computes the 1em. Do you know which svg library your emacs is using?
For the vertical alignment, I think it might be font dependent and there nothing much we can dot but adapt manually the offset.

@jamesnvc
Copy link

jamesnvc commented May 3, 2021

Do you know which svg library your emacs is using?

Looks like librsvg2 2.48.9 (I've built it myself from git).

I'm not sure how Emacs or librsvg computes the 1em

Reading svg_load_image in image.c, it seems like it pretty much just passes the content to rsvg. I'll have a look at rsvg and see if I can figure out what it's doing.

For the vertical alignment, I think it might be font dependent and there nothing much we can dot but adapt manually the offset.

Yeah...might be able to compute something from window-font-height?

@jamesnvc
Copy link

jamesnvc commented May 3, 2021

Ah, I see that it injects CSS into the SVG that sets a default font-size and font-family from the face used to display the image. Unsure then why it's different from the font-size set on the text element...

@jamesnvc
Copy link

jamesnvc commented May 3, 2021

Another interesting data point; I tried setting the font size in pt (instead of unsuffixed which will be pixels) and now it renders differently in Firefox and Emacs:

Firefox: screenshot-2021-05-03_12-54-14

Emacs:
screenshot-2021-05-03_12-54-33

svg:

<svg width="60" height="23" version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
  <rect width="50" height="21" x="5.0" y="0" rx="2" fill="#FFAB91"></rect>
  <rect width="49.0" height="20.0" x="5.5" y="0.5" rx="1.5" fill="#FFAB91"></rect>
  <text y="10.053691275167786pt" x="10.0pt" fill="#ffffff" font-size="10.053691275167786pt" font-weight="nil" font-family="PragmataPro Liga">
    TODO
  </text>
</svg>

@rougier
Copy link
Owner

rougier commented May 3, 2021

librsvg seems to have suffered from dpi problems (https://gitlab.gnome.org/GNOME/librsvg/-/issues/427) but I don't know if this is related.

@jamesnvc
Copy link

jamesnvc commented May 3, 2021

I do see calls to rsvg_handle_set_dpi_x_y which I assume are to address DPI issues, but maybe the text drawing does something different with those...

@jamesnvc
Copy link

jamesnvc commented May 6, 2021

I see there was an issue in Emacs' svg rendering on high-dpi screens, but it was apparently resolved: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=45124

@jamesnvc
Copy link

jamesnvc commented May 6, 2021

I have something working by taking a somewhat different approach -- calculating the font size in ems instead of pixels. Setting the text-y still needs some fiddling though...does this work for other folks?

diff --git a/svg-tag-mode.el b/svg-tag-mode.el
index 49cca62..a673aa6 100644
--- a/svg-tag-mode.el
+++ b/svg-tag-mode.el
@@ -141,14 +141,13 @@ (defun svg-tag--make (text face padding margin radius)
          (text-x      (+ tag-x
                          (/ (- tag-width (* (length text) txt-width))
                             2)))
-         (font-size   (* (ceiling
-                          (* (face-attribute face :height nil 'default)
-                             0.1))
-                         (image-compute-scaling-factor 'auto)))
-         (txt-height  (window-font-height))
+         (font-size   (format "%sem"
+                              (/ (face-attribute face :height nil 'default)
+                                 (face-attribute 'default :height nil))))
+         (txt-height  (window-font-height nil face))
          (svg-height  txt-height)
          (tag-height  (- txt-height 2))
-         (text-y      font-size)
+         (text-y      (- txt-height 5))
          (svg         (svg-create svg-width svg-height)))
     (svg-rectangle svg tag-x 0 tag-width tag-height
                    :fill (if box box-color background)

EDIT fixing patch

@tarsius
Copy link
Contributor Author

tarsius commented May 11, 2021

Doesn't work for me:

20210511-09:08:46

@deb75
Copy link

deb75 commented Jun 21, 2021

Hi,

Thanks for pointing out this.

It have tested most of the examples in the demo. It works nice on emacs / windows 10.

However, I still have to manually increase the font-size field up to 40 and also, to tweak padding, stroke fields
in order to get a similar visual appealing aspect. I guess the scaling on hidpi screens, either windows or linux, still messes up somewhere.

That said, it is not a too annoying issue because the svg-lib functions provide all the necessary parameters to adapt to the screen, as far as I have tested.

It would be nice if you could provide a more detailed documentation or example of all the functions parameters and what they do. Also, in the last exemple (svg-lib-icon), is it possible to provide the (file) path of icon ? or to provide a directory search path where to find icons ?

Thanks very much for providing this library.

Regards

@rougier
Copy link
Owner

rougier commented Jun 21, 2021

@deb75 Thansk for the report and ideas. I should have included a local repo for icons and have options to adjust font size. Can you open issues on the svg lib repo? In the end, I would like to understand the HiDPI stuff such that no tweaking would be necessary but I ran out of ideas. I need to find a windows machine to test.

@pstray
Copy link

pstray commented Aug 14, 2021

Using (font-dpi 96) in the code above seems to work fine here... the image and the default text seems identical.
I am using "DejaVuSansMono Nerd Font Mono-8" on a 1920x1080, 280mm x 160mm, so about 173.5 ppi.

@rougier
Copy link
Owner

rougier commented Aug 15, 2021

Thanks for the report. What is the code you're referring exactly (there's too much code in this issue). Also, how did you find the dpi that works ? Is i reported by Emacs somewhere?

@pstray
Copy link

pstray commented Aug 16, 2021

That would be your code from #14 (comment)

I tried 96 (as that is the default dpi set by most stuff nowadays it seems), after both higher and lower values, but 96 did get me the exact same width in the image inserted as in the same text on the next line in my emacs buffer.

It also works with 96 in my other laptop while docked on a 2560x1440 (597mm x 336mm) screen, so 108.9 ppi there...

The values emacs reports is (display-pixel-height) => 1440 and (display-mm-height) => 220 (the laptop-screen itself) which gives 166 ppi, which is also the value i have set for the Xresource Xft.dpi and given to xrandr --dpi.

Btw, 72.27 seems to only be used in typesetting, I have never encountered it on a computer (exept maybe in some TeX code)... 72 on the other hand, is pretty common, as used in PostScript and PDF amongst other things.

On the other hand, if i just use the following code, it works just fine (note that i use the PIXEL-SIZE element of the font-query)

(let* ((text "Hello guys!")
       (font       (query-font (font-at (point-min))))
       (font-size  (elt font 2))
       (family     (face-attribute 'default :family))
       (descent    (elt font 4))
       (ascent     (elt font 5))
       (svg-height (+ ascent descent))
       (char-width (elt font 7))
       (svg-width  (* char-width (length text)))
       (svg        (svg-create svg-width svg-height)))
  (svg-text svg text
            :fill "white"
            :stroke-width 0
            :font-family family
            :font-weight "300"
            :font-size font-size
            :x 0
            :y descent)
  (insert-image (svg-image svg :ascent 'center)))

@rougier
Copy link
Owner

rougier commented Aug 30, 2021

Thansk for the report and sorry for the delay. The fact that the code you provide doesn't need dpi is nice and may solve a lot of hassle. I think it corresponds more or less to what I did for svg-lib. In the end, I wonder where the dpi problem comes from when it occurs.

@aruZeta
Copy link

aruZeta commented Dec 12, 2021

Hey all, after been testing a bit, I decided to remake svg-tag--make calculations, and seems like I hit the jackpot and it works perfectly with the aligning (tho not horizontally since to do that you would have to remove the padding option and make the text align at the center of the tag, so maybe you could still have an option for min-padding, this is something I'm planning to make).

What I dont know is if it will work for everyone, I tried with some fonts and worked for me, here is the code for the function:

(defun svg-tag--make (text face padding margin radius)
  (let* ((foreground   (face-attribute face :foreground nil t))
         (background   (face-attribute face :background nil t))
         (box          (face-attribute face :box nil t))
         (box          (if (eq box 'unspecified) nil box))
         (border-color (or (plist-get box :color) foreground))
         (border-width (or (plist-get box :line-width) 0))
         (font-family  (face-attribute face :family nil 'default))
         (font-weight  (alist-get (face-attribute face :weight nil 'default)
				  svg-tag--font-weights))
	 (char-width   (window-font-width))
	 (char-height  (window-font-height))
	 (text-width   (* char-width (length text)))
	 (tag-width    (+ text-width (* padding 2)))
	 (box-width    (+ tag-width (* border-width 2)))
	 (box-height   (- char-height 2))
	 (tag-height   (- box-height (* border-width 2)))
	 (tag-x        (+ margin border-width))
	 (tag-y        (+ border-width 1))
	 (svg-width    (+ box-width (* margin 2)))
         (text-size    (* (ceiling
			   (* (face-attribute face :height nil 'default)
			      0.1))
			  (image-compute-scaling-factor 'auto)))
	 (text-x       (+ tag-x padding))
	 (text-y       (+ text-size (/ (- box-height (* (/ text-size 3.0) 4)) 2) border-width))
         (svg         (svg-create svg-width char-height)))
    (svg-rectangle svg margin 1 box-width box-height
                   :fill (if box border-color background)
                   :rx   radius)
    (when box
      (svg-rectangle svg
		     tag-x tag-y tag-width tag-height
                     :fill background
                     :rx   radius))
    (svg-text svg text
              :font-family font-family
              :font-weight font-weight
              :font-size   text-size
              :fill        foreground
              :x           text-x
              :y           text-y)
    (svg-image svg :scale 1 :ascent 'center)))

And here is a ss of how it looks
Sun-12-Dec-2021_12-09-00

And here with a larger font (from 120 height before to 180 here):
Sun-12-Dec-2021_12-18-36
It seems like the text is a bit bigger, but feels a bit small, tho it still aligns vertically at the center.

Hope this helps

Edit: just realized the width is a bit off

@aruZeta
Copy link

aruZeta commented Dec 12, 2021

Here is how to make the text stay at the center of the box (horizontally), we could make it by default like this or with a bool argument.

This is the only thing to change to make it work:

(text-x (+ tag-x
           padding
           (/ (- text-width
                 (* (/ (float char-width)
                        char-height)
                    (* (/ text-size
                          3.0)
                       4)
                    (length text)))
              2)))

And here a ss:
Sun-12-Dec-2021_12-59-34
It also works with bigger font height

Im now working in fixing the width, so it stays the same as the normal text

@aruZeta
Copy link

aruZeta commented Dec 12, 2021

Here is the fix for the width:

(total-width  (+ text-width (* padding 2) (* margin 2) (* border-width 2)))
(svg-width    (if (= (mod total-width
                          char-width)
                     0)
                  total-width
                (* (+ 1
                      (truncate (/ total-width
                                   char-width)))
                   char-width)))
(box-width    (- svg-width (* margin 2)))
(tag-width    (- box-width (* border-width 2)))

And if you want centered text with the above fix:

(text-x       (+ tag-x
                 padding
                 (/ (- tag-width
                       (* padding 2)
                       (* (/ (float char-width)
                             char-height)
                          (* (/ text-size
                                3.0)
                             4)
                          (length text)))
                    2)))

In the centering of the text, the padding could be better called as a min-padding, since the text will separate from it so it is centered.

Here is a ss:
Sun-12-Dec-2021_14-04-55

Should I make a pr to add the code or do you add it yourself @tarsius ? Since this pr is related to calculations too and I used your code as base.

@tarsius
Copy link
Contributor Author

tarsius commented Dec 12, 2021

@aru-hackZ, open a new pr.

Also, should this pr be closed, @rougier? I don't plan to work on it anymore. It seems things have moved beyond what we have here. Code-wise that is, it still seems like a place to have a conversation, so maybe it should stay open.

@aruZeta
Copy link

aruZeta commented Dec 12, 2021

@aru-hackZ, open a new pr.

Then I would like if this pr was merged, since the code I worked on was with yours as its base.

@tarsius
Copy link
Contributor Author

tarsius commented Dec 12, 2021

Just include my commit in your pr. 😀

(But of course one could first merge this too. In any case you don't have to wait for this to be merged before opening your pr.)

@aruZeta
Copy link

aruZeta commented Dec 12, 2021

I think I will wait for the pr to be merged first.

@rougier
Copy link
Owner

rougier commented Dec 15, 2021

Thank for the code and I'll need to test it a bit further. As or merging, it might be better to merge it in svg-lib which is now part of ELPA (and also have a svg-lib-tag which share most of the code). Could you post below you full code (with example) or a link to a git?

@aruZeta
Copy link

aruZeta commented Dec 15, 2021

Hey @rougier the new calculations I made, as I said, were based on the code tarsius has on his fork, so you would need to first merge his PR, the final code I had (which also aligns the text in the center of the tag) is this:

(defun svg-tag--make (text face padding margin radius)
  (let* ((foreground   (face-attribute face :foreground nil t))
         (background   (face-attribute face :background nil t))
         (box          (face-attribute face :box nil t))
         (box          (if (eq box 'unspecified) nil box))
         (border-color (or (plist-get box :color) foreground))
         (border-width (or (plist-get box :line-width) 0))
         (font-family  (face-attribute face :family nil 'default))
         (font-weight  (alist-get (face-attribute face :weight nil 'default)
                                  svg-tag--font-weights))
         (char-width   (window-font-width))
         (char-height  (window-font-height))
         (text-width   (* char-width (length text)))
         (box-height   (- char-height 2))
         (tag-height   (- box-height (* border-width 2)))
         (tag-x        (+ margin border-width))
         (tag-y        (+ border-width 1))
         (total-width  (+ text-width (* padding 2) (* margin 2) (* border-width 2)))
         (svg-width    (if (= (mod total-width
                                   char-width)
                              0)
                           total-width
                         (* (+ 1
                               (truncate (/ total-width
                                            char-width)))
                            char-width)))
         (box-width    (- svg-width (* margin 2)))
         (tag-width    (- box-width (* border-width 2)))
         (text-size    (* (ceiling
                           (* (face-attribute face :height nil 'default)
                              0.1))
                          (image-compute-scaling-factor 'auto)))
         (text-x       (+ tag-x
                          padding
                          (/ (- tag-width
                                (* padding 2)
                                (* (/ (float char-width)
                                      char-height)
                                   (* (/ text-size
                                         3.0)
                                      4)
                                   (length text)))
                             2)))
         (text-y       (+ text-size (/ (- box-height
                                          (* (/ text-size 3.0)
                                             4))
                                       2)
                          border-width))
         (svg         (svg-create svg-width char-height)))
    (svg-rectangle svg margin 1 box-width box-height
                   :fill (if box border-color background)
                   :rx   radius)
    (when box
      (svg-rectangle svg
                     tag-x tag-y tag-width tag-height
                     :fill background
                     :rx   radius))
    (svg-text svg text
              :font-family font-family
              :font-weight font-weight
              :font-size   text-size
              :fill        foreground
              :x           text-x
              :y           text-y)
    (svg-image svg :scale 1 :ascent 'center)))

@rougier
Copy link
Owner

rougier commented Dec 16, 2021

Thansk. It still doesn't look right on my machine:

Screenshot 2021-12-16 at 14 31 09

@aruZeta
Copy link

aruZeta commented Dec 16, 2021

The tag is the 'hello' that seems like it is bold? Then I would like to know your default face properties since maybe there is something im not taking in account when calculating the tag text size.

Also since i have a 1920x1080 screen i cant test if it works in bigger screens or with HiDPI so I need more input in thoae areas.

@rougier
Copy link
Owner

rougier commented Dec 16, 2021

I'm using "Roboto Mono" with light weight. Also, note that the tag is also a bit off (baseline doesn't match regular text). You can check svg-lib.el to see how I compute the ascent.

@aruZeta
Copy link

aruZeta commented Dec 16, 2021 via email

@gaetgu gaetgu mentioned this pull request Dec 12, 2022
@tarsius tarsius closed this Mar 15, 2023
tarsius added a commit to magit/forge that referenced this pull request Apr 24, 2023
For now at least I have decided against this.
Still, it might be interesting for future reference.
Also see rougier/svg-tag-mode#14.
tarsius added a commit to magit/forge that referenced this pull request Apr 24, 2023
For now at least I have decided against this.
Still, it might be interesting for future reference.
Also see rougier/svg-tag-mode#14.
tarsius added a commit to magit/forge that referenced this pull request Sep 23, 2023
For now at least I have decided against this.
Still, it might be interesting for future reference.
Also see rougier/svg-tag-mode#14.
tarsius added a commit to magit/forge that referenced this pull request Oct 4, 2023
For now at least I have decided against this.
Still, it might be interesting for future reference.
Also see rougier/svg-tag-mode#14.
tarsius added a commit to magit/forge that referenced this pull request Oct 4, 2023
For now at least I have decided against this.
Still, it might be interesting for future reference.
Also see rougier/svg-tag-mode#14.
tarsius added a commit to magit/forge that referenced this pull request Oct 6, 2023
For now at least I have decided against this.
Still, it might be interesting for future reference.
Also see rougier/svg-tag-mode#14.
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.

Wrong sizes on high resolution screens
6 participants