[Patch] Set colours correctly when capturing pane and grid cell resets attribute

Discussion:

Matthew Darby

2014-09-16 18:39:38 UTC

Hi,

grid_string_cells_code in grid.c currently resets all attributes when it finds the current cell lacks an attribute from the previous cell. Remaining non-colour attributes seem to be well-looked after, but any colours set are lost. The supplied patch restores the colours from the previous cell into the current cell. This seems to be fine even if the current cell also changes the colours, because the previously set colours will be ignored.

Thanks,
Matthew Darby

Nicholas Marriott

2014-09-17 15:30:04 UTC

Permalink

If the cell is default fg/bg, won't this add \e[0m\e[39m\[49m? The two
colour changes are unnecessary.

Post by Matthew Darby
Hi,
grid_string_cells_code in grid.c currently resets all attributes when it
finds the current cell lacks an attribute from the previous cell.
Remaining non-colour attributes seem to be well-looked after, but any
colours set are lost. The supplied patch restores the colours from the
previous cell into the current cell. This seems to be fine even if the
current cell also changes the colours, because the previously set colours
will be ignored.
Thanks,
Matthew Darby
------------------------------------------------------------------------------
Want excitement?
Manually upgrade your production database.
When you want reliability, choose Perforce
Perforce version control. Predictably reliable.
http://pubads.g.doubleclick.net/gampad/clk?id=157508191&iu=/4140/ostg.clktrk
_______________________________________________
tmux-users mailing list
https://lists.sourceforge.net/lists/listinfo/tmux-users

Nicholas Marriott

2014-09-17 20:55:09 UTC

Permalink

Does this work? If we do this can probably drop lastattr too and maybe
move the memcpy inside the _code func.

diff --git a/grid.c b/grid.c
index a84a58b..12ec643 100644
--- a/grid.c
+++ b/grid.c
@@ -477,7 +477,7 @@ grid_string_cells_bg(const struct grid_cell *gc, int *values)
* bytes.
*/
void
-grid_string_cells_code(const struct grid_cell *lastgc,
+grid_string_cells_code(struct grid_cell *lastgc,
const struct grid_cell *gc, char *buf, size_t len, int escape_c0)
{
int oldc[16], newc[16], s[32];
@@ -505,6 +505,8 @@ grid_string_cells_code(const struct grid_cell *lastgc,
if (!(attr & attrs[i].mask) && (lastattr & attrs[i].mask)) {
s[n++] = 0;
lastattr &= GRID_ATTR_CHARSET;
+ lastgc->fg = lastgc->bg = 7;
+ lastgc->flags &= ~(GRID_FLAG_FG256|GRID_FLAG_BG256);
break;
}
}

1) If the previous cell's colour was default, it will add the above default colours superfluously.
2) If the current cell colour changes in addition to the attribute being removed, you get a code like \0;39;49;31m. This is a superfluous colour change.
I'm not sure how to proceed without restructuring the function (it's well suited to the structure it already has). I'm very open to suggestions!

Post by Nicholas Marriott
If the cell is default fg/bg, won't this add \e[0m\e[39m\[49m? The two
colour changes are unnecessary.

Nicholas Marriott

2014-09-17 21:11:30 UTC

Permalink

Or this which actually builds...

diff --git a/grid.c b/grid.c
index a84a58b..8f5a669 100644
--- a/grid.c
+++ b/grid.c
@@ -75,7 +75,7 @@ void grid_reflow_split(struct grid *, u_int *, struct grid_line *, u_int,
void grid_reflow_move(struct grid *, u_int *, struct grid_line *);
size_t grid_string_cells_fg(const struct grid_cell *, int *);
size_t grid_string_cells_bg(const struct grid_cell *, int *);
-void grid_string_cells_code(const struct grid_cell *,
+void grid_string_cells_code(struct grid_cell *,
const struct grid_cell *, char *, size_t, int);

/* Create a new grid. */
@@ -477,7 +477,7 @@ grid_string_cells_bg(const struct grid_cell *gc, int *values)
* bytes.
*/
void
-grid_string_cells_code(const struct grid_cell *lastgc,
+grid_string_cells_code(struct grid_cell *lastgc,
const struct grid_cell *gc, char *buf, size_t len, int escape_c0)
{
int oldc[16], newc[16], s[32];
@@ -505,6 +505,8 @@ grid_string_cells_code(const struct grid_cell *lastgc,
if (!(attr & attrs[i].mask) && (lastattr & attrs[i].mask)) {
s[n++] = 0;
lastattr &= GRID_ATTR_CHARSET;
+ lastgc->fg = lastgc->bg = 7;
+ lastgc->flags &= ~(GRID_FLAG_FG256|GRID_FLAG_BG256);
break;
}
}

Post by Nicholas Marriott
Does this work? If we do this can probably drop lastattr too and maybe
move the memcpy inside the _code func.
diff --git a/grid.c b/grid.c
index a84a58b..12ec643 100644
--- a/grid.c
+++ b/grid.c
@@ -477,7 +477,7 @@ grid_string_cells_bg(const struct grid_cell *gc, int *values)
* bytes.
*/
void
-grid_string_cells_code(const struct grid_cell *lastgc,
+grid_string_cells_code(struct grid_cell *lastgc,
const struct grid_cell *gc, char *buf, size_t len, int escape_c0)
{
int oldc[16], newc[16], s[32];
@@ -505,6 +505,8 @@ grid_string_cells_code(const struct grid_cell *lastgc,
if (!(attr & attrs[i].mask) && (lastattr & attrs[i].mask)) {
s[n++] = 0;
lastattr &= GRID_ATTR_CHARSET;
+ lastgc->fg = lastgc->bg = 7;
+ lastgc->flags &= ~(GRID_FLAG_FG256|GRID_FLAG_BG256);
break;
}
}

Post by Nicholas Marriott
If the cell is default fg/bg, won't this add \e[0m\e[39m\[49m? The two
colour changes are unnecessary.

Matthew Darby

2014-09-17 18:31:20 UTC

Permalink

That's correct. In fact, after a test, it adds \e[0;39;49m if the previous cell had no colour. There's two issues I can see:

1) If the previous cell's colour was default, it will add the above default colours superfluously.
2) If the current cell colour changes in addition to the attribute being removed, you get a code like \0;39;49;31m. This is a superfluous colour change.

I'm not sure how to proceed without restructuring the function (it's well suited to the structure it already has). I'm very open to suggestions!

Post by Nicholas Marriott
If the cell is default fg/bg, won't this add \e[0m\e[39m\[49m? The two
colour changes are unnecessary.