Show NEW! in List Items menu

[Brambor]

Summary

Interface "Show NEW! in List Items menu"

Purpose of change

Discussed something similar in

• Item Snapshot #72697 (comment)

  • List the items you find in a room, highlight ones you haven't seen before

Relaxed transparent for pockets, so that open containers are considered transparent. One line change in commit 5bade42. Originally introduced in #56623 and @I-am-Erk was maybe against this, so we will see...

Describe the solution

Heavily inspired by this, which has done it for crafting menu. referred to as OTHER.

• Allow the player to track newly learned/unviewed recipes #49819

• OTHER allows for reading / unreading recipes. This allows only reading (marking as not NEW!) by going to the item entry by UP or DOWN action.

• OTHER marks as read for more than just UP & DOWN, like mouse support. This doesn't.

These actions will unNEW:

• UP, DOWN
• COMPARE, EXAMINE, SCROLL_ITEM_INFO_UP, SCROLL_ITEM_INFO_DOWN
• TRAVEL_TO
• RIGHT, LEFT (when there are 5 piles of a thing, the LEFT and RIGHT move to the next or previous pile)

These actions will NOT unNEW:

• PAGE_DOWN, PAGE_UP
• FILTER, RESET_FILTER, PRIORITY_INCREASE, PRIORITY_DECREASE, SORT
• zoom_in, zoom_out
• NEXT_TAB, PREV_TAB

Note: The List Items menu does not have mouse support.

uistate remembers a set of items (item_ids to be precise) in a new member variable read_items. It is persistent. I actually don't know if per character or totally.

• Enjoy the feature in the Item List menu brought up by V.
• if an item is not in read_items, NEW! is written, as seen in the picture
• If the item is a transparent container, then if the item or anything inside the item (recursively, for transparent containers) is not in read_items, NEW! is displayed.
  • If you saw a plastic bag but not meat jerky, then plastic bag -> meat jerky is NEW!.
• if it is not NEW! but some container in the nested path is not transparent, then it is marked as "hides contents"
  • 
• if you UP or DOWN to an item, then it is added to read_items, so it is no longer NEW!
• If the item is a transparent container, then the item and everything inside the item (recursively, for transparent containers) is put into read_items and it is no longer NEW!
  • if you DOWN to a zipper bag > plastic bag > meat jerky, then none of these items are NEW! anymore.

Describe alternatives you've considered

• Don't change the transparent pocket function, but add transparent to individual plastic bags, zipper bags etc.
  • My solution is simpler, so if it is agreed upon, I would greatly prefer it. Otherwise, I already see the frustration with "This one pocket isn't working with NEW! properly" and at least 5 PRs fixing these individually and still missing some.
  • Unless there is a good reason to do this alternative, I would prefer less frustration and menial adding of "transparent": true to some pockets.
• Don't hide forgetting items in the debug menu, allow the player to use it. Even add it to the List Items menu.
• Add "Mark all as read" to the List Items menu.
• Add "Show NEW! first" to the List Items menu.
• Reimplement List Items menu in ImGui. Maybe I should have, I spend too much time fixing these small things that will go away one day. - @mqrause is working on it in Rewrite and migrate V menu to ImGui #55503

Testing

TEST Item List-trimmed.tar.gz (updated for hides contents test)

NEW:
In this save, I verified that:

1. (requires merging of Container audit (mostly transparency) #73458)
2. NEW disappears on DOWN and UP, but not on PageUp etc.
3. I can restart it in the debug menu > p Player > F Forget all items
4. going to plastic bag > wild herbs unNEWs both plastic bag and wild herbs items
   • 
5. going to plastic bag, but not wild herbs does NOT unNEW plastic bag > wild herbs
6. going to both plastic bag and wild herbs does unNEW plastic bag > wild herbs

hides contents:

1. Similar to the above case
2. Merge Container audit (mostly transparency) #73458, so that plastic bags etc. are transparent.
3. explore list from save and get this
   • 

Apart from that, I checked the other things shown in the screenshot atop.

Additional context

Introduced a bug (probably):
From this code:

Cataclysm-DDA/src/item_contents.h

Lines 118 to 122 in db4aa91

	/** returns a list of pointers to all top-level items that are not mods */
	std::list<const item *> all_items_top() const;
	/** returns a list of pointers to all visible or remembered content items that are not mods */
	std::list<item *> all_known_contents();

I used all_items_top instead of all_known_contents because the former didn't work with a plastic bag, maybe it is not transparent yet. So the player probably reveals items inside sealed non-transparent containers they shouldn't see.

I would like to allow the player to browse these items later, so it should be addressed, it not only might leak the contents of a container (if you already have the item contained), but it could later show recipes etc. for the yet unseen item inside.

Discovered a ~bug:
These keybindings don't reflect the bound keybinding, The green letter is hardcoded:

We seem to be thinking similarly with @mqrause, which is cool, I like there is more of me since more things I care about will be implemented into CDDA :).

