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

magit-blame headers inherit stipple face with starting-column = 0. #67

Closed
aburlot opened this issue Sep 27, 2024 · 16 comments
Closed

magit-blame headers inherit stipple face with starting-column = 0. #67

aburlot opened this issue Sep 27, 2024 · 16 comments

Comments

@aburlot
Copy link

aburlot commented Sep 27, 2024

When I turn magit-blame on, I have some issue reading it:
indent-magit-blame
Looks like some bars are introduced on every character.

@jdtsmith
Copy link
Owner

Hmm, I don't see this:
image

@jdtsmith
Copy link
Owner

Can you see if M-x indent-bars-reset helps?

@jdtsmith
Copy link
Owner

Otherwise, go to a line just after one of the headers, and M-: (overlay-get (car (last (overlays-at (point)))) 'before-string) and report what you get.

@aburlot
Copy link
Author

aburlot commented Sep 28, 2024

Unfortunately, indent-bars-reset changes nothing. The command gives me this :

#("My name          2024-09-26 15:33 commentary and cleaning
" 0 20 (font-lock-face (magit-blame-name magit-blame-heading)) 20 21 (font-lock-face magit-blame-heading) 21 37 (font-lock-face (magit-blame-date magit-blame-heading)) 37 38 (font-lock-face magit-blame-heading) 38 61 (font-lock-face (magit-blame-summary magit-blame-heading)) 61 62 (font-lock-face magit-blame-heading))

Strangely, in Python, I have a mixed effect:
indent-bars

In C++, I notice that it starts to be messy when I am in a function, thus when the indent-bars start appearing:
indent-bars-2
But a few commits appear normally even inside the function. I tried the command on several lines, but they always show the same resultats as above.

I hope this helps!

@jdtsmith
Copy link
Owner

Do you have indent-bars-starting-column = 0. Always important to mention any non-default settings.

@shipmints
Copy link

It's nil. My config is super basic:

  (setopt indent-bars-prefer-character t)
  (setopt indent-bars-no-stipple-char ?│)
  (setopt indent-bars-depth-update-delay 0.3) ; default 0.075
  (setopt indent-bars-color-by-depth nil)
  (setopt indent-bars-color '(highlight :face-bg t :blend 0.325)) ; default '(highlight :face-bg t :blend 0.325)
  (setopt indent-bars-treesit-support t)
  (require 'indent-bars-ts)
  (setopt indent-bars-treesit-update-delay 0.3) ; default 0.125
  (setopt indent-bars-treesit-ignore-blank-lines-types '("module"))

@jdtsmith
Copy link
Owner

jdtsmith commented Sep 28, 2024

I meant this for @aburlot. I suspect they do. If so, this looks to me like a bug in magit-blame. It sets the font-lock-face on an overlay :before-string property, which is fine. But it should also set face=nil on that string. The reason why is that if there is any face on the first character in a line, the before-string overlay also inherits that face. Usually there is either no or simple foreground face on the first column, so the problem isn't noticed. With stipples in column 0, it's easy to notice.

Please open an issue with magit and mention the need for a face=nil property on magit-blame's before-string.

@jdtsmith jdtsmith changed the title magit-blame is unreadable magit-blame inherits stipple face Sep 28, 2024
@aburlot
Copy link
Author

aburlot commented Sep 29, 2024

Do you have indent-bars-starting-column = 0. Always important to mention any non-default settings.

Yes! When set back to nil, magit-blame is normal. Do you confirm that I should open an issue with magit?

@jdtsmith
Copy link
Owner

jdtsmith commented Sep 29, 2024

Yes, please do and link here. Magit should not inherit the face property of the first character on a line into its blame overlay :before-string, since it doesn't control what that is or will be (and face overrides font-lock-face).

@jdtsmith jdtsmith changed the title magit-blame inherits stipple face magit-blame headers inherit stipple face with starting-column = 0. Sep 29, 2024
@tarsius
Copy link

tarsius commented Sep 30, 2024

Magit should not inherit the face property of the first character on a line into its blame overlay :before-string

I cannot spot how Magit does that. It appears you do. If so, please tell me.

I did this

(defun magit-blame--update-heading-overlay (ov)
  (overlay-put
   ov 'before-string (propertize "fake heading\n" 'face nil)))

and the lines where still there.

@jdtsmith
Copy link
Owner

Oh right, a nil-face does not reset face attributes, but is instead ignored. Even switching magit's before-string property from font-lock-face to face doesn't work as you'd hope, since the overlay still inherits any unspecified attributes from the underlying text:

If the text comes from an overlay string via ‘before-string’ or
‘after-string’ properties (*note Overlay Properties::), or from a
display string (*note Other Display Specs::), and the string
doesn't contain a ‘face’ or ‘mouse-face’ property, or these
properties leave some face attributes undefined, but the buffer
text affected by the overlay/display property does define a face or
those attributes, Emacs applies the face attributes of the
"underlying" buffer text. Note that this is so even if the overlay
or display string is displayed in the display margins (*note
Display Margins::).

The best approach might be just to add default at the end of the list, so as to explicitly reset all other unmentioned attributes:

(let ((ov (make-overlay (point) (point)))
      (d (propertize "fake heading\n"
		     'font-lock-face
		     '((:foreground "red" :background "black") default))))
  (overlay-put ov 'before-string d))

This would also reset other attributes like :height that might vary in the buffer as well. Note that, oddly, font-lock-face does in fact override face specifically in this "overlay before-string inheriting underlying face" context.

@jdtsmith
Copy link
Owner

jdtsmith commented Oct 1, 2024

Fixed in magit.

@jdtsmith jdtsmith closed this as completed Oct 1, 2024
@tarsius
Copy link

tarsius commented Oct 1, 2024

Thanks for your help!

@jdtsmith
Copy link
Owner

jdtsmith commented Oct 1, 2024

Thanks for the fix (and magit, used daily)!

@tarsius
Copy link

tarsius commented Oct 1, 2024

I hesitated to end the list of faces with default, but in the end I concluded that was indeed the cleanest approach, not least because that way not every theme has to be updated individually.

@jdtsmith
Copy link
Owner

jdtsmith commented Oct 1, 2024

I agree, it seems like there should be a better way to "negatively override" face, but IME the attribute selection system is purely additive. There is a special reset attribute property, but then you have to enumerate all of them (including future attributes that don't exist yet!).

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

No branches or pull requests

4 participants