It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

• It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

• Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
• The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
• The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

@Hixie do I need any tests for a change so straightforward?

cc @willlarche

@googlebot I signed it!

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

This appears to be a nice fix for what was probably a typo; thanks! It will definitely need a test: we don't want this issue to crop up again.

Also: there are some presubmit DataTable test failures.

@HansMuller I've never done a pull request on a public repository so I'm unsure what the next steps are to get this approved.

You’re doing a great job!

Find a test file that’s already doing tests on these classes and add one specifically to check for your case that is being fixed.

What I want to test:
If you create a DataTable with DataColumns that have no onSort parameter, the text in the cells will not be centered with the columns and the text of the columns will be slightly misaligned because flutter thinks it needs to have space for the arrow associated with a column when the user taps on the column to sort it.

My fix corrects this (as shown below) and automatically centers the content in the column but I have not the slightest idea as to how to write a test for this.

I know the test file is flutter/packages/flutter/test/material/data_table_test.dart and the test should be in the existing test

  testWidgets('DataTable column onSort test', (WidgetTester tester) async {
    await tester.pumpWidget(
      MaterialApp(
        home: Material(
          child: DataTable(
            columns: const <DataColumn>[
              DataColumn(
                label: Text('Dessert'),
              ),
            ],
            rows: const <DataRow>[
              DataRow(
                cells: <DataCell>[
                  DataCell(
                    Text('Lollipop'), // wraps
                  ),
                ],
              ),
            ],
          ),
        ),
      ),
    );
    await tester.tap(find.text('Dessert'));
    await tester.pump();
    expect(tester.takeException(), isNull);
  });

I somehow need to test the _buildHeadingCell method of DataTable to make sure that it is possible for a null value to be passed into the onSort parameter. I am unsure how to test a private method in a test file.

Also, how is this test

  testWidgets('DataTable column onSort test', (WidgetTester tester) async {
    await tester.pumpWidget(
      MaterialApp(
        home: Material(
          child: DataTable(
            columns: const <DataColumn>[
              DataColumn(
                label: Text('Dessert'),
              ),
            ],
            rows: const <DataRow>[
              DataRow(
                cells: <DataCell>[
                  DataCell(
                    Text('Lollipop'), // wraps
                  ),
                ],
              ),
            ],
          ),
        ),
      ),
    );
    await tester.tap(find.text('Dessert'));
    await tester.pump();
    expect(tester.takeException(), isNull);
  });

testing onSort? There isn't even an onSort parameter passed so what exactly is this testing for?

Note: another bad ternary comparison argument is also present in this snipped of code in data_table.dart but I am unaware of its side effects.

      for (DataRow row in rows) {
        tableRows[rowIndex].children[0] = _buildCheckbox(
          color: theme.accentColor,
          checked: row.selected,
          onRowTap: () => row.onSelectChanged != null ? row.onSelectChanged(!row.selected) : null ,
          onCheckboxChanged: row.onSelectChanged,
        );
        rowIndex += 1;
      }

@HansMuller I'm stuck here and want to get this fix implemented as soon as possible! I'm struggling on the new tests I'm supposed to create (even the existing tests don't make sense) and I need some help.

@HansMuller I believe this pr is ready

@andermoran the PR still doesn't include a test. I'm afraid I don't have the time to help craft one at the moment, sorry about that.

Despite weeks of looking at several resources to figure out how to write flutter tests, I still can't get the hang of it. Will this bug fix ever be merged?

Might https://flutter.dev/docs/cookbook/testing/widget/introduction help?

@Hixie I’m not sure if you’re trolling me or not but I just said I’ve spent weeks looking at several resources to write flutter tests but have been unable to. Yes, I have looked at the intro page as well as many other resources. I’ve really tried my hardest to write these tests because I want this merge to happen badly but I’ve been unable to. I’ve looked at all the resources on the web about flutter testing as well as looking through flutter tests in flutter’s source code to how it was done there. At this point I’ve given up. I wish someone could at least write one test for this PR that I need so that I can work off that.

If you could file a bug that describes what you tried and how the documentation failed you, that would be great. I'm sad to hear that the current documentation was inadequate to the task.

Any update?

