[WIP][RFC] Sort unused cylinders to the end of cylinder list

[sfuchs79]

Describe the pull request:

• Bug fix
• Functional change
• New feature
• Code cleanup
• Build system change
• Documentation change
• Language translation

Pull request long description:

Sort the unused cylinders to the end of the cylinder list when
option to display unused cylinders is set to off.

Changes made:

Related issues:

Closes #880

Additional information:

Release note:

Documentation change:

Mentions:

[sfuchs79]

No idea regarding the tests.

But one other important point:
After doing this code it became clear to me that it is most likely NO GOOD IDEA to do the copy(&displayed_dive, current_dive) from CylindersModel::updateDive()
But that is maybe finally no huge issue for the overall idea because I can move the code also somewhere else and call it with a different strategy. For example I could put it into such a fixup_blabla function and call it after dive import and dive merge and...
But maybe then not every dive is automatically fixed up when it is once selected...
One other idea would be to not really fix the real dive data but only the displayed_dive data... but does this really work?

Ok, this is clearly an RFC item...

[dirkhh]

This has a few interesting side effects that I'm not sure you want.
The idea is that the displayed_dive is what you see, the current_dive is what's in our data storage.
So at any time a partial edit could be active that hasn't been accepted, yet. So no, please don't do that.

[dirkhh]

As for the test failure... the GitStorage test is especially sensitive to race conditions. If anyone, anywhere else is running that test at the same time, or messing with our rather well documented test account, this test can fail.
I could obviously restart the build, but since I asked you for a change, I figure we'll wait for the next Travis CI run after you push an updated commit.

[sfuchs79]

Yes, you are totally right about the copy_dive usage. I guess you saw from my comments that immediately after finishing the first version of this I had strong doubts that this copy_dive at this point is totally wrong.

How do you like the general idea of moving the "unused" cylinders to the end of the list?

Regarding the copy_dive I may have good news. The story is that I introduced the copy_dive in the middle of my work while there was still a pile of other bugs in my code. At that moment I incorrectly thought that I really need to do it. After removing all the other bugs I never removed the copy_dive - until now.
After removing the copy_dive now I found out that the code works almost perfectly the way I would like it.
There is just still a small glitch when entering the preferences dialog and immediately after leaving the preferences dialog. But I guess I can fix this. Maybe just another call to cylindersModel->updateDive(); needed in the right place...

[bstoeger]

Hmm... In C++ you can simply assign structs: temp_cyl=displayed_dive.cylinder[cylid]. Wouldn't that be more readable?

[bstoeger]

By the way, you could shorten the whole reorder logic significantly, but I don't know which of these constructs are acceptable:

#include <algorithm>
#include <numeric>
...
int mapping[MAX_CYLINDERS];
std::iota(&mapping[0], &mapping[MAX_CYLINDERS], 0); // Fill with 0..MAX_CYLINDERS-1
auto &cylinder = displayed_dive.cylinder;
if (pos > cylid) {
  std::rotate(&cylinder[cylid], &cylinder[cylid + 1], &cylinder[pos + 1]);
  std::rotate(&mapping[cylid], &mapping[cylid + 1], &mapping[pos + 1]);
} else {
  std::rotate(&cylinder[pos], &cylinder[cylid], &cylinder[cylid + 1]);
  std::rotate(&mapping[pos], &mapping[cylid], &mapping[cylid + 1]);
}
...

In my opinion, this is more readable than the various for-loops, but YMMV.

[dirkhh]

I am infamous for not wanting to add too much C++-y-ness to our code. But yeah, wow, that looks much nicer to me :-)
👍

[sfuchs79]

Doing a quick grep also shows me that we are already using such constructs like std:sort and others in Subsurface code.
So ok for me to change this, but:

• Will take me some time because I want to really learn and understand this, not just copy&paste and I then want to update at least also similar functions in same file (->seperate commits)
• I first like to finally know if all this open cylinder handling changes will get approved (and I'm not in a hurry here)

[dirkhh]

• yes on consciously using C++ things like std::rotate, std::sort, etc
• yes on UNDERSTANDING them before using them
• yes on cleaning up things as you go - @bstoeger is doing such an amazing job of this
• I think I would like to delay merging the open cylinder handling until after 4.7.5, but overall I think this is a good thing to work on as there are clearly cases where the current code is just wrong

[dirkhh]

This has been quiet for six weeks... is this abandoned, or did you just get busy? Or is this waiting for something from me? I can't quite tell by the comment flow

[sfuchs79]

@dirkhh
No, not abandoned. This change and the other cylinder related PR where still waiting for a final discussion and decision if these changes in cylinder handling are the right thing to do.
Also means that some more (functional) testing by s.o. other than me would be good.

[dirkhh]

I think it would make sense to try to have that conversation on the mailing list, mentioning this PR.
Cylinder handling is something that (usually) a lot of people have strong feelings about :-)

[sfuchs79]

Strange - I after very long time of using this in my "private" build finally found some bugs in this code.
Good thing that we didn't merge it yet.
I was able to fix one major bug now but there are still these issues:

• When not displaying unused cylinders at "middle" positions the cylinder indexes displayed in the info box in the profile are incorrect
• Immediately after leaving the preferences dialog when one deselected "display unused cylinders" the displayed cylinders in the equipment tab are incorrect. One needs to deselect and reselect a dive to get correct cylinders displayed.

I added 4 dives which can be used to debug this feature:
Test.zip

[sfuchs79]

I give up on this one...

[bstoeger]

Did you give up for technical or conceptual reasons? I could help with the first, but not the second (never did a multi tank dive, and never will).