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

Don't show cylinder pressure graph for wrong dc for merged dives #815

Conversation

sfuchs79
Copy link
Contributor

Describe the pull request:

  • Bug fix
  • Functional change
  • New feature
  • Documentation change
  • Language translation

Pull request long description:

When merging a logged dive from a dive computer and a planned dive
the cylinders are not always merged. They are e.g. not merged if we
have manually entered pressure values. So we correctly end up with
a doubled set of cylinders.
When now plotting the pressure graphs we need to make sure to only
plot graphs for the cylinders which really belong to the displayed dc.
This PR introduces this behavior.

Changes made:

Related issues:

This fixes part of what is reported in #697 (but not everything yet)

Additional information:

Mentions:

@willemferguson once had a example for such a situation. Can you please test this?

When merging a logged dive from a dive computer and a planned dive
the cylinders are not always merged. They are e.g. not merged if we
have manually entered pressure values. So we correctly end up with
a doubled set of cylinders.
When now plotting the pressure graphs we need to make sure to only
plot graphs for the cylinders which really belong to the displayed dc.
This commit introduces this behaviour.

Signed-off-by: Stefan Fuchs <sfuchs@gmx.de>
@sfuchs79
Copy link
Contributor Author

sfuchs79 commented Nov 15, 2017

@willemferguson
Ah, found Willems example file. Looks way better with this fix :-)

grafik

@dirkhh
Copy link
Collaborator

dirkhh commented Nov 16, 2017

Seems reasonable. It would be good to have Linus look at it as well.

@neolit123
Copy link
Member

hi, @sfuchs79 can you please ping Linus and the ML about this?

@torvalds
Copy link
Collaborator

I don't think this is necessarily wrong, but I posted a syntactically different but logically equivalent (hopefully) series as patches on the mailing list.

@sfuchs79
Copy link
Contributor Author

Closed because we have the better solution #826

@sfuchs79 sfuchs79 closed this Nov 17, 2017
@sfuchs79 sfuchs79 deleted the bugfix_pressure_graphs_merged_dives branch November 17, 2017 06:48
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.

None yet

4 participants