feat: Support for BorderChar on Table level

[assemmarwan]

Add support for BorderChar on Table Level as mentioned in the issue: #239

ToDo List

• Add the feature to the codebase
• Add basic tests for the new feature
• Update documentation (README)

[zhiburt]

Hi @assemmarwan

Thanks for contribution,

I think you misunderstood the issue a bit.
(As well as I did with the example in the issue :))

The idea is that you target a specific line in a table being not dependent on row/column.

For example you want to set a char at 3rd horizontal line, it might be a 3rd row (with offset 0) or it may be a 2nd (with offset 1) or even 1st row (with offset 3).

---

It'd need to create a new public function/struct like

BorderChar::horizontal_exact(3, ':', Offset::End(0))
// or
ExactLine::horizontal(3, BorderChar::horizontal(':', Offset::End(0)))

I'd go with the 1st interface as it less general, though ExactLine sounds interesting for some cases but not exactly here.

---

The implementation is relatively simple as well.
You'd need to calculate the needed exact row/column based on relative offset and index.

1. iterate over rows/columns
2. Use Table::get_width_ctrl, Table::get_height_ctrl to get row/column height/width
3. Check if you belong to the corresponding row/column
4. If you do then iterate over row/column to find a cell by Offset in a similar way.

[assemmarwan]

Hi @assemmarwan

Thanks for contribution,

I think you misunderstood the issue a bit. (As well as I did with the example in the issue :))

The idea is that you target a specific line in a table being not dependent on row/column.

For example you want to set a char at 3rd horizontal line, it might be a 3rd row (with offset 0) or it may be a 2nd (with offset 1) or even 1st row (with offset 3).

It'd need to create a new public function/struct like

BorderChar::horizontal_exact(3, ':', Offset::End(0))
// or
ExactLine::horizontal(3, BorderChar::horizontal(':', Offset::End(0)))

I'd go with the 1st interface as it less general, though ExactLine sounds interesting for some cases but not exactly here.

The implementation is relatively simple as well. You'd need to calculate the needed exact row/column based on relative offset and index.

1. iterate over rows/columns
2. Use Table::get_width_ctrl, Table::get_height_ctrl to get row/column height/width
3. Check if you belong to the corresponding row/column
4. If you do then iterate over row/column to find a cell by Offset in a similar way.

Ohhh, I think I got you.

I agree with you to use BorderChar::horizontal_exact() and BorderChar::vertical_exact().

If you don't mind share a sample table output when BorderChar::horizontal_exact(3, ':', Offset::End(0)) is used to test against it. Appreciate it 😃

[assemmarwan]

@zhiburt Do I keep the misunderstood feature 😅 ? Or should I discard it?

[zhiburt]

Do I keep the misunderstood feature 😅 ? Or should I discard it?

I guess it's better be discarded as we can do exactly this by.

Modify::new(Segment::all()).with(BorderChar ... )

If you don't mind share a sample table output when BorderChar::horizontal_exact(3, ':', Offset::End(0)) is used to test against it.

original table

+------+----------------+---------------+
| name | designed_by    | invented_year |
+------+----------------+---------------+
| C    | Dennis Ritchie | 1972          |
+------+----------------+---------------+

BorderChar::horizontal_exact(2, '$', Offset::Start(0))

+------+----------------+---------------+
| name | designed_by    | invented_year |
$------+----------------+---------------+
| C    | Dennis Ritchie | 1972          |
+------+----------------+---------------+

BorderChar::horizontal_exact(2, '$', Offset::Start(10))

+------+----------------+---------------+
| name | designed_by    | invented_year |
+------+--$-------------+---------------+
| C    | Dennis Ritchie | 1972          |
+------+----------------+---------------+

BorderChar::horizontal_exact(3, '$', Offset::Start(2))

+------+----------------+---------------+
| name | designed_by    | invented_year |
+------+----------------+---------------+
| C    | Dennis Ritchie $ 1972          |
+------+----------------+---------------+

---

To be honest I got stuck thinking about:

1. whether we shall allow indexing of intersections here (see 1st example)
2. whether we shall index only horizontal lines or all lines (see 3rd example)

Let me know what you think.

[assemmarwan]

Do I keep the misunderstood feature 😅 ? Or should I discard it?

I guess it's better be discarded as we can do exactly this by.

Modify::new(Segment::all()).with(BorderChar ... )

Nice. I will revert my changes 👍🏻

If you don't mind share a sample table output when BorderChar::horizontal_exact(3, ':', Offset::End(0)) is used to test against it.

original table

+------+----------------+---------------+
| name | designed_by    | invented_year |
+------+----------------+---------------+
| C    | Dennis Ritchie | 1972          |
+------+----------------+---------------+

BorderChar::horizontal_exact(2, '$', Offset::Start(0))

+------+----------------+---------------+
| name | designed_by    | invented_year |
$------+----------------+---------------+
| C    | Dennis Ritchie | 1972          |
+------+----------------+---------------+

BorderChar::horizontal_exact(2, '$', Offset::Start(10))

+------+----------------+---------------+
| name | designed_by    | invented_year |
+------+--$-------------+---------------+
| C    | Dennis Ritchie | 1972          |
+------+----------------+---------------+

BorderChar::horizontal_exact(3, '$', Offset::Start(2))

+------+----------------+---------------+
| name | designed_by    | invented_year |
+------+----------------+---------------+
| C    | Dennis Ritchie $ 1972          |
+------+----------------+---------------+

Thanks for the explanation 😃

To be honest I got stuck thinking about:

1. whether we shall allow indexing of intersections here (see 1st example)

For the sake of consistency with the current implementation, I would say not to allow it.

But if we were to allow it, how would we achieve that with current implementation? Because all offsets start without the external border (intersection) always. If you would point me to how it's done please.

2. whether we shall index only horizontal lines or all lines (see 3rd example)

I think to remain consistent with the original BorderChar implementation, we index horizontal lines only. Example 3 can be achieved using vertical_exact function.

BorderChar::vertical_exact(2, '$', Offset::Start(3))

Also to achieve this:

+------+----------------+---------------+
| name | designed_by    | invented_year |
+------+--$-------------+---------------+
| C    | Dennis Ritchie | 1972          |
+------+----------------+---------------+

We can do it by:

BorderChar::horizontal_exact(1, '$', Offset::Start(10))

[zhiburt]

For the sake of consistency with the current implementation, I would say not to allow it.

I think to remain consistent with the original BorderChar implementation, we index horizontal lines only.

Then actually we could achieve it by Modify::new(Rows::new(your_row_goes_here)).with(BorderChar('$', Offset::Start(7)))
I mean it then makes no scenes right?

So I would try to do it my way?

But if we were to allow it, how would we achieve that with current implementation? Because all offsets start without the external border (intersection) always. If you would point me to how it's done please.

You'd need to call cfg.get_border() to get borders of a given cell and then modify the *_corner field of it after which call cfg.set_border() to update it's intersection symbol.

So yes I tend to believe it's possible to do.
The algorithm basically stays the same.

1. iterate over rows/columns
2. Use Table::get_width_ctrl, Table::get_height_ctrl to get row/column height/width
3. Check if you belong to the corresponding row/column
4. If you do then iterate over row/column to find a cell by Offset in a similar way.
   (Considering intersection as 1 step)
   1. while i < offset:
   2. if i == offset => then it's intersection symbol
   3. for i = i; i <= row_length; i++
   4. if i == offset => then it's offseted char

[assemmarwan]

Alright, so I'll work on it such that:

• All lines are allowed ( borders or non-borders)
• Intersection is allowed

[assemmarwan]

I have noticed that the functions Table::get_width_ctrl & Table::get_height_ctrl are not public 😬 .

tabled/src/table.rs