[mqrause]

I used all_items_top instead of all_known_contents because the former didn't work with a plastic bag, maybe it is not transparent yet. So the player probably reveals items inside sealed non-transparent containers they shouldn't see.

You should use all_known_contents anyway. If it doesn't work correctly for some containers, that's a different bug with a different fix.

Discovered a ~bug:
These keybindings don't reflect the bound keybinding, The green letter is hardcoded:

Yeah, I noticed that too in #55503. I didn't get to reimplementing it yet, but I was planning to address it. Might not be worth it to fix in the current ui, since it'll be gone once I finished that PR. Though progress is slow currently.

[Brambor]

You should use all_known_contents anyway.

You are right, I will. Now that somebody else wrote it, I see I probably didn't just overlook something.

If it doesn't work correctly for some containers, that's a different bug with a different fix.

I would like to fix that bug first so that my feature isn't broken from day 0. I will look into it now.

[mqrause]

I would like to fix that bug first so that my feature isn't broken from day 0. I will look into it now.

For the most part it's just pockets missing "transparent": true. It just needs someone auditing that. But for a start it's probably enough if you pick out a bunch of common and obvious ones.

#56623 also has a little bit more discussion about the topic of knowing/seeing contents of containers if you're interested in improving the feature in general.

[Brambor]

Yeah, I already just read that :D

I am changing it with the following force-push, so I am requesting Erk, to review this too.

Edit: I seem to be unable to use GitHub to do that. so I will do it through Discord...

[Brambor]

@I-am-Erk Could you please review this? I am specifically asking for review of commit 5bade42, as you had something to say in #56623 whose implementation conditions I am relaxing now.

[Brambor]

I updated the PR description to reflect the latest force push. What changed:

• transparent, just ctrl+f it
• more actions unNEW, namely EXAMINE, SCROLL_ITEM_INFO_UP, SCROLL_ITEM_INFO_DOWN and more
  • I wanted it for testing and it made sense, so I made it.

[I-am-Erk]

I'm not quite sure the function of the line you're switching to an OR here. Is this making it so that you can identify the contents of a backpack from across the room using V, because it's not sealed? If so, I'm very much not in favour of that, though I like your basic implementation here. I don't recall how much of my recommendation on that first pr ever got implemented.

[mqrause]

I don't recall how much of my recommendation on that first pr ever got implemented.

We need to track:

* have you looked in this container before? Then show the contents.  If not,

* Is this container transparent? Then show the contents. If not,

* Is this container sealed? Then we should check if there's some kind of label and display that. If not,

* Don't show the contents and display "contents hidden" in `V`.

From that list only transparency got implemented. Everything else is still hidden.

Is this making it so that you can identify the contents of a backpack from across the room using V, because it's not sealed? If so, I'm very much not in favour of that

I agree with that. The question is if you can tell what's inside a pocket when it's not within reach to actually check the contents.
And on top of that, a change like that should be its own PR, it's kinda off topic for this one. Your implementation should simply work with whatever the known contents give you. Fixing/improving the things you get from that is a different thing.

[Brambor]

Oh! I can add green ? instead of NEW!, when the contents isn't known.

Maybe NEW! and NEW?? or just ??

This solves my unstated problem I just assumed was obvious:
I will use the absence of NEW! as an indicator that I no longer need to check that item. When there is ? I would know I need to further investigate that item.