@andermoran I looked into it since this is indeed an interesting PR and exposes some problems of our code coverage. I came up with a unit test as below:

  testWidgets('DataTable column label position - no sort', (WidgetTester tester) async {
    Widget buildTable({ int sortColumnIndex, bool sortAscending = true }) {
      return DataTable(
        sortColumnIndex: sortColumnIndex,
        sortAscending: sortAscending,
        columns: <DataColumn>[
          const DataColumn(
            label: Expanded(child: Center(child: Text('Name'))),
            tooltip: 'Name',
          ),
          DataColumn(
            label: const Expanded(child: Center(child: Text('Calories'))),
            tooltip: 'Calories',
            numeric: true,
            onSort: (int columnIndex, bool ascending) {},
          ),
        ],
        rows: const <DataRow>[
          DataRow(
            cells: <DataCell>[
              DataCell(Text('A long desert name')),
              DataCell(Text('A lot of calories')),
            ],
          ),
        ]
      );
    }

    await tester.pumpWidget(MaterialApp(
      home: Material(child: buildTable()),
    ));

    final Finder nameText = find.text('Name');
    expect(nameText, findsOneWidget);
    final Finder nameRow = find.ancestor(of: find.text('Name'), matching: find.byType(Row)).first;
    expect(tester.getCenter(nameText), equals(tester.getCenter(nameRow)));
  });

This test finds the text and its immediately wrapping row, and see if they have the same center, indicating that no other padding is added. This test breaks on master and passes on your PR.

However, I did find a bug (unrelated to this PR) that causes the header to disappear on mouse move, which I've filed at #51152. This bug seems to occur only when the text is not wrapped in the InkWell (which is only found out thanks to your PR), so until that issue is fixed, you'll need to wrap the header in InkWell unconditionally.

Hopefully this is a good starting point for you!

I fear @andermoran gave up due its frustration. One of team members from flutter should finalize the pull request. It has been already more than 5 months.

I fear @andermoran gave up due its frustration. One of team members from flutter should finalize the pull request. It has been already more than 5 months.

Can confirm. Was frustrating to create a test. Did give up.

@dkwingsmt If the original bug was alignment issues we should probably have the test toggle the sortability (have onSort be null once, then non-null) and verify that the alignment is correct in both cases.

@Hixie I can test that the alignment with onSort is consistent, but it's never gonna be at the center. (Although I can test that the row is centered in the cell, it's not useful since the row is supposed to fill up the cell).

I would like to have datacolumn label to be still centered even if it is sorted (up or down arrow appears on the right of the centered label). It can be annoying to see centered label moves when it is sorted.

@deadsoul44 It's a good proposal, but let me explain the obstacle. The current structure can roughly be presented as:

tableCell = Container(
  child: Row(
    children: [
      label,  // The `label` argument of `DataColumn`
      if (onSort != null) sortingArrow,
    ],
  ),
);

Imagine the cell is 100px wide, the arrow being 10px wide and the text 40 px wide. You would expect that:

1. If label: Text('Cities') the text is at the left edge of the cell, i.e. [0, 40]
2. If label: Center(child: Text('Cities')) the text is at the center of the cell, [30, 70]

• This wouldn't directly work because Row prefers the minimum size, but you can fix it by wrapping it with an Expand.

3. If label: Align(alignment: Alignment.centerRight), child: Text('Cities')) the text is at the right edge of the possible space, i.e. [50, 90]. (You'll also need Expand).

Apparently No. 2 wouldn't center the text in the cell with the current implementation, because the text will be at the center of the remaining space, which is [25, 65].
Alright, what if we add a mirroring padding at the left, so that since we know the arrow is 10px wide, make the text centered within [10, 90], which is [30, 70]? Well then No. 1 would place the text at [10, 50] instead of the desired [0, 40].

The root cause is that DataTable have no information of its child label, and can't decide what to align its child to. I'd like to hear if you have any suggestions!

I submitted a PR that continues this one in #51667

I understand that it is hard to align with current structure. Another option can be to add a "labelAlignment" argument. Depending on developer choice, the label can be right, center or left aligned. Currently, it is already assumed as right aligned if column is numeric and left aligned otherwise.

Pending reviewer for two weeks...

This PR has been continued by #51667, which has been merged.

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.