fairyglade/ly
13
605
Fork
You've already forked ly
65

Reorganize Config #892

Closed
OkayishComputer wants to merge 11 commits from OkayishComputer/ly:master into master
pull from: OkayishComputer/ly:master
merge into: fairyglade:master
fairyglade:master
fairyglade:zig-0.17
fairyglade:v1.4.x
fairyglade:v1.3.x
fairyglade:v1.2.x
fairyglade:v1.1.x
fairyglade:v1.0.x
First-time contributor
Copy link

What are the changes about?

reorganized options and rephrased / added some comments to make things clearer for new users

What existing issue does this resolve?

mentioned in #887

Pre-requisites

  • I have tested & confirmed the changes work locally

It should have no functional impact on ly itself, only on the config layout.

## What are the changes about? reorganized options and rephrased / added some comments to make things clearer for new users ## What existing issue does this resolve? mentioned in #887 ## Pre-requisites - [X] I have tested & confirmed the changes work locally It should have no functional impact on ly itself, only on the config layout.
grouped related settings/comments together under headings for easier parsing
Re-organized options to keep related settings together, rephrased some of the color comments to make the color behavior clearer to new users, added headings to sections, added diagram of main box
OkayishComputer changed title from (削除) WIP Reorganize Config (削除ここまで) to Reorganize Config 2025年12月19日 04:27:04 +01:00 
refined grouping and ordering of items a bit
grouped related settings/comments together under headings for easier parsing
Re-organized options to keep related settings together, rephrased some of the color comments to make the color behavior clearer to new users, added headings to sections, added diagram of main box
refined grouping and ordering of items a bit
keeping up to date
AnErrupTion left a comment
Copy link

I found 2 major "problems" (the second one isn't really a problem to be fair) after taking a quick glance at the changes:

  1. Don't replace the special $PREFIX_DIRECTORY, $CONFIG_DIRECTORY, etc. special variables. Those are used for platform-specific reasons and are automatically replaced at build time by the build script, so you don't need to touch them.
  2. The alphabetical order is no longer respected. I find this to be a problem because not only do other configuration & localisation files do the same, but also for coherency with the code (as struct fields are also sorted alphabetically). Functionally speaking, it doesn't change anything, but I think it'd be best to keep the previous way of doing things in that regard for coherency purposes.

I'll take on a proper review once that's done. Although after that quick glance I took at the changes, I can definitely say that I do like at least some of the new comments you've made!

I found 2 major "problems" (the second one isn't really a problem to be fair) after taking a quick glance at the changes: 1. Don't replace the special `$PREFIX_DIRECTORY`, `$CONFIG_DIRECTORY`, etc. special variables. Those are used for platform-specific reasons and are automatically replaced at build time by the build script, so you don't need to touch them. 2. The alphabetical order is no longer respected. I find this to be a problem because not only do other configuration & localisation files do the same, but also for coherency with the code (as struct fields are also sorted alphabetically). Functionally speaking, it doesn't change anything, but I think it'd be best to keep the previous way of doing things in that regard for coherency purposes. I'll take on a proper review once that's done. Although after that quick glance I took at the changes, I can definitely say that I do like at least some of the new comments you've made!
Author
First-time contributor
Copy link

I didn't even notice that it was alphabetical. That explains so much, lol.

Don't know how the variables got expanded; it was not my intention to change anything but the order and comments.

I didn't even notice that it was alphabetical. That explains so much, lol. Don't know how the variables got expanded; it was not my intention to change anything but the order and comments.
Contributor
Copy link

I like the new explanations for the color situation, I just want to comment that in my understanding (and my working configuration), even in eight-color mode, the colors must be 32-bit values with the styling in the most significant byte. So if you want to apply styling to a color with full_color set to false, the color value must be composed of one byte with the styling code, two null bytes, and a final byte with the indexed color code.

I like the new explanations for the color situation, I just want to comment that in my understanding (and my working configuration), even in eight-color mode, the colors must be 32-bit values with the styling in the most significant byte. So if you want to apply styling to a color with `full_color` set to `false`, the color value must be composed of one byte with the styling code, two null bytes, and a final byte with the indexed color code.

@asterane wrote in #892 (comment):

I like the new explanations for the color situation, I just want to comment that in my understanding (and my working configuration), even in eight-color mode, the colors must be 32-bit values with the styling in the most significant byte. So if you want to apply styling to a color with full_color set to false, the color value must be composed of one byte with the styling code, two null bytes, and a final byte with the indexed color code.

In 16-color mode, colors should be a single byte, or two for styling. You can technically pass in 32-bit color values, but that's not what's expected (although termbox2 may already be interpreting this correctly for us anyway).

@asterane wrote in https://codeberg.org/fairyglade/ly/pulls/892#issuecomment-9946323: > I like the new explanations for the color situation, I just want to comment that in my understanding (and my working configuration), even in eight-color mode, the colors must be 32-bit values with the styling in the most significant byte. So if you want to apply styling to a color with `full_color` set to `false`, the color value must be composed of one byte with the styling code, two null bytes, and a final byte with the indexed color code. In 16-color mode, colors *should* be a single byte, or two for styling. You can technically pass in 32-bit color values, but that's not what's expected (although termbox2 may already be interpreting this correctly for us anyway).
Contributor
Copy link

I tested on 1.3.1; with a value like 0x4008, the desired styling is ignored and I see regular dim white. Change the value to 0x40000008, and I see bright white as intended.

This happens because Ly sets TB_OPT_ATTR_W to 32 in order to support full color. Changing the Termbox output mode to eight-color doesn't change the argument width Termbox expects. This is documented in termbox2.h at lines 69 and 277, and the implementation of styling later in the file also demonstrates it.

I tested on 1.3.1; with a value like 0x4008, the desired styling is ignored and I see regular dim white. Change the value to 0x40000008, and I see bright white as intended. This happens because Ly sets `TB_OPT_ATTR_W` to `32` in order to support full color. Changing the Termbox output mode to eight-color doesn't change the argument width Termbox expects. This is documented in `termbox2.h` at lines 69 and 277, and the implementation of styling later in the file also demonstrates it.

Closing due to inactivity. If new activity is shown from OP, the PR can be reopened without any issues.

Closing due to inactivity. If new activity is shown from OP, the PR can be reopened without any issues.

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
No reviewers
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
fairyglade/ly!892
Reference in a new issue
fairyglade/ly
No description provided.
Delete branch "OkayishComputer/ly:master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?