-
Notifications
You must be signed in to change notification settings - Fork 131
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
Fix issue #106, to make it work on a real DEC VT320 #107
base: master
Are you sure you want to change the base?
Conversation
would be nice to have comments inline in the code to explain where that magic come from. also please provide a screenshot of what it will look like in modern terminal emulators, e.g. like xterm and gnome-terminal. i know it's backwards but most people will use ttyclock there... |
Added comments to clarify "magic strings" and simplified some expression. Tomorrow I will test it on both real VT320 and "normal" Terminal window on Xwindow screen and will post screenshot here |
ttyclock.c
Outdated
@@ -218,6 +218,8 @@ void | |||
draw_number(int n, int x, int y) | |||
{ | |||
int i, sy = y; | |||
unsigned int on = COLOR_PAIR(2) | A_REVERSE; | |||
unsigned int off = COLOR_PAIR(0) | A_NORMAL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
those seem a little ... terse. what is "on"? color? expand the variable.
why isn't this a #define
too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I'm going to use more significant names for "on/off", for example "pixel_on" and "pixel_off".
I've not used a #define because the aim is to calculate the value at the beginning of the function and use it immedialy in the inner for cycle. Since the #define is just a text replacement, it does not implement precalculated values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anarcat would you use different names than "pixel_on" / "pixel_off"? Can you suggest me? For example:
- colored_reverse
- transparent_normal
would be more descriptive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i frankly have no idea what's going on here, so i'll trust your judgement blindly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will explain:
pixel_on is a bitmask made by:
- COLOR_PAIR(2), defined by "init_color" here https://github.com/xorg62/tty-clock/blob/master/ttyclock.c#L540
- A_REVERSE is the attribute that defines reverse characters (reversed space is a big "pixel")
pixel_off is anothter bitmask made by:
- COLOR_PAIR(0) which means "not visible"
- A_NORMAL is "not reverse" attribute
so I use "pixel_on" to draw the "block characters" which are reversed spaces, supported by both xterm and DECterm, and "pixel_off" to draw the "empty space".
is it good to merge it? ;)
On 2022-02-08 01:32:49, Francesco Sblendorio wrote:
@sblendorio commented on this pull request.
> @@ -218,6 +218,8 @@ void
draw_number(int n, int x, int y)
{
int i, sy = y;
+ unsigned int on = COLOR_PAIR(2) | A_REVERSE;
+ unsigned int off = COLOR_PAIR(0) | A_NORMAL;
I will explain:
pixel_on is a bitmask made by:
- COLOR_PAIR(2), defined by "init_color" here https://github.com/xorg62/tty-clock/blob/master/ttyclock.c#L540
- A_REVERSE is the attribute that defines reverse characters (reversed space is a big "pixel")
pixel_off is anothter bitmask made by:
- COLOR_PAIR(0) which means "not visible"
- A_NORMAL is "not reverse" attribute
those explanations would be tremendously more useful as comments in the
source code. ;)
so I use "pixel_on" to draw the "block characters" which are reversed spaces, supported by both xterm and DECterm, and "pixel_off" to draw the "empty space".
then those are good names.
…--
Perl is "some assembly required". Python is "batteries included". PHP
is "kitchen sink, but it’s from Canada and both faucets are labeled C".
- Alex Munroe, PHP: a fractal of bad design
|
@anarcat just added the comment for "pixel_on" and "pixel_off" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
awesome, LGTM.
no merge? |
On 2022-02-11 01:05:41, Francesco Sblendorio wrote:
> awesome, LGTM.
no merge?
i'm actually not the owner of this repository. typically, i approve MRs,
then let them sit a while to give them a chance to see changes, then i
merge if nothing happens.
--
In a racist society it is not enough to be non-racist, we must be antiracist.
- Angela Davis
|
hmm... why do we mess with colors here at all then? |
to support backward compatibility we could leave the current color options, but adding an additional command line option, perhaps --no-color or -nc could disable the color setting and use the terminals 3 colors ( background, text, highlight) whatever they are called. |
Fix issue #106