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

[FIX] Reporting tabular in Data Table and Rank widgets #1573

Merged
merged 3 commits into from
Oct 7, 2016

Conversation

kernc
Copy link
Contributor

@kernc kernc commented Sep 16, 2016

QBrush. https://doc.qt.io/qt-4.8/qt.html#ItemDataRole-enum

Fixes report in Data Table.

@codecov-io
Copy link

codecov-io commented Sep 16, 2016

Current coverage is 88.66% (diff: 100%)

Merging #1573 into master will not change coverage

@@             master      #1573   diff @@
==========================================
  Files            78         78          
  Lines          8124       8124          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           7203       7203          
  Misses          921        921          
  Partials          0          0          

Sunburst

Powered by Codecov. Last update f539789...20c6e1e

@ales-erjavec
Copy link
Contributor

Ok. But I think you should still handle the case where the data(Qt.BackgroundRole) is not a QBrush in report_table, as there is no guaranty what the return value from QAbstractDataModel.data actually is. At the very least it needs to check for None values (i.e. an empty/null QVariant()).

@kernc kernc force-pushed the fix-table-report branch 2 times, most recently from 90ebbf0 to 1980720 Compare September 16, 2016 19:32
@kernc
Copy link
Contributor Author

kernc commented Sep 16, 2016

In Qt, QBrush is always returned for this role. No guarantee, but a convention. If the user passes arbitrary types for predefined roles, that's a bug waiting to be discovered.

For user roles, it is up to the developer to decide which types to use and ensure that components use the correct types when accessing and setting data.

Emphasis added.

None case is handled.

@ales-erjavec
Copy link
Contributor

Really anything which is qvariant_cast<QBrush>() convertible can be a returned/set for a Qt::BackgroundRole as far as Qt is concerned. This includes QColor and even QPixmap.

It is an unfortunate consequence of PyQt QVariant api 2 that the abstraction that is provided by the QVariant is no longer available.

Note that qvariant_cast<T>() or one of the equivalent converters is the only way to get any value out of a QVariant, and that if passed a value it cannot convert it returns a default constructed T(). So you might say that graceful fallback when presented with arbitrary types in Qt item/model view framework is not only a convention but a rule.

None case is handled.

Sorry I did not see that.

@kernc
Copy link
Contributor Author

kernc commented Sep 19, 2016

Thanks for the elaborate response. Please find the PR completely revised.

@kernc kernc changed the title itemmodels.TableModel: Qt.BackgroundRole should be QBrush [FIX] Reporting tabular in Data Table and Rank widgets Sep 19, 2016
@ajdapretnar
Copy link
Contributor

Is this solving crash for Report? Because the fix works, but there's also the issue of this:

fall-out

Also it makes reporting very slow. I don't remember how it was before, but it takes about 30s-1m to report on adult.tab (Win10).

@@ -216,16 +219,18 @@ def data(role=Qt.DisplayRole,
if row is None or col is None:
return model.headerData(col if row is None else row,
orientation, role)
return model.data(model.index(row, col), role)
return try_fmtnum(model.data(model.index(row, col), role))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not reformat the number here, at least not like this.

Orange used to keep track of the number of decimal places it should print for a specific variable. For instance, the "ST by exercise" has only a single decimal in the heart_disease.tab file, and the "age" and the "number of vessels colored" have none, obviously. Orange used to record this when loading the data. Now all numeric values have three decimals.

I consider this a bug (#1581). After this is fixed, model.data that is used in, say, the Table widget, will (again) show the numeric values with appropriate number of decimals, so this function should not reformat it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rebased and without the offending commit.

kernc and others added 3 commits October 7, 2016 09:45
They should not be overriden, unless there is some non-obvious reason,
in order to make report unittest work for this widget.
@kernc kernc added this to the 3.3.8 milestone Oct 7, 2016
@astaric astaric merged commit b5e2b96 into biolab:master Oct 7, 2016
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.

7 participants