Skip to content
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

ROW: clean up in preparation to hide CharRow & AttrRow #8446

Merged
17 commits merged into from
Jan 20, 2021

Conversation

DHowett
Copy link
Member

@DHowett DHowett commented Nov 30, 2020

Moving things out of CharRow into ROW helps us hide it as an implementation detail.
This is part one of many.

CharRow: Hide ClearCell, use ROW::ClearColumn

CharRow: Hide GetText, use ROW::GetText

CharRowBaseTests: remove dead file (never used!)

CharRow: Move DoubleBytePadded into ROW

CharRow: Move WrapForced into ROW

Char/AttrRow: Hide Reset, use ROW::Reset

Remove RowCellIterator (dead code)

RCI was unused; it was replaced by TextBufferCellIterator shortly after its creation

Move AttrRowTests to ut_textbuffer from ut_host

It had no reliance on the host.

@zadjii-msft
Copy link
Member

@msftbot make sure @miniksa signs off on this

@ghost ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 1, 2020
@ghost
Copy link

ghost commented Dec 1, 2020

Hello @zadjii-msft!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

  • I'll only merge this pull request if it's approved by @miniksa

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

@DHowett DHowett removed the AutoMerge Marked for automatic merge by the bot when requirements are met label Dec 1, 2020
@@ -36,6 +36,12 @@ class ROW final

size_t size() const noexcept;

void SetWrapForced(const bool wrap) noexcept;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could just move the getters/setters up here per the other PR and chat recently about inline potential.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no reason not to

@DHowett DHowett marked this pull request as ready for review January 11, 2021 19:02
@DHowett DHowett changed the title ROW/CharRow: clean up in preparation to hide CharRow ROW: clean up in preparation to hide CharRow & AttrRow Jan 11, 2021
@DHowett DHowett mentioned this pull request Jan 12, 2021
15 tasks
@DHowett
Copy link
Member Author

DHowett commented Jan 12, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Member

@zadjii-msft zadjii-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems straightforward

@zadjii-msft zadjii-msft removed their assignment Jan 15, 2021
@zadjii-msft zadjii-msft added Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. Product-Conhost For issues in the Console codebase Needs-Second It's a PR that needs another sign-off labels Jan 15, 2021
ghost pushed a commit that referenced this pull request Jan 18, 2021
This is by no means comprehensive. It will be unmarked as draft when it
is more comprehensive.

This pull request adds some tests for resizing a TextBuffer and
reflowing its contents. Each test takes the form of an initial state and
a number of buffers of different sizes. The initial state is used to
seed the first TextBuffer, and the subsequent buffers are only used to
compare.

I manually reimplemented some of the DBCS logic to ensure that the
buffers contain _exactly_ what they're supposed to. I know this is
non-ideal. After some of the CharRow changes in #8446 land, this will
need to be updated.

There's a cool bit of TAEF gore in here: the IDataSource. An IDataSource
allows us to programmatically return test cases. It's a code-only
version of its support for parameterized tests of the form `Data:x = {0,
1, 2}` . 

The only downsides are...

1. It looks like COM (it is not using COM under the hood, just the COM
   ABI)
2. Property values must be returned as strings.

To best support rich test types, I used IDataSource to produce _a lit of
array indices_ and nothing more. The test is run once for array member,
and it is the test's responsibility to look up the object to which that
index refers.

Works great though! Each reflow test is its own unit, and a failure in
an earlier reflow test will not tank a later one.
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookin' good. Can't wait until the next step!

@DHowett DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jan 20, 2021
@ghost
Copy link

ghost commented Jan 20, 2021

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit e7592ec into main Jan 20, 2021
@ghost ghost deleted the dev/duhowett/charrow_clean_1 branch January 20, 2021 21:16
mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
This is by no means comprehensive. It will be unmarked as draft when it
is more comprehensive.

This pull request adds some tests for resizing a TextBuffer and
reflowing its contents. Each test takes the form of an initial state and
a number of buffers of different sizes. The initial state is used to
seed the first TextBuffer, and the subsequent buffers are only used to
compare.

I manually reimplemented some of the DBCS logic to ensure that the
buffers contain _exactly_ what they're supposed to. I know this is
non-ideal. After some of the CharRow changes in microsoft#8446 land, this will
need to be updated.

There's a cool bit of TAEF gore in here: the IDataSource. An IDataSource
allows us to programmatically return test cases. It's a code-only
version of its support for parameterized tests of the form `Data:x = {0,
1, 2}` . 

The only downsides are...

1. It looks like COM (it is not using COM under the hood, just the COM
   ABI)
2. Property values must be returned as strings.

To best support rich test types, I used IDataSource to produce _a lit of
array indices_ and nothing more. The test is run once for array member,
and it is the test's responsibility to look up the object to which that
index refers.

Works great though! Each reflow test is its own unit, and a failure in
an earlier reflow test will not tank a later one.
mpela81 pushed a commit to mpela81/terminal that referenced this pull request Jan 28, 2021
Moving things out of CharRow into ROW helps us hide it as an implementation detail.
This is part one of many.

### CharRow: Hide ClearCell, use ROW::ClearColumn

### CharRow: Hide GetText, use ROW::GetText

### CharRowBaseTests: remove dead file (never used!)

### CharRow: Move DoubleBytePadded into ROW

### CharRow: Move WrapForced into ROW

### Char/AttrRow: Hide Reset, use ROW::Reset

### Remove RowCellIterator (dead code)

RCI was unused; it was replaced by TextBufferCellIterator shortly after its creation

### Move AttrRowTests to ut_textbuffer from ut_host

It had no reliance on the host.
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-CodeHealth Issues related to code cleanliness, linting, rules, warnings, errors, static analysis, etc. AutoMerge Marked for automatic merge by the bot when requirements are met Needs-Second It's a PR that needs another sign-off Product-Conhost For issues in the Console codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants