-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Add a way to quickly examine the contents of an item #51252
Add a way to quickly examine the contents of an item #51252
Conversation
Looking at that wallet graphic list, any chance some kind of default sort can be applied? |
It may look nicer if identical items are stacked in the list. |
&
I agree that some sorting /collapsing of identical items would be beneficial. Sorting feels like it should be fairly simple to add. Collapsing seems trickier, but I'll look into it.
I agree that certain things would be a lot simpler if this was yet another derived inventory_selector class. That's what I looked into first, but I couldn't see a way of making it work. It felt like (and I absolutely could be missing something here) inventory_selector was not intended to be extended to cases like this. The drawing was specifically walled off in private base class functions, and I didn't want to muck about with them and risk over-complicating/breaking everything else. |
It could work as a sort of nested inventory screen, without the item info directly in the same window, so you still press |
8ea1118
to
c7fdf60
Compare
toy reimplementation of CleverRaven#51252 using advuilist
Were you using IMO, it's a little concerning just how much code (or boilerplate) and functionality is duplicated for UI, but that's not specific to your PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job.
src/advanced_inv.cpp
Outdated
} | ||
item_location sitem_location = sitem->items.front(); | ||
bool contents_to_examine = false; | ||
if( !sitem_location->is_container_empty() ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This block of code is identical to the one in inventory_ui
and could perhaps be moved to a helper. It's not a big deal though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that the duplicate code here isn't ideal. I'm contemplating various ways to simplify the use of various inventory_selector classes (there seems to be a similarly-duplicated block of code for each of them), but I think that's a longer-term goal.
src/inventory_ui.cpp
Outdated
if( selected ) { | ||
return selected.any_item(); | ||
} | ||
} else if( input.action == "HIDE_CONTENTS" || input.action == "SHOW_CONTENTS" ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can fold this into the branch below with else if !(...) { on_input() }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose it could be folded in. If I understand you correctly, the main result of this would be one fewer check in the loop. However, the resulting line:
} else if( input.action == "HIDE_CONTENTS" || input.action == "SHOW_CONTENTS" ) { on_input(); }
Feels like it would be harder to parse six months from now. If that's just my read on it, then I can make the change.
Is there any particular reason why input_context doesn't have a "deregister_action"? Because that would allow this check to be removed entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There probably just wasn't any need for a deregister_action
so far.
If you want to disable more actions though, you'll still have to fold them into one of the branches or you'll get a clang-tidy error (duplicate branch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the unexpected behavior exactly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably the same I found with the pickup menu. Made an issue about that: #51490.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed in master now; let me know if you still have issues with HIDE/SHOW_CONTENTS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed in master now; let me know if you still have issues with
HIDE/SHOW_CONTENTS
Yep. 51727/52602 disable hiding/showing contents in this new screen, so this line is no longer necessary. I'll add the commit once I've dealt with some new rebasing conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be fixed in master now; let me know if you still have issues with
HIDE/SHOW_CONTENTS
Looks like I spoke before I'd fully caught up on the changes. After rebasing, you can use the show/hide contents buttons in this menu, and it won't hide/show/change the items listed, which was the problem before, but it will still say it's hiding things:
I've removed the specific block on hiding/showing in my code, because I don't consider this to be a major problem for the menu anymore, but it's still a bit quirky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't test atm, but from a quick look I think it's because you're replacing the ui_adaptor::on_screen_resize()
for the base class so prepare_layout()
never gets called to hide the entries. You can probably fix that by calling select_one_of( get_selected().locations)
in your resize callback.
EDIT: nooope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#52659 will definitely™ fix it. Sorry for the false info in the previous comment.
src/inventory_ui.cpp
Outdated
point start_position = point( ( inv_column_width + border_width + 1 ), | ||
get_header_height() + 1 ); | ||
|
||
draw_item_info( catacurses::newwin( height, width, start_position ), data ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates and destroys the info window every draw cycle, which seems excessive though it simplifies resizing. You could create this window in a dummy ui_adaptor::on_screen_resize()
since prepare_layout()
and others are private.
quick demo patch
diff --git a/src/inventory_ui.cpp b/src/inventory_ui.cpp
index b3e447b0ce..7b79e4ada2 100644
--- a/src/inventory_ui.cpp
+++ b/src/inventory_ui.cpp
@@ -2933,28 +2933,36 @@ void inventory_examiner::action_examine( const item_location sitem, const bool f
//Tries to create a window covering the right half of the inventory screen
data.without_getch = true;
- // NOTE: This assumes that the first column (currently own_inv_column) is used for the item list:
- int inv_column_width = get_all_columns().front()->get_width();
- /* NOTE: By flagging force_max_window_size as true, this forces the inventory_selector window to be
- * TERMX x TERMY , allowing us to assume the window starts at 0,0 and is TERMX by TERMY. If this is
- * changed in inventory_selector::prepare_layout, then everything breaks. This obviously isn't a
- * great solution */
-
- int border_width = 1; //Assumed, since the value in inventory_selector is private, but reasonable
- int width = TERMX - 2 * border_width - inv_column_width;
- int height = TERMY - get_header_height() - 1;
- point start_position = point( ( inv_column_width + border_width + 1 ),
- get_header_height() + 1 );
-
- draw_item_info( catacurses::newwin( height, width, start_position ), data );
+ draw_item_info( w_ex, data );
}
}
item_location inventory_examiner::execute()
{
shared_ptr_fast<ui_adaptor> ui = create_or_get_ui_adaptor();
+ ui_adaptor dummy;
+ dummy.on_screen_resize( [this]( ui_adaptor &ui) {
+ // NOTE: This assumes that the first column (currently own_inv_column) is used for the item
+ // list:
+ int inv_column_width = get_all_columns().front()->get_width();
+ /* NOTE: By flagging force_max_window_size as true, this forces the inventory_selector
+ * window to be TERMX x TERMY , allowing us to assume the window starts at 0,0 and is TERMX
+ * by TERMY. If this is changed in inventory_selector::prepare_layout, then everything
+ * breaks. This obviously isn't a great solution */
+
+ int border_width =
+ 1; // Assumed, since the value in inventory_selector is private, but reasonable
+ int width = TERMX - 2 * border_width - inv_column_width;
+ int height = TERMY - get_header_height() - 1;
+ point start_position =
+ point( ( inv_column_width + border_width + 1 ), get_header_height() + 1 );
+ w_ex = catacurses::newwin( height, width, start_position );
+ ui.position_from_window( w_ex );
+ } );
+ dummy.mark_resize();
while( true ) {
- ui_manager::redraw();
+ ui->invalidate_ui();
+ ui_manager::redraw_invalidated();
const inventory_entry &selected = get_active_column().get_selected();
if( selected ) {
diff --git a/src/inventory_ui.h b/src/inventory_ui.h
index 02ace17a7b..e03f6b14af 100644
--- a/src/inventory_ui.h
+++ b/src/inventory_ui.h
@@ -833,6 +833,7 @@ class inventory_examiner : public inventory_selector
{
private:
int examine_window_scroll;
+ catacurses::window w_ex;
public:
explicit inventory_examiner( Character &p,
const inventory_selector_preset &preset = default_preset ) :
You can also add action_examine()
to this callback to solve your resizing quirk. Not sure if this is appropriate style though; maybe @Qrox can weigh in.
EDIT: I just realized that this approach disables scrolling in the info window.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just override the create_or_get_ui_adaptor
function and move the item info window creation and drawing code to the callbacks there. Or, just pretend the item info window is an overlay instead of a part of the inventory menu, and create a separate ui_adaptor
for it. Do note that the game window may resize while get_input
is blocking for input, so redrawing it after ui_manager::redraw
instead of in a redraw callback will make it resize incorrectly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not terribly familiar with ui_adaptor right now, but I'll do some playing around with it. Right now I'd favour treating it as an overlay rather than trying to mess with any underlying functions.
How significant an impact is it to create the window every cycle? Given that it's waiting for user input every time I'd file it under "Not a great solution. Kind of annoying", but are there are any things happening further under the hood that might make it more of an issue?
src/inventory_ui.cpp
Outdated
point start_position = point( ( inv_column_width + border_width + 1 ), | ||
get_header_height() + 1 ); | ||
|
||
draw_item_info( catacurses::newwin( height, width, start_position ), data ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just override the create_or_get_ui_adaptor
function and move the item info window creation and drawing code to the callbacks there. Or, just pretend the item info window is an overlay instead of a part of the inventory menu, and create a separate ui_adaptor
for it. Do note that the game window may resize while get_input
is blocking for input, so redrawing it after ui_manager::redraw
instead of in a redraw callback will make it resize incorrectly.
src/inventory_ui.cpp
Outdated
if( selected ) { | ||
return selected.any_item(); | ||
} | ||
} else if( input.action == "HIDE_CONTENTS" || input.action == "SHOW_CONTENTS" ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the unexpected behavior exactly?
I've moved drawing to a ui_adaptor-based setup, and things have checked out fine on my end. The examine screen now draws correctly when filtering/resizing. Some of the setup is still a bit clumsy, but it seems to work. The keybinding is now 'o' for examining contents. I also fixed a bug where the item detail scroll rate was based on the size of the item list, and could thus be negative. |
src/inventory_ui.cpp
Outdated
|
||
const inventory_input input = get_input(); | ||
|
||
//Basically anything the player can do at this point will invalidate the examine screen | ||
ui_examine.invalidate_ui(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ui_manager::redraw()
always invalidates the topmost ui_adaptor
before redrawing. ui_manager::redraw_invalidated
can be used if you want to explicitly mark ui_adaptor
for redrawing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a specific sequence you recommend for this? My testing for this was a fun cycle of "the main window draws, but it's not calling on_redraw for ui_examine"->"ui_examine redraws, but the main window doesn't anymore"->"The main window is back, but ui_examine only shows up after user input"->etc.
I remember something going wrong when I didn't invalidate ui_examine, but I'd have to go back and test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Never mind. It turns out that line was a holdover from a previous iteration where ui_examine was created before calling create_or_get_ui_adaptor. I've removed that line, but left myself a comment at the redraw() call so I remember the thought process.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered. |
Is this ready @ZeroInternalReflection ? |
I considered it to be functional as of the last update, and the admittedly-limited testing I'd done had worked correctly. |
… inventory_selector
The previous keybinding of 'c' required a change to existing keybindings in the AIM.
Item detail scroll rate was linked to terminal height, and thus should updated on_screen_resize() rather than in ::execute(). Invalidation of ui_examine was not required since it was the most recently created ui_adaptor. Renamed inventory_examiner::action_examine(item_loc,bool) to draw_item_details(item_loc), since, after refactoring, it wasn't really related to inventory_selector::action_examine anymore.
213a2c3
to
2db305a
Compare
The commit history for this PR is getting to be a bit of a mess, but I've rebased to the latest version. This mostly resolves the issues with showing/hiding contents (see discussion with andrei8I above), and also brings the new 'g'et menu into the fold. |
Moving my conversation with @andrei8l above to the main thread: Of course, having the ability to hide items within this menu introduces a couple of other fun edge cases:
|
This is just a matter of repaging on refresh (or similar) since collapse status is implemented as a pocket setting. Most likely overkill though and probably out of scope for this PR.
That's because
Move (most of ) the functionality of |
Ensuring the paging was rechecked in inventory_selector after hiding something in inventory_examiner didn't turn out to be too complicated, so that's been implemented, hopefully solving the first item I raised above. For the second item, I did implement a way to expand/collapse the item you're looking at. I'm not a fan of the partial duplication of set_collapsed(), but I felt the cleanest implementation was for inventory_examiner to only look one level deep when expanding. Otherwise, it would need to remember the status of nested items. So, if you examine c'o'ntents of an item that you've collapsed, it'll show you the contents, but any nested containers will still be collapsed. I think that's a reasonable solution. For the third item, I think I agree that changing the showing/hiding behaviour of things like installed batteries is probably outside the scope of this PR. It'll be a confusing quirk, but it should be a fairly specific corner case now (an item with an installed battery/etc. inside a collapsed container inside the container you're actually looking at). |
Summary
Interface "Add examine contents menu to inventory menus"
Purpose of change
As raised in #50887, when looking at items in the player's inventory, 'e'xamining them will describe the container prior to describing the contents. For many items, this is logical (e.g. describing the mp3 player before describing the batteries). However, for other items, such as a bottled drink or a sealed can of food, the player is interested in the contents, and has to scroll to find that information.
Describe the solution
This branch adds a new inventory menu derived from inventory_selector that is set up to examine the contents of the selected item. This can be pulled up from the inventory menu (or the drop menu, the pickup menu with #51033, or the AIM). It's currently pulled up with 'o'. For an item that has no contents, the behaviour is equivalent to 'e'xamining. For an item with only one thing inside (a bottled drink), it looks like this:

But the menu also handles items containing multiple other items:

And can be called recursively.

Describe alternatives you've considered
Adding a new, independent class for displaying item contents:
See some of the earlier commits of this pull request. Eventually, proper sorting/collapsing/filtering code just added up to too much duplication of what inventory_selector already does.
Changing the behaviour of 'e'xamining items in the inventory to focus on contents:
This would require some judgment calls on when the player is likely to care more about the container or the contents, and I can see it constantly feeling like we guessed wrong.
Quicker navigation on the 'e'xamine screen to jump to the contents:
Probably of benefit, but might still not provide all of the information the player is looking for.
Testing
I've tested the behaviour of the menu with a variety of items. Empty, full of items, one item, full of items filtered to nothing, etc.
I have not tested the menu with any language other than English.
The menu looks reasonable on my computer monitor, but might not work well on different screen sizes/shapes.
Additional context
(This text was updated 2021-09-08 & 2021-09-10 & 2021-11-05based on changes to the PR)
Some notes on keybindings:
I've set it to use 'o' in both inventory_selector menus and the AIM:
Scrolling the item details uses Page Up/Page Down, where in the main inventory screen, that scrolls through the item list. I'd have preferred to use "<"/">", but those are in use in inventory_selector for show/hide contents.