Skip to content
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

Reimplement trading UI #52572

Merged
merged 8 commits into from
Nov 14, 2021
Merged

Reimplement trading UI #52572

merged 8 commits into from
Nov 14, 2021

Conversation

andrei8l
Copy link
Contributor

@andrei8l andrei8l commented Oct 31, 2021

Summary

Interface "reimplement trading UI"

Purpose of change

The trade UI is cumbersome and lacks many features compared to other item UIs
Fixes: #42363

Describe the solution

Reimplement the trading UI using two instances of inventory_drop_selector held together by a thin event queue.

Partially revert #51704 since the container issue is now handled differently and directly from the selection UI with the same behaviour as in the drop UI.

Describe alternatives you've considered

Using the AIM or parts of it: this approach was rejected twice

Testing

Trade with a neutral NPC. Trade with an NPC shopkeeper. Trade with an allied NPC while player items are on the floor. There should not be any surprises either when selecting items or after trading.

Trade for item stacks. Trade for items with charges. Trade for items from the floor and vehicles. Select a lot of items from the floor, then cancel the trade, then trade again for fewer items. This is to ensure that the changes to npctrade did not affect functionality.

Insert an unwanted item in a container (ex: vac_oven_small_full) and trade away the container. The balance and prices should not include the unwanted item. The unwanted item should spill before the container is traded.

Additional context

screenshot

std

screenshot smallest screen

small

If you're reviewing by commit

please note that
838b281 trade_ui: reimplement trading ui
duplicates some code that's later removed in
e5135ae npctrade: nuke trading_window

68fd32b inventory_ui: add support for fixed window sizes slightly changed the aspect of inventory_examiner (#51252), but the layout should be the same

@andrei8l andrei8l force-pushed the trade_ui branch 2 times, most recently from bd7e0cf to 7d08e1c Compare November 1, 2021 05:43
@BrettDong BrettDong added [C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. NPC / Factions NPCs, AI, Speech, Factions, Ownership labels Nov 1, 2021
@PatrikLundell
Copy link
Contributor

If it's easy to do, I'd suggest placing worn items at the bottom of the list, since those items are the ones you're least likely to want to trade away.

@andrei8l

This comment has been minimized.

@andrei8l andrei8l force-pushed the trade_ui branch 3 times, most recently from e136402 to 5da6f9f Compare November 3, 2021 18:09
@andrei8l andrei8l marked this pull request as ready for review November 3, 2021 18:18
@andrei8l andrei8l force-pushed the trade_ui branch 2 times, most recently from a8e0a71 to 52b9af6 Compare November 5, 2021 10:07
split input processing from the main loop so it can be reused
@ZeroInternalReflection
Copy link
Contributor

I have not looked at the trade ui side of this, but I did test out the impact on inventory_examiner. Unfortunately, while your implementation of forcing the window size is a lot tidier than my (admittedly hackish setup) was, it results in things getting a bit cramped in the header and item list of inventory_examiner:
Examiner_Screen

I think the header does need to be full-width, and there needs to be something forcing the first column to use the available space. I'd recommend changes along these lines:
inventory_examiner.txt
(If you'd like, I can add the changes line-by-line in a review instead of just the attached patch file, but this seemed simpler)

@andrei8l
Copy link
Contributor Author

The column in inventory_examiner getting truncated is almost certainly a bug in prepare_layout(). Forcing the width to 1/3 of TERMX is just a hack that works well only on certain screen sizes. This small change seems to fix it but I'm not too interested in investigating atm:

patch
diff --git a/src/inventory_ui.cpp b/src/inventory_ui.cpp
index 3f2396b1fc..b945e60ebd 100644
--- a/src/inventory_ui.cpp
+++ b/src/inventory_ui.cpp
@@ -1694,7 +1694,7 @@ void inventory_selector::prepare_layout( size_t client_width, size_t client_heig
     // If we have a single column and it occupies more than a half of
     // the available with -> expand it
     auto visible_columns = get_visible_columns();
-    if( visible_columns.size() == 1 && are_columns_centered( client_width ) ) {
+    if( visible_columns.size() == 1 ) {
         visible_columns.front()->set_width( client_width, columns );
     }

For the inventory_examiner header: I'd rather just remove it since it's not pertinent to the examined container (it's just the default inventory_selector header). Neither of these are the focus of this PR so I most likely won't address them unless they become blockers. Sorry!

@kevingranade kevingranade merged commit fcba0b1 into CleverRaven:master Nov 14, 2021
@andrei8l
Copy link
Contributor Author

Thanks for merging!

@andrei8l andrei8l deleted the trade_ui branch November 14, 2021 04:32
@wapcaplet
Copy link
Contributor

wapcaplet commented Nov 14, 2021

There are a number of open bugs related to the previous trading UI that may be resolved or obsoleted by this as well. A few I found are #41393, #43239, #45436, #46081, #46879, and there are surely others. I’ll try to retest a few tomorrow and close them if I can - I’m really looking forward to trying this feature out!

@ProfoundDarkness
Copy link
Contributor

If anyone out there is like me and kept tab for 'toggle category selection mode' and thus get confused about how to switch over to sell (your inventory) instead of buy (their inventory, default)...

Check your keybindings ("?") for "Switch lists" and rebind that to something other than tab... or rebind category selection mode (which will probably change the key in other inventory screens).

I've opted to rebind "Switch lists" to numpad plus (nice big target not far from arrow keys). Don't know if it will bite me in some way later. I was going to bind it to left arrow but that conflicted with "switch columns" which is also an old hold over I kept.

Also thanks for the new interface!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` Info / User Interface Game - player communication, menus, etc. NPC / Factions NPCs, AI, Speech, Factions, Ownership
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Trading interface needs improvement.
7 participants