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

[ENH] Datasets: Remove control area #4071

Merged
merged 4 commits into from
Oct 11, 2019

Conversation

janezd
Copy link
Contributor

@janezd janezd commented Oct 1, 2019

Sapientem sat?

Screenshot 2019-10-03 at 14 54 35

(Fixes #4069.)

To show indicators with different colors, it needs biolab/orange-widget-base#17. Before biolab/orange-widget-base#17 is merged, indicators will remain black, as they are now, but the widget will still work.

Includes
  • Code changes

@codecov
Copy link

codecov bot commented Oct 1, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@e378407). Click here to learn what that means.
The diff coverage is 78.57%.

@@            Coverage Diff            @@
##             master    #4071   +/-   ##
=========================================
  Coverage          ?   85.56%           
=========================================
  Files             ?      385           
  Lines             ?    69189           
  Branches          ?        0           
=========================================
  Hits              ?    59200           
  Misses            ?     9989           
  Partials          ?        0

@PrimozGodec
Copy link
Contributor

I tested it and it works well. There is just a need to change the documentation screenshots and maybe add a sentence about the status color. Should we do it here or in the separate PR?

Copy link
Contributor

@lanzagar lanzagar left a comment

Choose a reason for hiding this comment

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

I like the change.

Some comments:

  • Users could previously choose a dataset and click Send (or have auto send checked). After we remove the button, double click is probably the only way to do this? Is that obvious enough (honest question, I don't know)?
    We rarely use double clicking in Orange and there was a more direct way before that now just disappears. Maybe we could (temporarily?) have an info box at the top of the widget like in Test&Score, which would explain "Double click a dataset to load it" or similar?
  • Upon downloading a dataset warnings are printed: 'processEvents' argument is deprecated.
    It has nothing to do with this PR, but it should be as simple as searching for and removing all occurrences of processEvents=None.
  • This breaks Single Cell Datasets. There should probably be some refactoring and maybe a common base class, but I would avoid dragging all that into this PR. I think the following should be enough:
    • We add openclass=True, just to make it clearer that this widget is reused (it should have been there already)
    • You changed load_data to load_and_output. We could leave load_data as it was and just call it inside a new method, which should then work fine with the downstream changes.

if path is None:
self.Outputs.data.send(None)
else:
data = Orange.data.Table(path)
Copy link
Contributor

Choose a reason for hiding this comment

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

The previous load_data method is overriden in Single Cell Datasets.
You can still call it here and make this a new method instead of changing it from load_data, which should keep everything working downstream.

@janezd janezd force-pushed the owdataset-remove-control-area branch from e36ec39 to 85d0623 Compare October 10, 2019 12:51
@janezd
Copy link
Contributor Author

janezd commented Oct 10, 2019

Users could previously choose a dataset and click Send (or have auto send checked). After we remove the button, double click is probably the only way to do this? Is that obvious enough

I suppose. If nothing happens on single click, the normal reflex is to double-click.

Upon downloading a dataset warnings are printed: 'processEvents' argument is deprecated.
It has nothing to do with this PR, but it should be as simple as searching for and removing all occurrences of processEvents=None.

I didn't want to do this in this PR, but it's trivial, so now I did it.

This breaks Single Cell Datasets. There should probably be some refactoring and maybe a common base class, but I would avoid dragging all that into this PR. I think the following should be enough:
We add openclass=True, just to make it clearer that this widget is reused (it should have been there already)
You changed load_data to load_and_output. We could leave load_data as it was and just call it inside a new method, which should then work fine with the downstream changes.

I restored load_data, but I wouldn't add openclass=True. It is preferable to have a warning that reminds us to extract common routines to a base class. openclass=True is also a promise that we won't break compatibility; we can only do this after refactoring.

@PrimozGodec
Copy link
Contributor

Users could previously choose a dataset and click Send (or have auto send checked). After we remove the button, double click is probably the only way to do this? Is that obvious enough (honest question, I don't know)?

What about having a send button/apply at the bottom of the widget (similarly than file widget has)?

@janezd janezd added the needs discussion Core developers need to discuss the issue label Oct 10, 2019
@janezd
Copy link
Contributor Author

janezd commented Oct 11, 2019

Check whether pressing return also submits the data.

Add Tooltip: "Double click to send".

@janezd
Copy link
Contributor Author

janezd commented Oct 11, 2019

Use a different symbol for active data set. Move the delegate here so we can add different symbols in the future.

@janezd janezd force-pushed the owdataset-remove-control-area branch from 85d0623 to 3f3f093 Compare October 11, 2019 09:33
@janezd janezd removed the needs discussion Core developers need to discuss the issue label Oct 11, 2019
@janezd janezd force-pushed the owdataset-remove-control-area branch from 3f3f093 to 7c75ae6 Compare October 11, 2019 12:35
@lanzagar lanzagar changed the title Data Sets widget: Remove control area [ENH] Datasets: Remove control area Oct 11, 2019
@lanzagar lanzagar merged commit 4e3f471 into biolab:master Oct 11, 2019
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.

[RFC] Data Sets widget does not need control area
3 participants