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

Bug when selecting attribute in RGB image #755

Merged
merged 12 commits into from
Oct 22, 2015

Conversation

astrofrog
Copy link
Member

To reproduce:

  • Load RGB image (e.g. PNG)
  • Open image viewer
  • Change attribute, and nothing changes

@astrofrog
Copy link
Member Author

@ChrisBeaumont - I'm a bit confused about this bug because I'm pretty sure it would have worked in the past, but this should fix it.

@astrofrog
Copy link
Member Author

Weirdly, this does not work if I now load two images and then flick between the datasets in the same image viewer, in that _update_attribute raises an exception. I need to look into it.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - could you review this? Basically this fixes the scenario where you load two different datasets (in my case an RGB image and a FITS file), then drag dataset 1 onto the the main area and make an image viewer, then drag dataset 2 onto the main area (this should cause the data in the combo box to change, which it didn't before), then select dataset 1 from the data combo box again (which caused various errors).

Is this now the intended behavior, or should one not be able to add multiple datasets to an image viewer?

I'll add a regression test if this looks good to you otherwise.

@@ -237,7 +237,7 @@ def connect_current_combo(client, prop, widget):
"""

def _push_combo(value):
idx = widget.findData(value)
idx = _find_combo_data(widget, value)
Copy link
Member Author

Choose a reason for hiding this comment

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

This will go away once #771 is merged and this is rebased.

Copy link
Member Author

Choose a reason for hiding this comment

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

As indicated in #755, I think we actually need to merge this first, so I'm actually just going to merge in the changes from 771 here to make things easier.

@astrofrog astrofrog mentioned this pull request Oct 22, 2015
@astrofrog
Copy link
Member Author

Ignore for now - there are genuine test failures.

@astrofrog
Copy link
Member Author

@ChrisBeaumont - this is now ready for review. It grew beyond the original bug and fixes:

  • The fact that the image did not update if the RGB component was changed
  • The fact that the data and combo boxes were not updated correctly when showing multiple datasets in the same image viewer
  • Fixes minor bugs with widget properties and update_combobox
  • Includes the fix for the segfault with PySide

I think that everything should pass now. Let me know what you think!

if idx == -1:
return
# NOTE: we can't use findData here because if the data is not a
# string, PySide will crash
Copy link
Member

Choose a reason for hiding this comment

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

I've been bitten by findData like this too. We could envision fixing this method in our qt import/wrapper utility, so that findData behaves as we expect.

Copy link
Member

Choose a reason for hiding this comment

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

(well maybe, I'm not sure how much metaclass magic happens when you instantiate a QWidget, and whether that makes overriding methods tricky)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've made an issue to remind me to do this: #774

It will require some more in-depth testing hence why I'm postponing it for now.

@ChrisBeaumont
Copy link
Member

LGTM

astrofrog added a commit that referenced this pull request Oct 22, 2015
Bug when selecting attribute in RGB image
@astrofrog astrofrog merged commit 559aa08 into glue-viz:master Oct 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants