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

Refactor the corner view visibility #375

Merged
merged 1 commit into from
Jun 30, 2021

Conversation

MGaetan89
Copy link
Contributor

@MGaetan89 MGaetan89 commented Apr 9, 2021

I've refactored the AbstractTableAdapter#setAllItems method and added some tests for the corner view logic (creation and visibility).

This change fixes an issue where the corner view could remain visible when toggling TableView#mShowCornerView from true to false.

@Zardozz
Copy link

Zardozz commented Apr 12, 2021

There is two problems with this pull request.

  1. AbstractTableAdapterTest class is really in the wrong directory, it should be located under the "test" directory (have a package name of "com.evrencoskun.tableview.test"), this might not currently affect the tests but it is not the correct structure for this type of test.

The reason it should be under the "test" directory and package name of "com.evrencoskun.tableview.test" is that androidTest auto generates the androidTest resources under the "com.evrencoskun.tableview.test" thus making it hard to use them if not in the right package (though this class does not use them at the moment so it works).

  1. The change breaks the fix for Corner View not shown #308 for which you fixed by altering the tests by inverting there assertions in the CornerViewTest

setShowCornerView method is to set a Flag to change the behaviour of setAllItems without breaking the current behaviour

testColumnHeadersOnlyTableShowCorner and testColumnHeadersOnlyTableShowCornerResetEmptyTable and testColumnHeadersOnlyTableShowCornerResetNonEmptyTable Tests
set the cornerView to be shown even if they have no row data or row headers but do have column header data.

These tests in CornerViewTest need to pass without any changes to the tests.

The idea of the tests

// Set the option to show corner view when there is not row data
       tableView.setShowCornerView(true);

is the cornerView is created (i.e. Not Null) Assert.assertNotNull(cornerViewReset);

even when there are no rows

// Only want column headers
        SimpleData simpleData = new SimpleData(5, 0);

basically if setShowCornerView is true then the test in your code of boolean hasRowHeaders = rowHeaderItems != null && !rowHeaderItems.isEmpty(); should be ignored (or always return true)

@MGaetan89 MGaetan89 force-pushed the corner_view_visibility branch from 1d1933c to 89ab174 Compare May 2, 2021 16:13
@MGaetan89
Copy link
Contributor Author

Thanks for having a look at my PR. Indeed my approach was originally wrong.
I've updated my PR, so now the original tests remain unchanged, and it still fixes a couple of corner view visibility.

@MGaetan89
Copy link
Contributor Author

@evrencoskun any chance to have a look at this?

@evrencoskun evrencoskun merged commit f46ef65 into evrencoskun:master Jun 30, 2021
@MGaetan89 MGaetan89 deleted the corner_view_visibility branch June 30, 2021 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants