Allow opting-out of strike-through

[morganwillcock]

Would you consider adding a defcustom equivalent of modus-themes-italic-constructs and modus-themes-bold-constructs to control the use of strike-through?

Testing version 5.0.0 of the themes, faces like jabber-roster-user-offline are now using strike-through but this looks a little confusing because the separator in the roster (where users are displayed) also uses text that looks similar to strike-through for a separator:

The same face is used in messages and status updates but for the word "offline" rather than the username. An example would be when a user goes offline, the message displayed in the echo area is now "username is now Offline".

Given that strike-through makes the text harder to read and also the application of the faces may be unexpected, I would probably opt-out of the use of additional strike-through entirely if there was an option to do it.

[protesilaos]

Hello @morganwillcock!

I think what I have right now is simply wrong. How does this work for you? Please evaluate it and then check. If it is okay, then I will make it the default.

(set-face-attribute 'jabber-roster-user-offline nil :strike-through nil)

[morganwillcock]

That does remove the strike-through for the roster entry and the messages, although I think it might just be better to have a global opt-out and leave people to tweak specific faces on top of that. If I used Bongo or Forge I might have the same issues with their use of faces which use strike-through.

[protesilaos]

I removed the strike-through from the Jabber face. In principle, I am fine with such a user option, though I am not committed to the strike-through styles. I prefer to review them on a case-by-case basis. In Bongo, for example, it makes sense.

[morganwillcock]

I would worry that the case-by-case examples could end up being very scenario specific. The jabber.el one is fairly easy to assess because the offline status is likely always going to be a fixed string, whereas a custom status message, or a song name in Bongo, or subject in Forge could potentially be more or less problematic.

Just as an example:

As a status message I can take a good guess that this says "Offline", but if this was a song title I would also have to assume that the name of the song was not "0fftinc".

I also imagine the readability may be affected significantly by the font used, potentially something like a hyphen could be missed entirely:
--- -- -
(For me, in the GitHub web interface, these 6 hyphens are effectively not present because they are completely obscured by the strike-through.)

I wouldn't mind if the Modus themes used strike-through for every kind of offline/disabled/deleted type face as long as it was easy to opt-out.

[protesilaos]

Hello @morganwillcock! I was busy fixing a regression in modus-themes-with-colors. Now I am back.

To your point, I think it is better to remove the strike through than to try to work around it. How best to express the alternative though? The obvious choice is a subtle foreground, but this may not be enough.

[morganwillcock]

I think to remove strike-through entirely someone would have to look at each place it is used and then decide for each face:

• Is the strike-through (or the strike-through colour) the only differentiator between this face and another face which is going to be displayed in close proximity?
• Was the strike-through actually intended to indicate that reading the text can be skipped because it is now less relevant or less correct than other text being displayed?

It looks like there are 12 faces that use strike-through:

$ grep 'strike-through [t,]' modus-themes.el
    `(bongo-played-track ((,c :foreground ,fg-dim :strike-through t)))
    `(company-tooltip-deprecated ((,c :inherit modus-themes-fixed-pitch :background ,bg-dim :strike-through t)))
    `(custom-invalid ((,c :foreground ,err :strike-through t)))
    `(custom-rogue ((,c :foreground ,err :strike-through t)))
    `(forge-issue-unplanned ((,c :foreground ,fg-dim :strike-through t)))
    `(forge-pullreq-rejected ((,c :foreground ,err :strike-through t)))
    `(forge-topic-slug-unplanned ((,c :foreground ,fg-dim :strike-through t)))
    `(neo-vc-removed-face ((,c :strike-through t)))
    `(notmuch-tag-deleted ((,c :strike-through ,err)))
    `(package-status-disabled ((,c :foreground ,err :strike-through t)))
    `(transient-key-noop ((,c :inherit (bold modus-themes-fixed-pitch) :foreground ,keybind :strike-through t)))
    `(ztreep-diff-model-ignored-face ((,c :foreground ,fg-dim :strike-through t)))

9 of those already look to incorporate dimming or an error color, so possibly these only need to be checked against other faces for the same feature/package to see if they are still unique if strike-through is removed while still being consistent with original intentions:

$ grep 'strike-through [t,]' modus-themes.el | grep '\(-dim\|,err\) '
    `(bongo-played-track ((,c :foreground ,fg-dim :strike-through t)))
    `(company-tooltip-deprecated ((,c :inherit modus-themes-fixed-pitch :background ,bg-dim :strike-through t)))
    `(custom-invalid ((,c :foreground ,err :strike-through t)))
    `(custom-rogue ((,c :foreground ,err :strike-through t)))
    `(forge-issue-unplanned ((,c :foreground ,fg-dim :strike-through t)))
    `(forge-pullreq-rejected ((,c :foreground ,err :strike-through t)))
    `(forge-topic-slug-unplanned ((,c :foreground ,fg-dim :strike-through t)))
    `(package-status-disabled ((,c :foreground ,err :strike-through t)))
    `(ztreep-diff-model-ignored-face ((,c :foreground ,fg-dim :strike-through t)))

...that leaves 3 where strike-through is used on face that doesn't already use dimming or an error colour:

$ grep 'strike-through [t,]' modus-themes.el | grep -v '\(-dim\|,err\) '
    `(neo-vc-removed-face ((,c :strike-through t)))
    `(notmuch-tag-deleted ((,c :strike-through ,err)))
    `(transient-key-noop ((,c :inherit (bold modus-themes-fixed-pitch) :foreground ,keybind :strike-through t)))

A subtle foreground colour to dim the text seems the logical thing to try for a filename that is currently displayed but deleted, or for a key-binding that is currently displayed but does nothing. For mail, maybe it depends on whether the face is used to indicate what is going to be deleted or whether it indicates what was already deleted - personally I would want to be very aware of the former (highlight it) and less concerned with the latter (dim it).

How do you normally view faces for comparison? Is there any function I can call to display faces based on a face-name prefix like "neo-vc-" without actually having the corresponding packages installed?

[protesilaos]

Thank you @morganwillcock for looking into this!

...that leaves 3 where strike-through is used on face that doesn't already use dimming or an error colour:

I will review them all because I actually am not a big fan of the strike-through effect. I had it there because it was used in some of the default values of those faces.

To your point, the transient-key-noop is no longer relying on a string-through style. I will check notmuch now, since I am a user of it. Then, I will set up neotree for review.

How do you normally view faces for comparison? Is there any function I can call to display faces based on a face-name prefix like "neo-vc-" without actually having the corresponding packages installed?

You need to install the package and require it. Then you can preview the faces with list-faces-display. Give it a universal prefix argument to be prompted for a regexp of faces to match.

Though note that the preview alone is not enough. They might look fine there, but ideally we want to have a good idea of how they are used in context.