Implement dive-invalid flag. Needs some UI work and fine-tuning. #2440

bstoeger · 2019-12-13T06:12:46Z

Describe the pull request:

Pull request long description:

This implements the "mark dive invalid" feature that has been requested a few times.

This implements the core code. However, the UI code is only a proof of concept: Invalid dives, if shown, are marked by a gray background. This is unfortunate as this is not visible when selected. I'd ask someone else to do the UI part, if there is interest.

The invalid flag is considered for the statistics and when calculating a new dive number. This probably needs some fine-tuning. For example, what about residual N2 in the planner?

Changes made:

Implement invalid-flag
Save/load invalid-flag
Implement undo-command to set/reset the invalid-flag
Update filter when changing the show-invalid-dives preferences
Ignore invalid dives in statistics / when calculating new dive numbers

Related issues:

This feature was suggested as a potential solution for #2154.

Additional information:

Release note:

Let's wait for approval / refinement.

Documentation change:

Should be done, yes.

Mentions:

@dirkhh @willemferguson @atdotde

bstoeger · 2019-12-13T06:19:36Z

Sorry for the color-explosion-banner. I just scrolled through the tag-list and checked everything that applies. 😛

dirkhh · 2019-12-13T12:13:47Z

I think this should best be looked at by someone who actually expressed interest in this. I always found this a very strange concept.

bstoeger · 2019-12-13T13:43:12Z

I'll ask on the mailing list. If there is no interest, we can also remove the stub and the obscure preferences entry. Fine with me.

janiversen · 2020-01-25T09:17:56Z

just for information, we have a case in issue #2555 where the invalid flag could be useful.

Also I think, maybe wrongly, that #2513 might be related to this PR as well, or more correctly some work regarding invalid flag already merged.

Implement reading/writing the flag from/to XML/git. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

bstoeger · 2020-03-18T23:04:43Z

Rebased on master - that was somewhat painful with all the prefs churn. On the mailing list, there were two or three positive, though not ecstatic, comments and no negatives. In a quick test, the main problem was that if a dive is selected you don't see whether it is invalid or not. So this would need some UI work. Also it is unclear if "invalid" dives should be considered by the planner to calculate remaining gas load, or whatever magic it does. :-P

In any case, I'd really like to come to a decision here. I'd prefer to not have to rebase again.

bstoeger · 2020-03-19T08:03:45Z

Update: Make the font italicized for invalid dives. Looks absolutely horrid, but at least one now recognized invalid dives when selected. The best I can come up with (apart from writing a custom "delegate", which I really don't feel like doing).

dirkhh · 2020-03-19T15:33:51Z

I am hoping to find time to play with this today... sorry to not have provided feedback earlier.
Oddly not going out hasn't improved the net number of available hours for Subsurface...

dirkhh · 2020-03-19T21:44:51Z

OK, played with this.
You are a UI genius. If this professor thing doesn't work for you, consider a career as a visual designer. That use of italics is inspired and game changing. I'm all in.
:-)
The feature seems to work as intended, I wasn't able to break anything or to spot a scenario where it seemed broken. My testing wasn't comprehensive, I'm sure, but I tried what felt like the obvious scenarios.
Hopefully later this afternoon I'll try to come up with something even more inspired concerning the UI, but with that one caveat, I don't have a lot to complain about.
Thank you.

bstoeger · 2020-03-19T22:37:02Z

New attempt: use struck out font instead of italics - I think that's universally understood as "invalid".

dirkhh · 2020-03-20T16:32:08Z

Looked at your new UI approach. Definitely better. That is indeed intuitive.
One thing that I found strange when playing with it was the "make valid" wasn't the top entry in the context menu for an invalid dive. That's of course a pain because that means having two entries there, exactly one of which is visible at a time - but when I played with this I had the pretty strong feeling that the "make valid" entry should be the first.

Now we need to implement something similar for the mobile UI - but that's not required to merge this part, I guess. Oh - my mobile build is broken with this branch - but it seems to work on GitHub Actions, so I'm guessing it's something that I borked here.
Edited to add: yep, my mobile build directory was completely messed up, no idea how I did that... blew it away and did a clean build, no issues - except that we have no UI support for invalid dives, of course

dirkhh

I almost clicked merge, then I noticed that the commit message of 0d45e5c may need some work :-)

dirkhh · 2020-03-20T16:51:31Z

qt-models/divetripmodel.cpp

