Unable to delete cylinders and their associated pressure data after Liquivision data import

[vklimovs]

Describe the issue:

• Bug
• Change request
• New feature request
• Discussion request

Issue long description:

After importing Liquivision file (.lvd) that contains data from more than 1 pressure transmitter (sensor), it's not possible to edit Cylinders section of Equipment tab.

Operating system:

Windows 10, English
Liquivision Lynx by downloading data using proprietary app and then importing into Subsurface.

Subsurface version:

Subsurface 4.7.4
Previous Subsurface versions had bugs wrt import of .lvd files which are now fixed (thank you!)
Using official release.

Steps to reproduce:

0. Using proprietary software (Liquivision dive logger) download dive data from the dive computer and save into .lvd file. Some dives must have pressure readings from more than 1 sensor.
1. Import said .lvd file into Subsurface
2. Attempt to delete one of the Cylinders from Cylinders section of Equipment tab

Current behavior:

Nothing happens - after performing the deletion and pressing "Apply changes", all deleted cylinders reappear.

Expected behavior:

Cylinder and its associated pressure information is deleted from the database and disappears from the UI and dive profile graph. It should be possible to delete all cylinders and their associated pressure data.

Additional information:

Attached is a sample log file in which it's possible to see the issue:
test.zip

[mturkia]

The log file has sensor 0 and sensor 4 readings. And that results in 5 cylinder rows in the equipment tab even though only 1 cylinder is defined in the log file. Clearly this case is incorrectly handled by Subsurface.

[vklimovs]

Fwiw, having sensors be numbered 0 and 4 makes sense - this evidently comes from a computer itself that has well defined index of transmitters (configurable by user). On this particular dive only transmitters/sensors 0 and 4 were present. Index 0 is reserved for the transmitter of the user of the computer themselves. Buddies 1,2,3 (thus sensors 1,2,3) were not present on this dive, thus their transmitters were also not present.

The problem is with handling of this data past import, as you correctly noticed.

[sfuchs79]

I did some investigations around this during the last weeks because I also saw an issue and finally I think I understand part of it a little bit better. From my point of view we have two issues:

• Display of unused cylinders. The fact that there may be unused cylinders coming from a DC import is well known and there is even the preferences option if we want to show them or not. BUT if you say "don't show unused cylinders" then this covers only unused cylinders correctly if they are the last cylinders in the list. If, as in this case, cyl 0 is used, cyl 1-3 unused and cyl 4 is used, cyl 1-3 will always be displayed.
  The reason for this is here:
  https://github.com/Subsurface-divelog/subsurface/blob/master/qt-models/cylindermodel.cpp#L491
  The same thing also becomes visible if you merge dives where the first dive has unused cylinders. There these cylinders also change from being not displayed to being visible.
• Deleting cylinders is still a little bit a mess. I tried to improve this during the last months but up to now failed to catch all potential scenarios. The cylinders reappearing after they are deleted seems to come from pressure info being repopulated for the wrong cylinder index but I don't know yet where this comes from.

[sfuchs79]

Ahhh, that's funny:
@vklimovs Your DC does support how many pressure sensors? 5? Even more?
That's not good!!! Linus allows you to have only 2 sensors!
Yeah, I'm just kidding - I'm in a funny mood - unfortunately ill at home with a bad cold.

Linus in fact forewarned us that this handling of multiple pressure sensors is not yet complete:
adb4b66

@dirkhh I'll once simply try to increase MAX_SENSORS plus some bugfix because MAX_SENSORS is even not used consistently - at another point the number of 2 is hard coded...
Beside this what do you think? I guess we sooner or later ask Linus? ;-)

[sfuchs79]

Ok, what I wrote about the MAX_SENSORS (and Linus) is not correct ;-)
Even with max sensors set to 2 it would be possible to deal with the max 9 (?!) pressure sensors from the Lynx.

[dirkhh]

Please keep in mind that we also need to distinguish between the side-mount case (two sensors displaying the pressures of the two tanks that are used at the same time without gas switches), the rebreather case (whatever that case may be, I have no clue about rebreathers), and then this case where the dive computer tracks pressure readings from other divers.
IMHO, we should simply ignore the other sensors unless there are tanks defined for them.

[sfuchs79]

Created a PR #870 with a couple of small improvements. Inside this PR there is one commit which fixes the "can't remove the cylinder" part of this issue for me.
For the "why are the unused cylinder even displayed if I disabled the prefs option" (see above), the explanation is given above with the link to the code. But difficult how to fix this because I think it's not so easy to exclude cylinders in the middle of the list from being displayed. Maybe not fix it at all?!

[vklimovs]

Liquivision supports up to 10 transmitters (sensors). From my POV, the most important part is being able to delete the cylinders and associated data, I don't actually care about my buddys data graph at that point.

[sfuchs79]

@vklimovs
Ok, we are now half way done for deleting the cylinders.
You can now (with latest continuous build, link below) safely delete the unused cylinders w/o them reappearing like zombie cylinders ;-)
For the two cylinders with pressure info there is still a bug which needs to be fixed.
@dirkhh
The question is here already the strategy:
Scenario: We have two (or more) cylinders with pressure info in samples.

• Do we even allow to delete one of these cylinders (they are all "used")? Usually one wants to keep them as long as these are your own cylinders. But with such fancy DCs where you can track the cylinder pressure of your buddies you really may want to delete them later on.
• If yes, what to do with the corresponding pressure info in the samples? Delete it as well?

Today we do "everything" wrong here: We first allow to delete the "used" cylinder and then we "merge" the pressure info which is clearly incorrect.

https://github.com/Subsurface-divelog/subsurface/releases

[vklimovs]

I think even if pressure data is present user should be free to delete the cylinder though, why not? User can edit (delete) any other data about the dive, why not the cylinder pressure.