Lines 293 to 313 in 64d6808

	fn get_width_ctrl(&self) -> CachedEstimator<'_, WidthEstimator> {
	match &self.widths {
	Some(widths) => CachedEstimator::Cached(widths),
	None => {
	let mut w = WidthEstimator::default();
	w.estimate(&self.records, &self.cfg);
	CachedEstimator::Ctrl(w)
	}
	}
	}
	fn get_height_ctrl(&self) -> CachedEstimator<'_, HeightEstimator> {
	match &self.heights {
	Some(heights) => CachedEstimator::Cached(heights),
	None => {
	let mut w = HeightEstimator::default();
	w.estimate(&self.records, &self.cfg);
	CachedEstimator::Ctrl(w)
	}
	}
	}

What do you recommend that I use to determine the target position of the character?

[zhiburt]

I have noticed that the functions Table::get_width_ctrl & Table::get_height_ctrl are not public grimacing .

I've forgot about it. (it actually a subject to change in the future).
You can use crate::width::get_table_widths and crate::height::get_table_total_height2 in the meantime.

What do you recommend that I use to determine the target position of the character?

What do you mean?
I think this must work?

• iterate over rows/columns

• Use Table::get_width_ctrl, Table::get_height_ctrl to get row/column height/width

• Check if you belong to the corresponding row/column

• If you do then iterate over row/column to find a cell by Offset in a similar way.
  (Considering intersection as 1 step)

  1. while i < offset:
  2. if i == offset => then it's intersection symbol
  3. for i = i; i <= row_length; i++
  4. if i == offset => then it's offseted char

[assemmarwan]

@zhiburt Would you please assist about how to tell whether a cell has borders. This is assuming I have the border.

is this way valid?

let mut border = table.get_config_mut()
                .get_border((0, 0), (count_rows, count_cols));

let has_border = border.top.is_some();

[zhiburt]

is this way valid?

Yes

                  _______   < ---- top border
                 |       | 
                 |  cell |  < ---- right border
left border ---> |       |
                  _______   < ---- bottom border

PS: Maybe its worth to improve the documentation of papergrid a bit 😞

[zhiburt]

Accidentally I needed to experiment with this;

And I've got some work in progress patchwork. (surely it requires some tests and refactorings)

I'd say it's quite bad from performance perspective but it seems to work (I didn't test it well, just have a few runs).

impl<R> tabled::TableOption<R> for SetHorizontalChar
where
    R: Records,
{
    fn change(&mut self, table: &mut tabled::Table<R>) {
        let shape = table.shape();

        let is_last_line = self.line == (shape.0 * 2);
        let mut row = self.line;
        if is_last_line {
            row = self.line - 1;
        }

        let mut evaluator = tabled::papergrid::width::WidthEstimator::default();
        evaluator.estimate(table.get_records(), table.get_config());
        let widths: Vec<_> = evaluator.into();

        let mut i = 0;
        for column in 0..shape.1 {
            let has_vertical = table.get_config().has_vertical(column, shape.1);

            if has_vertical {
                if self.position == i {
                    let mut border = table.get_config().get_border((row, column), shape);
                    if is_last_line {
                        border.left_bottom_corner = Some(self.c1);
                    } else {
                        border.left_top_corner = Some(self.c1);
                    }

                    table.get_config_mut().set_border((row, column), border);

                    return;
                }

                i += 1;
            }

            let width = widths[column];

            if self.position < i + width {
                let offset = self.position + 1 - i;
                // let offset = width - offset;

                table.get_config_mut().override_horizontal_border(
                    (self.line, column),
                    self.c2,
                    tabled::papergrid::Offset::Begin(offset),
                );

                return;
            }

            i += width;
        }

        let has_vertical = table.get_config().has_vertical(shape.1, shape.1);
        if self.position == i && has_vertical {
            let mut border = table.get_config().get_border((row, shape.1), shape);
            if is_last_line {
                border.left_bottom_corner = Some(self.c1);
            } else {
                border.left_top_corner = Some(self.c1);
            }

            table.get_config_mut().set_border((row, shape.1), border);
        }
    }
}