Also I will add forgetting the item contents to debug. Maybe my testing was weird since I was next to the items, so my character might have remembered the contents... I need to improve this a bit...

I will revert the change of transparency then. And will make a separate PR, if I have further thoughts on it.

[I-am-Erk]

Oh! I can add green ? instead of NEW!, when the contents isn't known.

Maybe NEW! and NEW?? or just ??

This solves my unstated problem I just assumed was obvious: I will use the absence of NEW! as an indicator that I no longer need to check that item. When there is ? I would know I need to further investigate that item.

Also I will add forgetting the item contents to debug. Maybe my testing was weird since I was next to the items, so my character might have remembered the contents... I need to improve this a bit...

I will revert the change of transparency then. And will make a separate PR, if I have further thoughts on it.

Do you mean NEW? for a container that has contents you haven't checked? If so, we might want a different word, like:

• NEW! container has never been seen before
• Unseen or similar: container has been seen, contents have not. Maybe Closed or Shut?
• no label: you've seen the contents and the container

I'd like a nice short word for "Unviewed" but I don't think New? really conveys the problem in a clear way to users.

[Brambor]

It doesn't need to be too short. The view menu has some space to grow horizontally. That said, I leaned to single char ? and expected the user to understand it from experience. But that is not a good design.

hides content is more descriptive in regards to the content that is unviewed.

Edit: it should be short

[Brambor]

This is the current state (not pushed):

Note that I haven't changed plastic bags to be transparent yet, that is why it "hides content".

[Brambor]

@I-am-Erk

Is this making it so that you can identify the contents of a backpack from across the room using V, because it's not sealed? If so, I'm very much not in favour of that,

According to you, while plastic bags aren't transparent, they shouldn't show their contents, right? Plastic bags are a bad example since I will change them to be transparent, but for example, the 30 gallon plastic barrel will not be transparent so it should not show its contents.

I am pointing this out because otherwise there would be exactly the discrepancy there is now (my unpushed version) with plastic bags: It says "hides contents" but you can clearly see what is inside the (non-transparent) plastic barrel.

Furthermore, the first step in the proposed implementation:

have you looked in this container before? Then show the contents. If not,

This can be remembered only as long as the item is in direct sight, otherwise it would leak information or be very frustrating. If you saw inside a barrel to check it does have water, then somebody else switched it for gasoline out of your sight, your character would either

1. know it contains gasoline, which they shouldn't (leaking internal state, identifying across the room)
2. be presented it contains water, but upon closer inspection see it contains gasoline. This might be frustrating and reported as a bug "Why did it say it contains water?"
3. Stop saying it contains water, which leaks internal state (The player knows it has been tampered with so they can deduce it doesn't contain water anymore.)

Therefore it would say it contains water only as long as it is in direct sight after inspecting it is water.

2 would be possible too, but people (including @ kevingranade) already argued against a similar case that the player character is incapable of remembering all items at a glance. In this case, they are trying to remember only the contents of all items, but I would argue that is nearly the same thing.
#72697 (comment)

"memorize every item in the given area" that's pushing things way too far.

You're either claiming the character has world- class memory that only dozens of people have, or it's abstracting away the process of writing everything down.

Another more likely case might be inspecting the contained water amount from a distance. Either a barrel NPC drank from or a barrel with a funnel that is being filled by rain.

[Brambor]

Update again, added the "hides contents" for non-transparent containers (when nothing NEW! found by recursive search over transparent containers). Note the discrepancy: the item hides its contents, yet the player can see there is yarn inside the bowl.

It doesn't have to have contents to have hides contents as to not leak internal information. It just states it is possible.

It is gray rather than light green as to not attract attention. It is very ordinary.

The wording is somewhat long. But I will live with it for now.

[Brambor]

This PR is ready once tests finish. I will play with it right now, but I already checked whatever I could.

[Maleclypse]

Debug size is fixed in another PR already merged so clang is technically fixed.

[Brambor]

Note: These errors are still in:

Error: src/game.cpp:8628: error: ‘new_col’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
src/game.cpp:8604: note: ‘new_col’ was declared here
Error: src/game.cpp:8628: error: ‘new_str’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
src/game.cpp:8601: note: ‘new_str’ was declared here