purcell / default-text-scale

Easily adjust the font size in all Emacs frames
79 stars 10 forks source link

Resetting may set size to a wrong value #5

Open maurolopes opened 5 years ago

maurolopes commented 5 years ago

I noticed that resetting the size after multiple sequential increases may make the new size wrong (that is, different from the original one). I have reproduced a minimal example to illustrate (in my case the initial size is 11 and the size after reset is 10):

Copy this into the *scratch* buffer and evaluate:

(default-text-scale-mode)
(message "Initial size: %d" (face-attribute 'default :height))

(dotimes (i 3)
  (default-text-scale-increase)
  (message "%d" (face-attribute 'default :height)))
(default-text-scale-reset)
(message "After reset: %d" (face-attribute 'default :height))

Resulting info in the *Messages* buffer:

Initial size: 113
Default font size is now 12
120
Default font size is now 13
128
Default font size is now 13
138
Default font size is now 10
After reset: 108

I guess it's because the new height is calculated by taking the current height, multiplying, rounding, then setting, while default-text-scale--complement is calculated using the actual delta, with no rounding.

Any ideas?

purcell commented 5 years ago

It's strange that your font heights are not multiples of 10. I don't think Emacs handles that well, and I suspect that's where the rounding is coming from. I don't think there's any rounding of the deltas going on, other than when the "Default font size is now X" is being printed.

maurolopes commented 5 years ago

I tested now with emacs --no-init-file, my font height is still 113. Are you sure this is non-standard somehow? I couldn't find any documentation saying that.

purcell commented 5 years ago

Interesting! What platform are you on? If I run the following (similar) snippet on my MacOS machine:

(set-face-attribute 'default nil :height 113)
(default-text-scale-mode)
(message "Initial size: %d" (face-attribute 'default :height))
(dotimes (i 3)
  (default-text-scale-increase)
  (message "%d" (face-attribute 'default :height)))
(message "Complement: %d" default-text-scale--complement)
(default-text-scale-reset)
(message "After reset: %d" (face-attribute 'default :height))

then I get the following output:

Initial size: 113
Default font size is now 12
123
Default font size is now 13
133
Default font size is now 14
140
Complement: -30
Default font size is now 11
After reset: 110
purcell commented 5 years ago

In all those cases, it looks like the underlying Emacs platform implementation or window toolkit is stepping in to round the size to a supported size: note the jumps from 113->123->133, then to 140. The solution might be to calculate the complement by adding up the actual differences in font sizes.

purcell commented 5 years ago

I think efb5ce1 should fix this. When MELPA has built a new package in a few hours, perhaps you would be kind enough to install the new package and re-test it? It certainly works locally for me.

maurolopes commented 5 years ago

I'm using Arch Linux.

Thanks for the fix! I will test it soon. I still have a couple of questions about the solution though:

1) Why do you set default-text-scale--complement to 0 in default-text-scale-decrease and not in default-text-scale-reset?

2) Why does default-text-scale--complement even need to exist (and be updated with every change)? I thought it could simply store the initial size when the minor mode is enabled and the reset function would set the size back to that value, no?

3) Ah, maybe you store the delta because the initial sizes could be different in different frames, so you can undo the change in each frame without setting them all to the same number. But then this new solution won't work, since the actual height differences won't be the same, as the increment depends on the current height (in my case it increments by 8 when at 120, by 10 when at 128, etc). It would need to store one different complement (or initial value) for each frame.

purcell commented 5 years ago
  1. Why do you set default-text-scale--complement to 0 in default-text-scale-decrease and not in default-text-scale-reset?

LOL, because I'm stupid, or under-caffeinated (or both!). Fixed in 5fb2640. Thanks!

  1. Why does default-text-scale--complement even need to exist (and be updated with every change)? I thought it could simply store the initial size when the minor mode is enabled and the reset function would set the size back to that value, no?

For a long time I resisted even having this functionality because there is actually no 100% reliable way to reset the "normal" size, because "normal" is not clearly defined. For example, what if someone increases the font size with default-text-scale-increase, and then saves that to their custom file? At least setting the --complement var back to zero when resetting avoids that particular issue.

Ah, maybe you store the delta because the initial sizes could be different in different frames

No, I think that if the initial sizes were different in different frames then that would cause problems. It should also be a very unusual case.

maurolopes commented 5 years ago

Ah, it all makes sense now. Thanks for the explanation and for the fix!

maurolopes commented 5 years ago

I ran again my initial snippet and the fix didn't work :(

Initial size: 113
Default font size is now 123
120
Default font size is now 130
128
Default font size is now 138
138
Default font size is now 108
After reset: 108

If I keep running it over and over again, the font size keeps decreasing until it reaches 83, where it becomes stable (resets back to 83 after each run).

purcell commented 5 years ago

That's weird. :-/ What's the output if you try the following?

(set-face-attribute 'default nil :height 113)
(default-text-scale-mode)
(message "Initial size: %d" (face-attribute 'default :height))
(dotimes (i 3)
  (default-text-scale-increase)
  (message "Size: %d, complement: %d" (face-attribute 'default :height) default-text-scale--complement))
(default-text-scale-reset)
(message "After reset: %d" (face-attribute 'default :height))
maurolopes commented 5 years ago
Initial size: 113
Default font size is now 123
Size: 120, complement: -10
Default font size is now 130
Size: 128, complement: -20
Default font size is now 138
Size: 138, complement: -30
Default font size is now 108
After reset: 108
purcell commented 5 years ago

Right, I get this:

Initial size: 113
Default font size is now 123
Size: 120, complement: -10
Default font size is now 130
Size: 130, complement: -20
Default font size is now 140
Size: 140, complement: -30
Default font size is now 110
After reset: 110

I'll have to dig into this a bit more... :-)

purcell commented 5 years ago

Hahaha, what I discovered is amazing! When the code tries to request the new font size (to calculate the actual delta), Emacs lies to it: it seems the new size is not correctly reported until a redraw has been forced. Even a message all is enough to force that. So all this is due to an Emacs bug or - more charitably perhaps - quirk.

maurolopes commented 5 years ago

Oh, very interesting. Great find! Now reset works perfectly. See my output:

Initial size: 113
Stale font size: 123
Default font size is now 120
120
Stale font size: 130
Default font size is now 128
128
Stale font size: 138
Default font size is now 138
138
Stale font size: 113
Default font size is now 113
After reset: 113

On the other hand, I'd expect one decrease to reverse one increase, but it doesn't happen:

(default-text-scale-mode)
(message "Initial size: %d" (face-attribute 'default :height))
(default-text-scale-increase)
(message "%d" (face-attribute 'default :height))
(default-text-scale-decrease)
(message "%d" (face-attribute 'default :height))
Initial size: 113
Stale font size: 123
Default font size is now 120
120
Stale font size: 110
Default font size is now 110
110

Is that ok?

purcell commented 5 years ago

On the other hand, I'd expect one decrease to reverse one increase, but it doesn't happen:

This might just be the hardest possible Emacs package to get right.

purcell commented 5 years ago

I guess that this behaviour hasn't really changed, right? Nothing in the logic related to increasing and decreasing the font size has been modified.

zenspider commented 5 years ago

This might be related, I'm not sure... but when I use these commands to change the font size, (frame-width) lies until (I think) I do something to force a full redraw. This is making it hard to calculate the number of windows I want (basically (/ (frame-width) 80)) vs font size. If this is NOT related to the emacs size lie mentioned above, let me know and I'll file a separate issue.

jacksonludwig commented 3 years ago

I'm currently getting this issue on emacs 27.1. Any workaround or anything for it?

kenranunderscore commented 3 years ago

This might just be the hardest possible Emacs package to get right.

That might just be correct :) I've just discovered this package and ran into this issue as well (though it's ofc way better with the package and this slight problem than having to adjust every buffer manually!).

For now I've defined my own default font height and I'm resetting (inside a hydra) with the following snippet:

(lambda ()
  (interactive)
  (setq default-text-scale--complement 0)
  (set-face-attribute 'default nil :height my/default-font-height))
bkhl commented 2 years ago

I also worked this around similarly to @kenranunderscore . Here's what I do currently, in case it is of use for anyone. Since as you see I do set the default height in my basic configuration anyway, and don't change it around in any other ways, this seems to work alright for me.

(defvar my/default-text-height 130)

(set-face-attribute 'default nil
                    :family "Iosevka"
                    :height my/default-text-height)
(set-face-attribute 'variable-pitch nil
                    :family "Crimson Pro"
                    :height (/ 1.4 1.3))
(set-face-attribute 'fixed-pitch nil
                    :family "Iosevka"
                    :height (/ 1.3 1.4))

(use-package default-text-scale
  :bind
  (("C-)" . (lambda ()
              (interactive)
              (setq default-text-scale--complement 0)
              (set-face-attribute 'default
                                  nil
                                  :height bkhl/default-text-height)
              (message "Default font size is now %d"
                       (face-attribute 'default :height))))
   ("C-+" . default-text-scale-increase)
   ("C-_" . default-text-scale-decrease)))