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

Allow theme colors to be specified as integers #1903

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jubalh
Copy link
Member

@jubalh jubalh commented Oct 28, 2023

This PR got opened by @jubalh for toastal who sent his patch via mailinglist and in the profanity MUC.

@jubalh
Copy link
Member Author

jubalh commented Oct 28, 2023

Can't set myself as reviewer by a PR opened by myself.. Great.
Anyways .. I'll have to review this later. Not just opened PR so the patch doesn't get lost.

@jubalh jubalh force-pushed the others/toastal/colors branch from e0e1bff to 97329c6 Compare October 29, 2023 18:08
@jubalh jubalh added the feature label Oct 29, 2023
@jubalh jubalh added this to the next milestone Oct 29, 2023
@jubalh jubalh force-pushed the others/toastal/colors branch from 97329c6 to ffa9073 Compare October 29, 2023 18:11
Copy link
Contributor

@H3rnand3zzz H3rnand3zzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ul could be renamed to something like col_value/col_number, but other than that LGTM.

Allow theme colors to be specified as integers.

Works with both decimal & hex. Given the XXX warning, it seems
reasonable to allow users to specify their theme’s colors as the base
integers since the names are pretty arbitrary.

We changed the variable name `ul` to `col_value` per @H3rnand3zzz
suggestion.
@jubalh jubalh force-pushed the others/toastal/colors branch from ffa9073 to bccfad3 Compare November 20, 2023 11:49
@jubalh
Copy link
Member Author

jubalh commented Nov 20, 2023

ul could be renamed to something like col_value/col_number

done!

I guess we probably should document this possibility somewhere/add an example?

@H3rnand3zzz
Copy link
Contributor

H3rnand3zzz commented Nov 20, 2023

I guess we probably should document this possibility somewhere/add an example?

that would be great finishing touch

@jubalh
Copy link
Member Author

jubalh commented Dec 14, 2023

toastal mentioned he later found a problem with his patch regarding background colors.
I asked several times what problem exactly and how to reproduce but so far didn't get an answer.

I'm considering to close this PR until a new patch that is known to work correctly, or way to reproduce the issue is given.

@toastal
Copy link

toastal commented Dec 15, 2023

I had mentioned the issue. My patch only worked on background colors (titlebar, statusbar, roster.header), but not the other colors. I have never written C so I don’t know how to go about this any more than the time I spent on this.

The code has an explicit block warning about problematic color names & it seems an odd limitation to make users supply strings that need to reverse lookup the matching uint8 color when the alternative of just allowing uint8 values to be put in the config instead of the strings should be a bit quicker, but also allow users to get to the colors with duplicate names.

@toastal
Copy link

toastal commented Dec 15, 2023

If anyone would like to take it over the line themselves or pairing I would assist (especially testing), but I don’t have the skill to finish this--I don’t even know how to properly debug it either… was just shooting at the wall & seeing what stuck.

@H3rnand3zzz
Copy link
Contributor

If anyone would like to take it over the line themselves or pairing I would assist (especially testing), but I don’t have the skill to finish this--I don’t even know how to properly debug it either… was just shooting at the wall & seeing what stuck.

Can you just provide some steps to reproduce a problem? E.g. specific setting in theme. Once it's reproducible, it will be easy to fix.

@toastal
Copy link

toastal commented Dec 16, 2023

@H3rnand3zzz

Here were some colors I was trying

[colours]
bkgnd=default
titlebar=0x36
titlebar.text=0x07
titlebar.brackets=0xB7
titlebar.unencrypted=0xC5
titlebar.encrypted=0x94
titlebar.untrusted=0xD6
titlebar.trusted=0x94
titlebar.online=0x07
titlebar.offline=0x87
titlebar.away=0x87
titlebar.chat=0x07
titlebar.dnd=0x63
titlebar.xa=0x07
statusbar=0xEC
statusbar.text=0xF7
roster.header=0xA2
roster.chat=0x63
roster.online=0x8D

[ui]
time.lastactivity=%y-%m-%d %H:%M:%S
time.vcard=%y-%m-%d %H:%M:%S
time.muc=%m-%d %H:%M:%S

@toastal
Copy link

toastal commented Dec 31, 2023

A program with desired behavior, meli

VALID COLOR VALUES

Color values are of type String with the following valid contents:
[…]
0-255 byte for 256 colors.
xterm(1) name but with some modifications (for a full table see COLOR NAMES addendum) (Case-sensitive)

https://meli-email.org/documentation.html#meli-themes.5_VALID_COLOR_VALUES

I’m going to assume they chose to make modifications since there are name overlaps in the xterm names (not that the descriptions are great)

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

Successfully merging this pull request may close these issues.

4 participants