UITester support for TableEditor

[aaronayres35]

After looking though these changes again, I felt like this branch (same as #1706) really was a good starting point. I went ahead and rebased on top of master, and will clean up this PR as needed with the intention that this ultimately will be merged.

I'm struggling to remember why I hadn't opened a PR for this previously. I suspect it was that the wx implementation is incomplete. Qt I believe is fully functional (?)
wx has allowed failures on CI, and many TableEditor tests already fail. I remember this making adding functionality difficult because even if I thought i had wx support working tests may have still failed for other reasons...

EDIT: I am going to remove any wx changes in this PR and defer adding wx support for TableEditor to a later PR
Checklist

• Add a news fragment if this PR is news-worthy for end users. (see docs/releases/README.rst)

[aaronayres35]

is data the right thing here for displayed text? (look at Qt docs for QAbstractItemModel)

[corranwebster]

Yes, data with DisplayRole should provide the text to display.

[aaronayres35]

On master the following TableEditor Tests fail on wx:

test_filtered_table_editor
test_table_editor
test_table_editor_select_cell
test_table_editor_select_cell_index
test_table_editor_select_cell_indices
test_table_editor_select_cells
test_table_editor_select_column
test_table_editor_select_column_index
test_table_editor_select_column_indices
test_table_editor_select_columns
test_table_editor_select_row
test_table_editor_select_row_index
test_table_editor_select_row_indices
test_table_editor_select_rows

(Note this is effectively every test - I run python -m unittest traitsui/tests/editors/test_table_editor.py -v in an env created with python etstool.py install --toolkit=wx --editable and I see 16 tests run with 14 errors and 2 skipped tests.

Given this, I intend to skip all these tests on wx. Note this is now a part of #1533. Even further, I intend to rewrite the tests to use the new UI Tester support on Qt. Thus, even if the TableEditor issues are fixed, we will need to follow through with the UI Tester support to simply stop skipping these tests on wx. Either that or use this PR as a ref and revert the tests back to their old non UITester using state. I am doing this based off the following section of the developer guide in the traitsui docs: https://docs.enthought.com/traitsui/traitsui_dev_guide/testing.html#where-are-the-tests

[aaronayres35]

It would be nice to have shift click functionality to do this sort of thing with the tester...

[aaronayres35]

I've just added a SelectedIndices query class.
For the current implementation, for TableEditor, Selected and SelectedIndices effectively just serve as proxies to the selected and selected_indices traits on the table editor (see code above.)
In theory it may be better for these to dive down to the Qt level (?)

[aaronayres35]

This is one of the weird things with UITester support though IMO. We want to test traitsui editors are working correctly.
So having tests be down at qt level is really what we want. Ie I do something in traitsUI and we verify that does what we expect down in qt land.
However, say I were implementing the handler for the TableEditor to do a SelectedIndices query. I would end up basically rewriting:

traitsui/traitsui/qt4/table_editor.py

Lines 626 to 643 in 347f0b5

	@cached_property
	def _get_selected_indices(self):
	"""Gets the row,column indices which match the selected trait"""
	selection_items = self.table_view.selectionModel().selection()
	indices = self.model.mapSelectionToSource(selection_items).indexes()
	if self.factory.selection_mode.startswith("row"):
	indices = sorted(set(index.row() for index in indices))
	elif self.factory.selection_mode.startswith("column"):
	indices = sorted(set(index.column() for index in indices))
	else:
	indices = [(index.row(), index.column()) for index in indices]
	if self.factory.selection_mode in {"rows", "columns", "cells"}:
	return indices
	elif len(indices) > 0:
	return indices[0]
	else:
	return -1

It creates sort of a weird chasing your own tail scenario 🤔

For downstream users they just want to see that when they run their code, ____ is selected. For which this (just using traits on the editor) is perfectly fine.

[corranwebster]

UITester strikes me as being useful for two things:

• testing that TraitsUI editors are integrating correctly with the toolkit
• as a framework for integration tests for TraitsUI apps

For unit tests of applications you can usually get away with tests at the traits level (does changing this trait in a UI change the corresponding thing in the model? or vice-versa) where you don't really care/can trust that the UI then does the right thing.

[aaronayres35]

I believe once this PR gets the green light, a lot of the code can be pulled to add support for TabularEditor which also ends up wrapping a QTableView. Likewise that will probably get only qt support first as wx tests are currently all failing.

[rahulporuri]

What's the rationality behind this API? Why not just return None if nothing is selected? Also, why not always return a list - which might contain one or more elements depending on the size of the selection.

[aaronayres35]

TBH it was because I am currently just mirroring the implementation of selected_indices in the TableEditor itself. See comment: #1707 (comment) and code:

traitsui/traitsui/qt4/table_editor.py

Lines 627 to 643 in 762f66b

	def _get_selected_indices(self):
	"""Gets the row,column indices which match the selected trait"""
	selection_items = self.table_view.selectionModel().selection()
	indices = self.model.mapSelectionToSource(selection_items).indexes()
	if self.factory.selection_mode.startswith("row"):
	indices = sorted(set(index.row() for index in indices))
	elif self.factory.selection_mode.startswith("column"):
	indices = sorted(set(index.column() for index in indices))
	else:
	indices = [(index.row(), index.column()) for index in indices]
	if self.factory.selection_mode in {"rows", "columns", "cells"}:
	return indices
	elif len(indices) > 0:
	return indices[0]
	else:
	return -1

I agree this is confusing though and lazy. I can at the very least wrap this with a special handler to give a more consistent return type. The SelectedIndices query class has no reason to be at all tied with a specific Editor (in fact it definitely shouldn't be).

Nonetheless I do still have the question of whether or not I should be reimplementing/rewriting the above code in the UITester TableEditor support code as a specifc handler for the SelectedIndices query. As mentioned in my comment I think that is a little unnecessary / we have this strange chase your own tail scenario... 🤔

[aaronayres35]

I need to carefully think about the best api for Selected and SelectedIndices before pushing this through 🤔

It i tricky because with a TableEditor, one could have rows, columns, cells, etc. selected. The api needs to accommodate this use case but also be general and not overly complex for other Editors to use

[aaronayres35]

I am going to update Selected so that it also always returns a list (either a singleton list for one selection, list of all objects selected, or an empty list if nothing is selected).
It is also a bit strange with SelectedText as in that case it doesn't really make sense to want to make that query if multiple objects are selected (at least I can't imagine a use case). Hence for that I think I will not update it to enforce that it returns a list.

[rahulporuri]

under what circumstances do we return an integer vs a tuple.

[aaronayres35]

Currently, it would be an int if selecting entire rows or columns, whereas it would be a tuple if selecting specific cells. However, if used with say a ListEditor, InstanceEditor, or EnumEditor, a selected index would likely be an integer. Currently those implementations do not exist (in current tests querying what is selected we just use SelectedText rather than inspecting the object itself that is selected). These implementations can be added in the future though.
I've updated the doc string to try to make this more clear. ~(will push commit shortly)~Edit: done

[aaronayres35]

I find the list here slightly clunkier than before... However, i think being consistent with the api of SelectedIndices is more important.

[corranwebster]

Looks good at a high level. From examples of usage, looks like it is reasonably user-friendly.