search

minad / marginalia

:scroll: marginalia.el - Marginalia in the minibuffer
GNU General Public License v3.0
782 stars 27 forks source link

Coloured file attributes #91

Closed tecosaur closed 3 years ago

tecosaur commented 3 years ago

As discussed in the post-merge comments of #90.

Screenshot

image

Worth noting

I've defined specific foreground colours for the new file privilege faces, however it may be worth changing to inherit from ... something? I couldn't see any natural fits so I've just used specific colours. Happy to change if there are any good ideas.

minad commented 3 years ago

Thanks!

protesilaos commented 3 years ago

Thank you! I believe the list included the owner and group like owner:group? Now there is nothing. Is their removal intentional?

iyefrat commented 3 years ago

@protesilaos yeah, it only shows owner:group when it's not the current user.

protesilaos commented 3 years ago

@iyefrat I see. Thanks!

tecosaur commented 3 years ago

As a sample, @protesilaos this is how it looks in action:

image

minad commented 3 years ago

@protesilaos It would be great if you add support for the additional faces to the modus themes. Thank you!

minad commented 3 years ago

@tecosaur I applied some simplifications to your PR https://github.com/minad/marginalia/commit/8886e2413f6c5ea47ae01b78d53b99bb75b5fa2b. The alist is unnecessary, one can use a list and member.

protesilaos commented 3 years ago

As a sample, @protesilaos this is how it looks in action:

Thanks! I just noticed the root:root on my end but not the rest, so I thought something was amiss.

@protesilaos It would be great if you add support for the additional faces to the modus themes. Thank you!

I did it ~3 minutes after this was merged :)

Note that I replicated the diredfl style, but I am reviewing it now as I think we can improve things further (for the sake of consistency).

minad commented 3 years ago

Great, thank you very much!

tecosaur commented 3 years ago

Note that I replicated the diredfl style, but I am reviewing it now as I think we can improve things further (for the sake of consistency).

@protesilaos I've made a PR to Doom themes, so if you have any ideas for how the consistency could be improved it could apply there too -- would you mind letting me know if you have any good ideas?

protesilaos commented 3 years ago

On 2021-07-26, 06:02 -0700, tecosaur @.***> wrote:

Note that I replicated the diredfl style, but I am reviewing it now as I think we can improve things further (for the sake of consistency).

@protesilaos I've made a PR to Doom themes, so if you have any ideas for how the consistency could be improved it could apply there too -- would you mind letting me know if you have any good ideas?

Yes, sure! Once I am done, I will check there.

-- Protesilaos Stavrou https://protesilaos.com

protesilaos commented 3 years ago

Commit 33e86dc (with 444c5d1 tweaks) hardcodes certain font-lock faces. Is it possible to replace those with marginalia-specific ones? That would help to better adapt those styles to the rest of the marginalia aesthetic at the theme level.

protesilaos commented 3 years ago

Also note that marginalia-file-modes is no longer used.

minad commented 3 years ago

@protesilaos

Commit 33e86dc (with 444c5d1 tweaks) hardcodes certain font-lock faces. Is it possible to replace those with marginalia-specific ones? That would help to better adapt those styles to the rest of the marginalia aesthetic at the theme level.

I am not sure. This is a font-lock specific usage, the faces keep their semantic meaning. We highlight strings with font-lock-string etc. Why would you want to adjust these? Actually I would not want the theme to change these.

Also note that marginalia-file-modes is no longer used.

Thanks, I will remove this and for giving it a thorough check!

protesilaos commented 3 years ago

I am not sure. This is a font-lock specific usage, the faces keep their semantic meaning. We highlight strings with font-lock-string etc. Why would you want to adjust these? Actually I would not want the theme to change these.

The faces may be retaining their semantics but have lost the original context in which that meaning is substantiated in terms of visual properties. For example, when we have strings in code syntax, we are not in the process of interacting with minibuffer completion (so the presence of a potentially prominent line highlight, maybe highlights of multiple matching characters scattered across the list, etc.) and we are not faced with a neatly organised tabular presentation. So what may be appropriate in code syntax is not necessarily right in the context of completion+marginalia. Each case needs to be treated differently in order to achieve optimal results.

In practice, the problem is that we are left with a C-h v where we cannot control how marginalia will look. Whereas we can in every other scenario, be it M-x, C-h P, C-x 8 RET, C-x C-f, C-x b... This has the potential to lead to conflicts and inconsistencies, such as when a user applies bold weight in code syntax (i.e. font-lock) only to find bold constructs in completion annotations. Those will be competing for attention with highlighted matches of those characters that are passed to the minibuffer prompt.

Granted, I am not claiming that this is a major issue, breaking change, or whatnot. It is not! This is about consistency within the scope of marginalia, which then ramifies to theme design. With the requisite flexibility we can optimise for each type of listing in accordance with its particular requirements.

Also note that marginalia-file-modes is no longer used.

Thanks, I will remove this and for giving it a thorough check!

You are welcome!

-- Protesilaos Stavrou https://protesilaos.com

minad commented 3 years ago

The faces may be retaining their semantics but have lost the original context in which that meaning is substantiated in terms of visual properties.

I agree with that. On the other hand there is no serious problem here. There won't be a conflict with completion style highlighting, since the faces are only applied to annotations. The only highlighting we interact with is the highlighting of the current line or maybe additional mouse highlighting. I assume that a theme will not use a highlighting which will conflict with the basic font-lock faces, since these are used in many contexts.

I noticed that we have marginalia faces like marginalia-number - so it makes sense to unify the usage of these faces. There is at least an inconsistency here. I will look into it.

protesilaos commented 3 years ago

@protesilaos I've made a PR to Doom themes, so if you have any ideas for how the consistency could be improved it could apply there too -- would you mind letting me know if you have any good ideas?

Yes, sure! Once I am done, I will check there.

@tecosaur As promised, I am done. Will now for a brisk walk and will be back ready to help. I suppose the best place to continue this discussion is in your PR?

oantolin commented 3 years ago

@tecosaur

yeah, it only shows owner:group when it's not the current user.

Yay! I can drop the custom file annotator from my configuration now. 😛