Refactor handling of model/view framework

ubruhin commented 5 years ago

Since a few months, I experienced sporadic errors in the ComboBox.model_items() on macOS builds on Travis-CI, which has become very cumbersome as this made CI fail very often in my project. See here for details and investigation: https://github.com/LibrePCB/LibrePCB/issues/364

The root cause of these failures is the method ComboBox.model_items() which looks like that:

https://github.com/parkouss/funq/blob/3e60dbbc78e9816e41ac37e5a94876e61ad8808a/client/funq/models.py#L784-L796

It looks very hacky, with the click() and accessing private members of the QComboBox ;) So not very surprising that it doesn't work reliable...

When I started to fix this, I realized that the handling of the model/view framework in funq has an architectural issue: The ModelItem objects are always related to the view, not to the model. But in Qt's model/view framework, items are part of the model, and don't know anything about the view. Thus I refactored it to fix this architectural issue.

Attention: This is a breaking change, as methods needed to be moved to different classes! How should we handle breaking changes? Should I increment the version number to 2.0.0?

Here the actual changes I made:

Player: Add action "model" to get the model of a view widget

Allows to get the QAbstractItemModel of a QComboBox or QAbstractItemView subclass.

Client: Move actions from ModelItem to AbstractItemView

Actions (e.g. select, edit, click) are performed on the item view, not on the item itself. The item represents only the data contained in the model, while the view is responsible for user interaction. To fix the current architectural issue, following methods are moved and renamed:

ModelItem._action() -> AbstractItemView._item_action()
ModelItem.select() -> AbstractItemView.select_item()
ModelItem.edit() -> AbstractItemView.edit_item()
ModelItem.click() -> AbstractItemView.click_item()
ModelItem.dclick() -> AbstractItemView.dclick_item()

Make model items related to models, not views

Protocol: Rename property "viewid" of model items to "modelid" and let it point to the item's model instead of its view (items are part of a model, not of a view).
Client: Add AbstractItemModel (represents QAbstractItemModel), with the method items() to get all items of the model.
Client: Replace model_items() methods of AbstractItemView and ComboBox with model(), which returns the model of the view. The items can then be fetched using the returned AbstractItemModel object.

Add functional tests for QComboBox and QTableView

Note: Some of these tests are still failing on Ci - I'm working on that, thus the "WIP" in the PR title...

Update documentation about the model/view framework

rafaeldelucena commented 5 years ago

Attention: This is a breaking change, as methods needed to be moved to different classes! How should we handle breaking changes? Should I increment the version number to 2.0.0?

We can merge this into master and update the Changelog to an Unreleased version (same with your other PR) after that we can create a new version and for me 2.0.0 will be a good choice, since the breaking changes.

ubruhin commented 5 years ago

We can merge this into master and update the Changelog to an Unreleased version (same with your other PR) after that we can create a new version and for me 2.0.0 will be a good choice, since the breaking changes.

:+1:

Btw, I added a commit with a workaround to fix test failures on Python 3 and opened #58 to keep in mind that it should be fixed properly some time.

Now it seems that the only issue is something related to Qt 4 as all other CI jobs are now successful.

And argghh, the Windows build throws these errors, which are related to #55 but I'm not able to reproduce them :sob:

https://ci.appveyor.com/project/parkouss/funq/builds/21360912:

======================================================================
FAIL: test_click.py:TestClick.test_click_header_R1_by_name
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\projects\funq\client\funq\testcase.py", line 190, in wrapper
    return func(self, *args, **kwargs)
  File "C:\projects\funq\tests-functionnal\test_click.py", line 79, in test_click_header
    self.assertEquals(self.get_status_text(), orientation + " Header clicked: " + str(result_index))
AssertionError: u'' != 'H Header clicked: 0'
-------------------- >> begin captured logging << --------------------
funq.client: INFO: The tested application will be launched in the directory 'C:\\projects\\funq\\tests-functionnal\\.' with the command ['C:\\Python27\\Scripts\\funq.exe', '--port', '1052', 'C:\\projects\\funq\\tests-functionnal\\./funq-test-app/funq-test-app']
funq.client: INFO: Launching tested application [2740].
--------------------- >> end captured logging << ---------------------
======================================================================
FAIL: test_click.py:TestClick.test_key_click_sometext
----------------------------------------------------------------------
Traceback (most recent call last):
  File "c:\projects\funq\client\funq\testcase.py", line 190, in wrapper
    return func(self, *args, **kwargs)
  File "C:\projects\funq\tests-functionnal\test_click.py", line 70, in test_key_click
    self.assertEquals(self.get_status_text(), text)
AssertionError: u'' != 'Hello this is me !'
-------------------- >> begin captured logging << --------------------
funq.client: INFO: The tested application will be launched in the directory 'C:\\projects\\funq\\tests-functionnal\\.' with the command ['C:\\Python27\\Scripts\\funq.exe', '--port', '1052', 'C:\\projects\\funq\\tests-functionnal\\./funq-test-app/funq-test-app']
funq.client: INFO: Launching tested application [2532].
--------------------- >> end captured logging << ---------------------
----------------------------------------------------------------------

rafaeldelucena commented 5 years ago

Looking more carefully for the code, I think those changes makes sense, also matching Qt model/view design makes easier to new people starting use funq.

ubruhin commented 5 years ago

OK I found the issue with Qt4, I used a QComboBox signal in the test app which didn't exist in Qt4 -> fixed it.

In addition, I updated the changelog.

If CI is now successful, I think this is ready to merge from my side :)

ubruhin commented 5 years ago

...and one more time a failure on AppVeyor, it's so frustrating :sob: Could maybe someone restart the build to see if it was a sporadic failure?

ubruhin commented 5 years ago

Rebased to master.

parkouss / funq