emacs-sideline / sideline

Show information on the side
GNU General Public License v3.0
118 stars 14 forks source link

Fix right align with variable width fonts #21

Closed chuxubank closed 3 months ago

chuxubank commented 4 months ago

When fixing #20 , we use current window's font width to calculate the string width, but for variable width fonts, which usually used in variable-pitch-mode, this will cause error.

Whereas emacs's built-in string-pixel-width do not respect the face remapping, which cause wrong string width when use buffer-face-mode.

This pr mixed the @gustavotcabral 's patch and emac's built-in string-pixel-width to directly get the correct string width.

This fix is not that heavy because we are doing almost the same thing as emac's built-in string-pixel-width.

Some screenshots after fix:

image image
jcs090218 commented 4 months ago

The only issue with this patch is the space calculation needs to be corrected. Here are the conditions we want to consider:

  1. Make sure it works with text-scale-mode
  2. Make sure it works with buffer-face-mode
  3. line space calculation is correct (space enough to display sideline information)

🤔

chuxubank commented 4 months ago

For 1 and 2, I tested and just works well. BTW, variable-pitch-mode is kind of buffer-face-mode. For 3, it does have some problem. When text-scale-mode is increasing, the space calculating seemed not doing the right thing, and may cause overflow when align left.

image

I will try to fix it.

chuxubank commented 4 months ago

It seemed that only if I change the sideline--str-len to this:

(defun sideline--str-len (str)
  "Calculate STR length."
  (length str))

Then all things works for me?

image image

Sorry, I forget to test align left.

jcs090218 commented 4 months ago

It seemed that only if I change the sideline--str-len to this:

I think you got it! TBH, I don't remember how I created this since it's been a while. 😓 Can you apply the changes to this PR? Thanks!

jcs090218 commented 4 months ago

Sorry, I forgot to test align left.

Yeah, this project got a little complicated. I seldom forget things I need to test. Please make sure you have sidelines on the same line (one on each side), so they don't conflict with each other. 👍

chuxubank commented 3 months ago

I finally fixed the space calculation by using (window-font-width) instead of (frame-char-width) in sideline--str-len Test Result:

image image

@jcs090218 Could can have a check with my new commit?

chuxubank commented 3 months ago

Sorry I find out that the left align fix is done by your https://github.com/emacs-sideline/sideline/commit/c1729b2b9d2ca6b37bf605ca2271e570f30316f0 Simply replace the sideline--str-len with length is enough. Test Result:

image image

Works better than (window-font-width)solution.

@jcs090218 Should I delete the sideline--str-len function and replace it with length in the code or simply keep it as is?

jcs090218 commented 3 months ago

Does the latest commit works for you? See a4626181434534385fbb199fa17d606f89e86e7c. 🤔

chuxubank commented 3 months ago

Does the latest commit works for you? See a462618. 🤔

Yes, as long as we respect the face-remapping-alist then it should works!

image

Thanks for the quick fix, I will close this PR.