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

how is the light/dark detection supposed to work? #273

Closed
fommil opened this issue Oct 16, 2017 · 13 comments
Closed

how is the light/dark detection supposed to work? #273

fommil opened this issue Oct 16, 2017 · 13 comments
Labels

Comments

@fommil
Copy link

fommil commented Oct 16, 2017

Hi, I'm the author of these two themes

I can't work out how your light / dark detection is supposed to work, but I do know that it never works for me...

(defface markdown-code-face
  (let* ((default-bg (or (face-background 'default) "unspecified-bg"))
         (light-bg (if (equal default-bg "unspecified-bg")
                       "unspecified-bg"
                     (color-darken-name default-bg 3)))
         (dark-bg (if (equal default-bg "unspecified-bg")
                       "unspecified-bg"
                     (color-lighten-name default-bg 3))))
    `((default :inherit fixed-pitch)
      (((type graphic) (class color) (background dark)) (:background ,dark-bg))
      (((type graphic) (class color) (background light)) (:background ,light-bg))))
  "Face for inline code, pre blocks, and fenced code blocks."
  :group 'markdown-faces)

e.g. I still get this in the dark theme

Defined in ‘markdown-mode.el’.

           Family: unspecified
          Foundry: unspecified
            Width: unspecified
           Height: unspecified
           Weight: unspecified
            Slant: unspecified
       Foreground: unspecified
DistantForeground: unspecified
       Background: #EEEEEE
        Underline: unspecified
         Overline: unspecified
   Strike-through: unspecified
              Box: unspecified
          Inverse: unspecified
          Stipple: unspecified
             Font: unspecified
          Fontset: unspecified
          Inherit: (quote org-code)

note that the background is white here, which is not at all what I want. Rather than overriding this manually, is it possible that either

  • my themes can be made compatible with your approach
  • your detection can be ignored entirely (i.e. no explicit colour settings)

I'd prefer the latter.

@jrblevin
Copy link
Owner

What I'm trying to accomplish is as follows: If the user is using a theme with markdown-code-face defined, then do nothing. The defface shouldn't override it. Otherwise, if the background is light, use a slightly darkened background for code. Else if the background is dark, use a slightly lightened background for code.

In an ideal world, this would update automatically when the user switches themes (unless the theme defines the face), but that was problematic (see #241).

I'm can't tell for sure, but here is my guess at what's happening: since markdown-code-face was defined in your light theme, but not in the dark theme (until the recent "workaround" commit), when you load the dark theme the old face sticks around (which had background #EEEEEE). Are you disabling the light theme first (M-x disable-theme)?

I'm stumped on the best way to handle this. I couldn't find a built-in face that was suitable, but if themes don't define it (and I figured most wouldn't, at least not at first) I wanted a reasonable default.

Nice themes, by the way. I will try them out!

@ivan-m
Copy link

ivan-m commented Oct 25, 2017

I'm using alect-themes (specifically the alect-dark theme), and the same thing occurs: I get a blinding white background in code blocks.

There doesn't seem to be anything in the theme setting markdown-code-face (though it does set markdown-pre-face).

ivan-m added a commit to ivan-m/.emacs.d that referenced this issue Oct 25, 2017
@daveliepmann
Copy link

I'm getting the same blinding white background in inlined code after updating from markdown-mode-20170706.1309 to markdown-mode-20171031.1725. I fixed it using @ivan-m's (set-face-attribute 'markdown-code-face nil :background nil) solution.

Stylistically I'm not convinced that markdown-mode should be touching colors at all in this situation; enforcing monospace seems perfectly sufficient. Maybe if the color wasn't so intrusive (e.g the massive contrast of a white background in a dark theme) I'd feel differently.

@jrblevin
Copy link
Owner

jrblevin commented Nov 3, 2017

To be clear, the intention is not white background for code in a dark theme—I agree that’s awful. The issue seems to be when switching from a light to dark theme when the dark theme doesn’t define the face (or vice versa).

@daveliepmann
Copy link

daveliepmann commented Nov 3, 2017

I experienced this issue without theme switching, merely by having a dark theme without this new face defined.

@jrblevin
Copy link
Owner

jrblevin commented Nov 3, 2017

That's unexpected. Which theme may I ask?

@daveliepmann
Copy link

Theme is eigengrau

@fommil
Copy link
Author

fommil commented Nov 4, 2017

given the trouble that this is causing, could you perhaps turn off the colour detection and just rely on the face being provided by the theme? If specific themes are broken then maybe they could just be updated to support markdown mode?

@jrblevin
Copy link
Owner

jrblevin commented Nov 6, 2017

Thanks, @daveliepmann. I can't reproduce the issue with eigengrau. For me, I get a subtle change in the background color for code blocks as expected (see the screenshot).

emacs -Q -l eigengrau-definitions.el -l eigengrau-theme.el -l markdown-mode.el

screen shot 2017-11-05 at 9 04 02 pm

I've only seen the issue when first using a light theme, at which point markdown-code-face gets defined as a light color, and then switching to a dark theme which doesn't itself define markdown-code-face (or the opposite case).

@ivan-m
Copy link

ivan-m commented Nov 6, 2017

The issue seems to be when switching from a light to dark theme when the dark theme doesn’t define the face (or vice versa).

Does this mean that if I call (package-initialize) and set up auto-loading of packages (including markdown-mode) before falling load-theme that this might be the issue? (I do this because use-package doesn't seem to work well with themes.)

@jrblevin
Copy link
Owner

jrblevin commented Nov 6, 2017

I think you nailed it @ivan-m, thanks. For example, when I switch around the load order in my eigengrau example above, I do indeed get the light background:

emacs -Q -l markdown-mode.el -l eigengrau-definitions.el -l eigengrau-theme.el

@jrblevin
Copy link
Owner

jrblevin commented Nov 6, 2017

@fommil I'm not opposed to removing it if needed--I just hoped to somehow fix it instead.

Some background on this for those interested: I originally tried updating markdown-code-face every time markdown-mode was invoked (unless it was already defined by a theme). That way, if the user switched from a light to dark background theme, reloading markdown-mode would suffice in case of issues. My approach there proved problematic (e.g., #241 and #250), so I switched to simply setting it only once at package load time in commit 7bf0736. As is clear from this issue, that approach is too limited, but perhaps there is a better way to update the face on-the-fly than what I tried previously. For example, I looked for theme loading/unloading hooks but it appears there aren't any.

Does anyone perhaps know how other packages handle updating background colors like this?

@fommil
Copy link
Author

fommil commented Nov 6, 2017

If the user changes the theme I think there are sufficiently many bugs in every emacs package that a refresh-buffer is expected. I don't think you need to workaround this corner case. Otherwise every single major or minor mode would have to do the same. I very much vote to remove this workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants