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

removing a one or more now updates commodities lists #104

Merged
merged 3 commits into from
Jun 8, 2015

Conversation

FlanFlanagan
Copy link
Member

No description provided.

@gidden
Copy link
Member

gidden commented Jun 7, 2015

I set up a simulation with two sources, one for commodity c and one for d, with a sink that takes both, and have tried the following:

  • remove commodity d: still in source list, still in sink list, but arrow is removed
  • re-add commodity d: arrow not reinstated

I suppose I don't understand what is the PR supposed to do?

@FlanFlanagan
Copy link
Member Author

can you show me the behavior.

This PR lets you remove a onOrMore field with a commodity somewhere inside it and the Pane should reflect the removal of the commodity.

@gidden
Copy link
Member

gidden commented Jun 7, 2015

Hi Radio,

I think this is really close to solving the problem. I see that now removal happens if I explicitly click the "remove" button. Removal does not happen if I "remove" the commodity by changing the drop box. I think the best way to try this is the following pseudo code:

if commodity changes via drop down box:
    if commodity does not exist in other list (as another drop-down box entry):
        remove commodity connections

@FlanFlanagan
Copy link
Member Author

It's more complex than that, but I see the problem.

@FlanFlanagan
Copy link
Member Author

Need a set of eyes on this, it's been updated to fix the bug matt encountered.

@gidden
Copy link
Member

gidden commented Jun 8, 2015

If I remove commodities from the list on the left, they still stay on the agents.

@FlanFlanagan
Copy link
Member Author

Oh sorry, I meant a different bug. This PR was not meant for the list on the left. But I'll just put that code into this PR instead of making a new one.

@gidden
Copy link
Member

gidden commented Jun 8, 2015

ok. while this would be a nice fix to have I'm not sure how much work it will take and where it ranks on our triage list. perhaps @gonuke can offer an opinion.

@FlanFlanagan
Copy link
Member Author

Oh, so I have the underlying code working. I just need to get it to redraw the formBuidlers, which I'm working on now. Currently if you close the form and reopen it the change is reflected, but now live.

@FlanFlanagan
Copy link
Member Author

er not live*

@gonuke
Copy link
Member

gonuke commented Jun 8, 2015

Does this last commit make this ready for review?

@gonuke
Copy link
Member

gonuke commented Jun 8, 2015

This looks like it does what we need for now. Removing the commodity from open comboBoxes will require more effort that we currently have time for.

@FlanFlanagan
Copy link
Member Author

Unfortunately I agree with that. If I get a chance I will take a stab today but no promises.

@gonuke
Copy link
Member

gonuke commented Jun 8, 2015

No - I don't think we should be taking stabs at anything now. There is too much chance that other new things emerge and we need to keep the docket clear fr that.

I'll merge this and we can just make an issue for later.

gonuke pushed a commit that referenced this pull request Jun 8, 2015
removing a one or more now updates commodities lists
@gonuke gonuke merged commit d60d08a into cyclus:v1.3.0-release Jun 8, 2015
@FlanFlanagan
Copy link
Member Author

Aye aye

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.

3 participants