+static QFont struckOutFont()
+{
+	QFont font;
+	font.setStrikeOut(true);


good choice - but doesn't match the commit message :-)

Connect command to context menu. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Hide invalid dives if prefs.display_invalid_dives is false. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Mark invalid dives in the dive list by striking them out and rendering them with a grey color. The color-change is not sufficient, because the default model delegate ignores color hints if the item is selected. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

If the dive the user clicked on is invalid show an option to make the dive valid. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

The user had to restart the application or manually change the filter if they changed the flag. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Adding dives uses the number of the last dive to create a new dive number. Ignore invalid dives. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

dirkhh · 2020-03-20T22:20:39Z

for some reason I no longer get notifications when you push things...
yes, this is good. thanks!

bstoeger added dive-list core dive-filter enhancements-we-might-add work-in-progress needs-testing needs-documentation-change needs-release-note git rfc undo parser labels Dec 13, 2019

bstoeger mentioned this pull request Dec 13, 2019

New feature: track pool dives #2154

Closed

4 tasks

Core: introduce invalid flag for dives

38a60da

Implement reading/writing the flag from/to XML/git. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

bstoeger force-pushed the invalid branch from de8e494 to b83da92 Compare March 18, 2020 22:54

bstoeger force-pushed the invalid branch from b83da92 to 7868fa9 Compare March 19, 2020 08:00

bstoeger force-pushed the invalid branch from 7868fa9 to 81f29e0 Compare March 19, 2020 22:35

dirkhh approved these changes Mar 20, 2020

View reviewed changes

bstoeger added 2 commits March 20, 2020 18:05

Undo: implement invalidate-dive command

8fb582e

Connect command to context menu. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Filter: hide invalid dives

698abf1

Hide invalid dives if prefs.display_invalid_dives is false. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

bstoeger added 5 commits March 20, 2020 18:05

Dive list: add option to mark dives valid

39a1c54

If the dive the user clicked on is invalid show an option to make the dive valid. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Dive filter: reset filter on change of display_invalid_dives flag

d20ff96

The user had to restart the application or manually change the filter if they changed the flag. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Core: consider invalid flag when adding dives

0383f1b

Adding dives uses the number of the last dive to create a new dive number. Ignore invalid dives. Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

Ignore invalid dives in statistics

2b7bc3f

Signed-off-by: Berthold Stoeger <bstoeger@mail.tuwien.ac.at>

bstoeger force-pushed the invalid branch from 81f29e0 to 2b7bc3f Compare March 20, 2020 17:08

dirkhh merged commit f4f1503 into subsurface:master Mar 20, 2020

bstoeger deleted the invalid branch March 21, 2020 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement dive-invalid flag. Needs some UI work and fine-tuning. #2440

Implement dive-invalid flag. Needs some UI work and fine-tuning. #2440

bstoeger commented Dec 13, 2019

bstoeger commented Dec 13, 2019

dirkhh commented Dec 13, 2019

bstoeger commented Dec 13, 2019

janiversen commented Jan 25, 2020

bstoeger commented Mar 18, 2020

bstoeger commented Mar 19, 2020

dirkhh commented Mar 19, 2020

dirkhh commented Mar 19, 2020

bstoeger commented Mar 19, 2020 •

edited

dirkhh commented Mar 20, 2020 •

edited

dirkhh left a comment

dirkhh Mar 20, 2020

dirkhh commented Mar 20, 2020

Implement dive-invalid flag. Needs some UI work and fine-tuning. #2440

Implement dive-invalid flag. Needs some UI work and fine-tuning. #2440

Conversation

bstoeger commented Dec 13, 2019

Describe the pull request:

Pull request long description:

Changes made:

Related issues:

Additional information:

Release note:

Documentation change:

Mentions:

bstoeger commented Dec 13, 2019

dirkhh commented Dec 13, 2019

bstoeger commented Dec 13, 2019

janiversen commented Jan 25, 2020

bstoeger commented Mar 18, 2020

bstoeger commented Mar 19, 2020

dirkhh commented Mar 19, 2020

dirkhh commented Mar 19, 2020

bstoeger commented Mar 19, 2020 • edited

dirkhh commented Mar 20, 2020 • edited

dirkhh left a comment

Choose a reason for hiding this comment

dirkhh Mar 20, 2020

Choose a reason for hiding this comment

dirkhh commented Mar 20, 2020

bstoeger commented Mar 19, 2020 •

edited

dirkhh commented Mar 20, 2020 •

edited