pimutils / khal

:calendar: CLI calendar application
https://lostpackets.de/khal/
MIT License
2.61k stars 200 forks source link

Question about coloring #1182

Closed cpfaff closed 1 year ago

cpfaff commented 2 years ago

I was trying to get as close to gruvbox coloring as possible. So I started by copying the dark theme in ui/colors.py first and adapted everything to my needs. This is already quite nice. However I thought I could do better. I just saw that it might also be possible to use hex colors. But when I specify one of these and call ikhal I get spit out the message that I require more colors than are specified:

Traceback (most recent call last):
  File "/usr/bin/ikhal", line 5, in <module>
    main_ikhal()
  File "/usr/lib/python3.10/site-packages/click/core.py", line 1130, in __call__
    return self.main(*args, **kwargs)
  File "/usr/lib/python3.10/site-packages/click/core.py", line 1055, in main
    rv = self.invoke(ctx)
  File "/usr/lib/python3.10/site-packages/click/core.py", line 1404, in invoke
    return ctx.invoke(self.callback, **ctx.params)
  File "/usr/lib/python3.10/site-packages/click/core.py", line 760, in invoke
    return __callback(*args, **kwargs)
  File "/usr/lib/python3.10/site-packages/click/decorators.py", line 26, in new_func
    return f(get_current_context(), *args, **kwargs)
  File "/usr/lib/python3.10/site-packages/khal/cli.py", line 498, in interactive_cli
    controllers.interactive(
  File "/usr/lib/python3.10/site-packages/khal/controllers.py", line 575, in interactive
    ui.start_pane(
  File "/usr/lib/python3.10/site-packages/khal/ui/__init__.py", line 1329, in start_pane
    loop = urwid.MainLoop(
  File "/usr/lib/python3.10/site-packages/urwid/main_loop.py", line 118, in __init__
    screen.register_palette(palette)
  File "/usr/lib/python3.10/site-packages/urwid/display_common.py", line 856, in register_palette
    self.register_palette_entry(*item)
  File "/usr/lib/python3.10/site-packages/urwid/display_common.py", line 923, in register_palette_entry
    basic = AttrSpec(foreground, background, 16)
  File "/usr/lib/python3.10/site-packages/urwid/display_common.py", line 540, in __init__
    raise AttrSpecError(('foreground/background (%s/%s) require ' +
urwid.display_common.AttrSpecError: foreground/background ('#00FF00'/'#00FF00') require more colors than have been specified (16).

It seems to be stuck to 16 colors. I do not get this as my suckless terminal actually supports true colors. Do I need to make some more adaptions to khal code to pick that up correctly? And how would I do it?

screenshot-2022-09-03:10:46

EDIT:

* khal --version
khal, version 0.10.5
WhyNotHugo commented 2 years ago

Can you share your code/changes?

Nothing obvious comes to mind, but the issue here is in urwid and what values are being passed to it. The issue is not due to terminal support.

cpfaff commented 2 years ago

Sure I can share it. Here you go. I only edited the colors.py adding another array with colors for gruvbox.

gruvbox = [
    ("header", "white", "black"),
    ("footer", "white", "black"),
    ("today", "#00FF00", "#00FF00"),
    ("today focus", "black", "yellow"),
    ("date header", "white", "black"),
    ("date header focused", "yellow", "black"),
    ("date header selected", "yellow", "black"),
    ("reveal focus", "yellow", "black"),
    ("dayname", "light gray", ""),
    ("monthname", "light gray", ""),
    ("edit", "yellow", "black"),
    ("edit focused", "yellow", "black"),
    ("alert", "white", "dark red"),
    ("mark", "white", "dark green"),
]

I also set this new theme to be picked up in khal.spec. I also set it default here for me just for now altough this could be done by configuration later of course.

theme = option('dark', 'light', 'gruvbox', default='gruvbox')
cpfaff commented 2 years ago

Here in that case you see where I just tried to use hex color codes. That breaks it. Before that I had there "black" and "yellow" named colors.

cpfaff commented 2 years ago

Intresting. Just tried using hex colors in the config to set the calendar colors. There the hex colors work.

cpfaff commented 2 years ago

I am investigating. Here it breaks when passing the palette to the urwid main loop.

    palette = _add_calendar_colors(
        getattr(colors, pane._conf['view']['theme']), pane.collection)

I also saw that there is a fuction that generates a urwid compatible color entry which is named _urwid_palette_entry in __init__.py. That one is also used adding the calendar colors to the palette. Thus it is called from _add_calendar_colors. I see there conditional treatment for different colors and this does the right thing for hex values coming from the configuration. The hard coded color themes however , as far as I can see are passed without such a treatment? That makes it break.

WhyNotHugo commented 2 years ago

So the built-in themes are in a different format than those provided via a config file, and because of that they're also fed differently to urwid. From what I see in your research here, the built-in ones don't support hex colours.

I wonder if built-in themes should share the same format as user-defined ones. E.g.: Just put them into ini files and thread them the same. When a user wants a custom colour, they just write a custom ini file too and point their config file to it.

This would remove the "two separate formats" flow and code. Potentially, code can be made simpler, fix this bug and also make is easy for users to share colour themes or even upstream them into contrib/ (since each theme is just an file).

(ini is just an example here, could be whatever fits best really)

@geier Thoughts?

geier commented 1 year ago

yeah, the color scheme code should be completely re-done. Defining a complete color scheme in the config should be allowed as well. (Or perhaps we should allow plugins.)

geier commented 1 year ago

If I see this correctly, the entries need to be in the form (name, foreground_16_colors, background_16_colors, monochrome_settings, foreground_unilimited_colors, background_unlimited_colors), see the urwid documentation.

Seems to work

image image

(those color names are from the gruvbox site).

While playing a bit with those colors I noticed how many elements currently do not have a attribute element (e.g. most buttons), so we need some work there as well.

geier commented 1 year ago

if anyone is interested in testing defining colors in the config file, please see https://github.com/pimutils/khal/pull/1279

geier commented 1 year ago

I'll close this for now, please re-open this or another one if any questions are still open.