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 right align with variable width fonts #21

Closed
wants to merge 2 commits into from
Closed

Fix right align with variable width fonts #21

wants to merge 2 commits into from

Conversation

chuxubank
Copy link
Contributor

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

sideline.el Show resolved Hide resolved
@jcs090218
Copy link
Member

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
Copy link
Contributor Author

chuxubank commented Apr 4, 2024

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
Copy link
Contributor Author

chuxubank commented Apr 4, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

chuxubank commented Apr 27, 2024

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
Copy link
Contributor Author

chuxubank commented Apr 27, 2024

Sorry I find out that the left align fix is done by your c1729b2
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?

@chuxubank chuxubank requested a review from jcs090218 May 10, 2024 00:11
@jcs090218
Copy link
Member

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

@chuxubank
Copy link
Contributor Author

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.

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.

2 participants