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] Tree Viewer: check if there is selected class value #2224

Merged
merged 1 commit into from
May 3, 2017
Merged

[FIX] Tree Viewer: check if there is selected class value #2224

merged 1 commit into from
May 3, 2017

Conversation

jerneju
Copy link
Contributor

@jerneju jerneju commented Apr 12, 2017

Issue

Tree Viewer crashes when Target Class combo selected index number is higher than actual number of class values.
https://sentry.io/biolab/orange3/issues/205702715/

Description of changes
Includes
  • Code changes
  • Tests
  • Documentation

@jerneju
Copy link
Contributor Author

jerneju commented Apr 12, 2017

@janezd
Copy link
Contributor

janezd commented Apr 12, 2017

This is a patch, not a fix. :)

The combo should not have had an invalid value in the first place. I guess you should discover why this occurred and fix the problem there.

@codecov-io
Copy link

codecov-io commented Apr 12, 2017

Codecov Report

Merging #2224 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2224      +/-   ##
==========================================
+ Coverage   72.49%   72.49%   +<.01%     
==========================================
  Files         319      319              
  Lines       55210    55211       +1     
==========================================
+ Hits        40024    40025       +1     
  Misses      15186    15186

Copy link
Contributor

@janezd janezd left a comment

Choose a reason for hiding this comment

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

This is a patch, not a fix. :)

The combo should not have had an invalid value in the first place. I guess you should discover why this occurred and fix the problem there.

@jerneju
Copy link
Contributor Author

jerneju commented Apr 14, 2017

Now I fixed it in another way.

@lanzagar
Copy link
Contributor

Does not fix the problem for me.
Also, there are now two consecutive calls to openContext.

@janezd
Copy link
Contributor

janezd commented Apr 21, 2017

Now I fixed it in another way.

Where is this other fix? Did you forget to push?

@jerneju
Copy link
Contributor Author

jerneju commented Apr 21, 2017

Where is this other fix? Did you forget to push?

Changed it back.

@janezd
Copy link
Contributor

janezd commented Apr 28, 2017

I don't get it. :) What is the state of this PR now? Should we just close it since it doesn't fix the problem in the right way and it doesn't contain any code that would be useful in the actual fix?

If we merge this as a "temporary fix", we nobody will remember to remove it after the actual fix ... and we have too much of such dead code already. :)

@janezd
Copy link
Contributor

janezd commented May 3, 2017

I thought that you would have to call self.color_combo.setCurrentIndex(self.target_class_index) after openContext, but setup_scene calls activate_loaded_settings which in turn calls the combo's setCurrentIndex. Knowing this, even the existing call self.color_combo.setCurrentIndex(self.target_class_index) is actually redundant ... but leave it as it is.

@janezd janezd merged commit 8228bcc into biolab:master May 3, 2017
@jerneju jerneju deleted the index-treeviewer branch May 4, 2017 07:22
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.

4 participants