facelessuser / sublime-markdown-popups

Markdown popup dependency for Sublime
https://facelessuser.github.io/sublime-markdown-popups/
Other
112 stars 13 forks source link

Fix the `font_face` setting adaptation #127

Closed absop closed 2 years ago

absop commented 2 years ago

Currently, mdpopups can't automatically adapt the font_face setting of a view, I hope to fix that by submitting this PR.

The only effect of the only submit is the monospace font used by mdpopups, which's change is shown in the following two screenshots.

Before the modification

image

After the modification

image

facelessuser commented 2 years ago

This isn't a bug, but was done intentionally. Not everyone uses Sublime with a Monospace font, as frightening as that is 🙃 . Some people write prose, and other things with it as well. We picked a number of default Monospace fonts to ensure code blocks always render sanely, but you can override those in you own user CSS. Just create a mdpopups.css file in Packages/Users and add:

html {
  --mdpopups-font-mono: "My font";

We've chosen some sane defaults on purpose. With that said, I'm open to exposing the variable for users to use through. If that is desireable, maybe as view_font_style. Then a user could potentially override the font with the user CSS via:

html {
  --mdpopups-font-mono: "{{ var.view_font_style }}";
}
absop commented 2 years ago

How can you get someone who doesn't write or read code in a monospaced font to read code displayed in a pop-up code block using a monospaced font?

I think we should do things the simple way, not everyone will modify CSS, although it seems simple.

rwols commented 2 years ago

Are there any examples of packages geared towards prose and non-mono space fonts that use mdpopups?

absop commented 2 years ago

Are there any examples of packages geared towards prose and non-mono space fonts that use mdpopups?

Yes, there is. I wote one, YoudaoTranslator, but it dose not use monospace font.

facelessuser commented 2 years ago

There are many different plugins that use Mdpopups, and some may display certain information that is preferred to be displayed in a monospace font. Maybe that is code, maybe they are displaying a table with ASCII characters, ASCII diagrams, or something else.

What I am saying is I am willing to compromise on this decision and add a user font variable that you can access in your own user CSS to do exactly what you are proposing here, but I am not keen on forcing the default font for preformatting code blocks (that do not have to include "code" per se).

absop commented 2 years ago

There are many different plugins that use Mdpopups, and some may display certain information that is preferred to be displayed in a monospace font. Maybe that is code, maybe they are displaying a table with ASCII characters, ASCII diagrams, or something else.

What I am saying is I am willing to compromise on this decision and add a user font variable that you can access in your own user CSS to do exactly what you are proposing here, but I am not keen on forcing the default font for preformatting code blocks (that do not have to include "code" per se).

Maybe we can keep the default.css unchanged, but exposing the font_face/view_font_face/view_font_style variable (which will automatically get the value of current "font_face" setting of current view), so that if the user add a "{{ var.font_face/view_font_face/view_font_style }}" in their own CSS, they can let mdpopups adapt their font_face setting.

facelessuser commented 2 years ago

I'm sorry, I misspoke when I said view_font_style, I meant view_font_face. I see no need for a font style variable.

I'm not sure we need both view_font_face and font_face. Mdpopups use view-specific context. If you want some high-level default, just hardcode the font to what you want in this case.

Are there any examples of packages geared towards prose and non-mono space fonts that use mdpopups?

Even if there aren't now, there could be in the future. I'm speaking generally. My point is that I don't want to make general assumptions, but I'm willing to provide a means by which a user can configure there local setup in such a way that it assumes monospace is always being used. This is via exposing view_font_face which can be used in user CSS.

absop commented 2 years ago

Perhaps we could add a new setting, like the one below

{
    // The monospace font family used by mdpopups, it might be:
    // - a list of strings, represents the font-family you want to use
    // - "view_font_face", to automatically get and use the font that
    // your current view is using
    "mdpopups.monospace_font": [
        "sf mono",
        "Consolas",
        "Liberation Mono",
        "Menlo",
        "Courier",
        "monospace"
    ],
}
facelessuser commented 2 years ago

I don't think the setting makes sense. We have the user CSS to allow you to override anything, font or otherwise. It was basically added to ensure a user can override whatever they want and take control if they don't like the defaults.

Adding a font-specific setting like this just seems redundant, at least to me.

absop commented 2 years ago

I don't think the setting makes sense. We have the user CSS to allow you to override anything, font or otherwise. It was basically added to ensure a user can override whatever they want and take control if they don't like the defaults.

Adding a font-specific setting like this just seems redundant, at least to me.

I think for most people, they don't need so complicate feature, they even don't need to know there are a user CSS, edit the settings file is the simplest thing.

facelessuser commented 2 years ago

I think for most people, they don't need so complicate feature, they even don't need to know there are a user CSS, edit the settings file is the simplest thing.

I would argue most people don't need to change the font either, but occasionally, some do. We decided to provide one simple way to change any styling that is desired.

We override Sublime's default style, plugins can append their overrides, and lastly, the user gets the final say with their overrides.

Some people are very opinionated on certain things. Providing one way to override any style instead of providing a special setting for each thing is much less maintenance.

If anything, I'd prefer just creating a simple FAQ to more directly spell out commonly asked things, like controlling monospace fonts.

absop commented 2 years ago

I don't think we can argue to a conclusion, this PR is really not do things in a right way, so I'll close it.

But, Please expose the view_font_face variable. I think, at least, it's useful for the LSP plugin or its users. Nobody wants, to switch a font (the other styles are almost controlled by color schemes), they need to modify ST's settings file (This is relatively simple), and then find the css file to edit it carefully.

I think, for most users of ST, the settings is the simplest, most common used and also the best way to control their editor. They can't remember many things.

facelessuser commented 2 years ago

I don't think we can argue to a conclusion, this PR is really not do things in a right way, so I'll close it.

I think you can expose the variable via this pull request if you like. At the very least, if you do not wish to complete the work, you can open an issue so I don't forget.

Though, thinking about this more, it probably should just reference the global setting's font option instead of the view. The theme is cached for a given color scheme, not really for a given view. I had forgotten that. If we wanted the cache to update when the font changes in the settings file, we'd have to add a check to refresh the cached CSS on the font setting change.

I think, for most users of ST, the settings is the simplest, most common used and also the best way to control their editor.

They aren't controlling the editor, they are editing the popup style/theme. That theme is contained within the CSS file and it allows overrides. That's simply how the theme is controlled. We do use settings where it makes sense, but using it for the font style is redundant as it is contained within the CSS which can already be overridden via a user's CSS. Also, every setting we use that causes changes to the CSS theme requires us to refresh the theme cache when those settings update, so we try and keep those to a minimum.


The question of whether it would have been better to just use the default font or force a monospace was always a big question from the very beginning.

The problem is that people don't directly install mdpopups, they install something else that requires mdpopups. When mdpopups updates, you can't trigger a Package Control message to let people know what has changed. It's kind of expected that it just works out of the box. If things aren't formatted properly, they may assume it is the plugin that is causing the issue and not mdpopups.

The reason we hardcoded the monospace font was to ensure the average user, who isn't even aware that they are using mdpopups, always has sane formatting out of the box. The current default provides that at the expense that the code blocks may not match a user's favorite font. I'm sure the topic could be argued in favor of either direction. I chose the path that ensured a sane output over font aesthetics.