Send plymouth mailing list submissions to
[email protected]
To subscribe or unsubscribe via the World Wide Web, visit
http://lists.freedesktop.org/mailman/listinfo/plymouth
or, via email, send a message with subject or body 'help' to
[email protected]
You can reach the person managing the list at
[email protected]
When replying, please edit your Subject line so it is more specific
than "Re: Contents of plymouth digest..."
Today's Topics:
1. Re: [PATCH 1/4] ply-text-display: support bright colors
(Ray Strode)
2. Re: [PATCH 2/4] ply-terminal: remove pointless casts (Ray Strode)
3. Re: [PATCH 3/4] build: fix typo in configure.ac (Ray Strode)
4. Re: [PATCH 4/4] ply-text-progress-bar: strip quotes if
present (Ray Strode)
5. Re: [PATCH 1/4] ply-text-display: support bright colors
(Jan Engelhardt)
6. Re: [PATCH 3/4] build: fix typo in configure.ac (Jan Engelhardt)
7. Re: [PATCH 1/4] ply-text-display: support bright colors
(Ray Strode)
8. Re: [PATCH 1/4] ply-text-display: support bright colors
(Jan Engelhardt)
9. Re: [PATCH 3/4] build: fix typo in configure.ac (Ray Strode)
----------------------------------------------------------------------
Message: 1
Date: Mon, 30 Jul 2012 15:59:50 -0400
From: Ray Strode <[email protected]>
To: Jan Engelhardt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/4] ply-text-display: support bright colors
Message-ID:
<CAA_UwzJpDtzyRc+PYQ=Ox5KexR=u5fww3lbdjaopa2j--mo...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi,
> @@ -44,8 +44,16 @@ typedef enum
> PLY_TERMINAL_COLOR_BLUE,
> PLY_TERMINAL_COLOR_MAGENTA,
> PLY_TERMINAL_COLOR_CYAN,
> + PLY_TERMINAL_COLOR_GRAY,
> + PLY_TERMINAL_COLOR_DARKGRAY,
> + PLY_TERMINAL_COLOR_BRIGHTRED,
> + PLY_TERMINAL_COLOR_BRIGHTGREEN,
> + PLY_TERMINAL_COLOR_YELLOW,
> + PLY_TERMINAL_COLOR_BRIGHTBLUE,
> + PLY_TERMINAL_COLOR_BRIGHTMAGENTA,
> + PLY_TERMINAL_COLOR_BRIGHTCYAN,
> PLY_TERMINAL_COLOR_WHITE,
> - PLY_TERMINAL_COLOR_DEFAULT = PLY_TERMINAL_COLOR_WHITE + 2
> + PLY_TERMINAL_COLOR_DEFAULT,
> } ply_terminal_color_t;
So this highlights a problem in the current API: we have this special color
"DEFAULT" that's supposed to mean the terminal default color (and indeed does
mean that in an ANSI escape sequence sort of way), but that same value means
"bright red" when setting up terminal palettes.
I think rather than trying to keep this square peg, we could just drop
PLY_TERMINAL_COLOR_DEFAULT, and introduce new apis:
ply_text_display_reset_background_color (display);
ply_text_display_reset_foreground_color (display);
What do you think?
minor review nits below:
> + if (color == PLY_TERMINAL_COLOR_DEFAULT)
> + color = 9;
We unfortunately use GNU coding style and, also, we don't use tabs.
> @@ -190,9 +197,19 @@ void
> ply_text_display_set_foreground_color (ply_text_display_t *display,
> ply_terminal_color_t color)
> {
> + bool hi = false;
> +
> + if (color >= PLY_TERMINAL_COLOR_DARKGRAY &&
> + color <= PLY_TERMINAL_COLOR_WHITE) {
> + color -= PLY_TERMINAL_COLOR_DARKGRAY;
> + hi = true;
> + } else if (color == PLY_TERMINAL_COLOR_DEFAULT) {
> + color = 9;
> + }
> ply_terminal_write (display->terminal,
> - COLOR_SEQUENCE_FORMAT,
> - FOREGROUND_COLOR_BASE + color);
> + "\x1b\x5b""%d;%u;%um", hi,
> + FOREGROUND_COLOR_BASE + color,
> + BACKGROUND_COLOR_BASE + display->background_color);
Should update COLOR_SEQUENCE_FORMAT to use the new format rather than
stop using it, or remove the COLOR_SEQUENCE_FORMAT definition (though
I'd prefer if we kept it).
Out of curiosity, why are you setting the background color in the
set_foreground_color function?
--Ray
------------------------------
Message: 2
Date: Mon, 30 Jul 2012 16:08:10 -0400
From: Ray Strode <[email protected]>
To: Jan Engelhardt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 2/4] ply-terminal: remove pointless casts
Message-ID:
<CAA_UwzKc==sueojcvn-uir6yys0ur+mu1ejgb67anvgrn09...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi,
On Sun, Jul 29, 2012 at 2:32 PM, Jan Engelhardt <[email protected]> wrote:
> The target variables are already of the desired type.
I'm pretty indifferent about this change. I think it's slightly
clearer the way it is now,
but it's clearly subjective. I don't feel strongly about it, so sure.
--Ray
------------------------------
Message: 3
Date: Mon, 30 Jul 2012 16:09:07 -0400
From: Ray Strode <[email protected]>
To: Jan Engelhardt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 3/4] build: fix typo in configure.ac
Message-ID:
<caa_uwzjgnhd79izo42cgwn0luu-a4wywkm_f0jjudvxjbvb...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi,
On Sun, Jul 29, 2012 at 2:32 PM, Jan Engelhardt <[email protected]> wrote:
> -AC_ARG_WITH(release-file,
> AS_HELP_STRING([--with-release-file=<path_to_release_file>],[Release File to
> use to detect distribution (by default
> /etc/system-reelase)]),RELEASE_FILE=${withval},RELEASE_FILE=/etc/system-release)
> +AC_ARG_WITH(release-file,
> AS_HELP_STRING([--with-release-file=<path_to_release_file>],[Release File to
> use to detect distribution (by default
> /etc/system-relase)]),RELEASE_FILE=${withval},RELEASE_FILE=/etc/system-release)
Yours has a typo too !
--Ray
------------------------------
Message: 4
Date: Mon, 30 Jul 2012 16:09:57 -0400
From: Ray Strode <[email protected]>
To: Jan Engelhardt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 4/4] ply-text-progress-bar: strip quotes if
present
Message-ID:
<caa_uwzkutjujdhucg5whwqdo0m7cw3_t7qp7c-26m6yr96f...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi,
On Sun, Jul 29, 2012 at 2:32 PM, Jan Engelhardt <[email protected]> wrote:
> The syntax for sysconfig-style files allows for quotes.
Sure.
--Ray
------------------------------
Message: 5
Date: Tue, 31 Jul 2012 10:06:06 +0200 (CEST)
From: Jan Engelhardt <[email protected]>
To: Ray Strode <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/4] ply-text-display: support bright colors
Message-ID: <[email protected]>
Content-Type: TEXT/PLAIN; charset=US-ASCII
On Monday 2012-07-30 21:59, Ray Strode wrote:
>
>I think rather than trying to keep this square peg, we could just drop
>PLY_TERMINAL_COLOR_DEFAULT, and introduce new apis:
>
>ply_text_display_reset_background_color (display);
>ply_text_display_reset_foreground_color (display);
>
>What do you think?
I am completely impartial to that :)
>minor review nits below:
>
>> + if (color == PLY_TERMINAL_COLOR_DEFAULT)
>> + color = 9;
>
>We unfortunately use GNU coding style
Somehow I must have missed that; plymouth looks a lot cleaner than glibc.
>> + if (color >= PLY_TERMINAL_COLOR_DARKGRAY &&
>> + color <= PLY_TERMINAL_COLOR_WHITE) {
>> + color -= PLY_TERMINAL_COLOR_DARKGRAY;
>> + hi = true;
>> + } else if (color == PLY_TERMINAL_COLOR_DEFAULT) {
>> + color = 9;
>> + }
>> ply_terminal_write (display->terminal,
>> - COLOR_SEQUENCE_FORMAT,
>> - FOREGROUND_COLOR_BASE + color);
>> + "\x1b\x5b""%d;%u;%um", hi,
>> + FOREGROUND_COLOR_BASE + color,
>> + BACKGROUND_COLOR_BASE + display->background_color);
>Should update COLOR_SEQUENCE_FORMAT to use the new format rather than
>stop using it, or remove the COLOR_SEQUENCE_FORMAT definition (though
>I'd prefer if we kept it).
It should then be removed, because you need different sequences
almost everytime. Format strings should preferably also be at the
site of where they are used, rather than stashed away into a lonely
#define somewhere at the top.
>Out of curiosity, why are you setting the background color in the
>set_foreground_color function?
Because the "0" in \e[0;...m, which is emitted when going from bold to
non-bold, resets everything, including bgcolor?
------------------------------
Message: 6
Date: Tue, 31 Jul 2012 10:07:17 +0200 (CEST)
From: Jan Engelhardt <[email protected]>
To: Ray Strode <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 3/4] build: fix typo in configure.ac
Message-ID: <[email protected]>
Content-Type: TEXT/PLAIN; charset=US-ASCII
On Monday 2012-07-30 22:09, Ray Strode wrote:
>Hi,
>
>On Sun, Jul 29, 2012 at 2:32 PM, Jan Engelhardt <[email protected]> wrote:
>> -AC_ARG_WITH(release-file,
>> AS_HELP_STRING([--with-release-file=<path_to_release_file>],[Release File to
>> use to detect distribution (by default
>> /etc/system-reelase)]),RELEASE_FILE=${withval},RELEASE_FILE=/etc/system-release)
>> +AC_ARG_WITH(release-file,
>> AS_HELP_STRING([--with-release-file=<path_to_release_file>],[Release File to
>> use to detect distribution (by default
>> /etc/system-relase)]),RELEASE_FILE=${withval},RELEASE_FILE=/etc/system-release)
>Yours has a typo too !
With a long unreadable line like that, are you surprised? :)
------------------------------
Message: 7
Date: Tue, 31 Jul 2012 11:22:17 -0400
From: Ray Strode <[email protected]>
To: Jan Engelhardt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/4] ply-text-display: support bright colors
Message-ID:
<caa_uwzkd+87vnsgpgx62hyfndy_yx8_og9-0zvhbw4d-sad...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi,
On Tue, Jul 31, 2012 at 4:06 AM, Jan Engelhardt <[email protected]> wrote:
>
> On Monday 2012-07-30 21:59, Ray Strode wrote:
>>
>>I think rather than trying to keep this square peg, we could just drop
>>PLY_TERMINAL_COLOR_DEFAULT, and introduce new apis:
>>
>>ply_text_display_reset_background_color (display);
>>ply_text_display_reset_foreground_color (display);
>>
>>What do you think?
>
> I am completely impartial to that :)
So I gave this some more thought and decided this is actually a bad
idea. The problem is then there's no good value to return for
ply_text_display_get_{foreground,backgroud}_color then when the colors
are reset. We need to have a symbolic value anyway
to represent the unset case. So since we need to have a value tucked
away in the enum anyway for the getters, we might as
well use it for the setters.
>>> + if (color >= PLY_TERMINAL_COLOR_DARKGRAY &&
>>> + color <= PLY_TERMINAL_COLOR_WHITE) {
>>> + color -= PLY_TERMINAL_COLOR_DARKGRAY;
>>> + hi = true;
>>> + } else if (color == PLY_TERMINAL_COLOR_DEFAULT) {
>>> + color = 9;
>>> + }
>>> ply_terminal_write (display->terminal,
>>> - COLOR_SEQUENCE_FORMAT,
>>> - FOREGROUND_COLOR_BASE + color);
>>> + "\x1b\x5b""%d;%u;%um", hi,
>>> + FOREGROUND_COLOR_BASE + color,
>>> + BACKGROUND_COLOR_BASE + display->background_color);
>>Should update COLOR_SEQUENCE_FORMAT to use the new format rather than
>>stop using it, or remove the COLOR_SEQUENCE_FORMAT definition (though
>>I'd prefer if we kept it).
>
> It should then be removed, because you need different sequences
> almost everytime. Format strings should preferably also be at the
> site of where they are used, rather than stashed away into a lonely
> #define somewhere at the top.
Well the idea is to try to somewhat separate logic from mechanics. if
a drive by reader sees
terminal_write (COLOR_SEQUENCE, FG_COLOR_BASE + color)
then it might be pretty obvious to the user that the code is trying to
change the fg color of the terminal.
If a driveby user sees
terminal_write ("\x1b\x5b%d;%d%um", hi, FG_COLOR_BASE + color,
BG_COLOR_BASE + display->bg_color);
then they'll have to decode the line noise and figure out what it means.
That's also one of the reasons I don't like seeing BG color in the FG
color function. It means people not familar with
the code have to work harder to follow along.
Using bg color also has the problem that display->bg_color needs to be
initialized before you can set fg_color.
> Because the "0" in \e[0;...m, which is emitted when going from bold to
> non-bold, resets everything, including bgcolor?
Instead of using \e[0m maybe use \e[22m ? That should just toggle
high brightness off, but leave bg color alone, I think.
--Ray
------------------------------
Message: 8
Date: Tue, 31 Jul 2012 17:37:57 +0200 (CEST)
From: Jan Engelhardt <[email protected]>
To: Ray Strode <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 1/4] ply-text-display: support bright colors
Message-ID: <[email protected]>
Content-Type: TEXT/PLAIN; charset=US-ASCII
On Tuesday 2012-07-31 17:22, Ray Strode wrote:
>
>If a driveby user sees
>
>terminal_write ("\x1b\x5b%d;%d%um", hi, FG_COLOR_BASE + color,
>BG_COLOR_BASE + display->bg_color);
>
>then they'll have to decode the line noise and figure out what it means.
Add a comment? Though it borders on Obviousness to say
/* This is the VT100 code for changing $foo */
given the function is all about setting color.
>> Because the "0" in \e[0;...m, which is emitted when going from bold to
>> non-bold, resets everything, including bgcolor?
>Instead of using \e[0m maybe use \e[22m ? That should just toggle
>high brightness off, but leave bg color alone, I think.
Let's use that.
------------------------------
Message: 9
Date: Tue, 31 Jul 2012 11:43:54 -0400
From: Ray Strode <[email protected]>
To: Jan Engelhardt <[email protected]>
Cc: [email protected]
Subject: Re: [PATCH 3/4] build: fix typo in configure.ac
Message-ID:
<caa_uwzjy79fdlj-vtqkhcw4-0hs5w03_zeoccvwj3pvqmjq...@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Hi,
On Tue, Jul 31, 2012 at 4:07 AM, Jan Engelhardt <[email protected]> wrote:
> On Monday 2012-07-30 22:09, Ray Strode wrote:
>
>>Hi,
>>
>>On Sun, Jul 29, 2012 at 2:32 PM, Jan Engelhardt <[email protected]> wrote:
>>> -AC_ARG_WITH(release-file,
>>> AS_HELP_STRING([--with-release-file=<path_to_release_file>],[Release File
>>> to use to detect distribution (by default
>>> /etc/system-reelase)]),RELEASE_FILE=${withval},RELEASE_FILE=/etc/system-release)
>>> +AC_ARG_WITH(release-file,
>>> AS_HELP_STRING([--with-release-file=<path_to_release_file>],[Release File
>>> to use to detect distribution (by default
>>> /etc/system-relase)]),RELEASE_FILE=${withval},RELEASE_FILE=/etc/system-release)
>>Yours has a typo too !
>
> With a long unreadable line like that, are you surprised? :)
Nope, yay for autogoo. Though, I guess i could add some line breaks.
--Ray
------------------------------
_______________________________________________
plymouth mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/plymouth
End of plymouth Digest, Vol 42, Issue 6
***************************************
