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

Migrate inventory_ui.cpp and game_inventory.cpp to ui_adaptor #40094

Merged
merged 1 commit into from
May 4, 2020

Conversation

Qrox
Copy link
Contributor

@Qrox Qrox commented May 3, 2020

Summary

SUMMARY: Interface "Migrate inventory_ui.cpp and game_inventory.cpp to ui_adaptor"

Purpose of change

Migrate more UIs to ui_adaptor.

Describe the solution

Encapsulate inventory ui initialization and redrawing with ui_adaptor.

Testing

Opened and closed sub-UIs (such as keybindings menu), and lower UIs were redrawn correctly. (If the inventory menu is opened from unmigrated UIs, then lower UIs are not redrawn because they are disabled, which is expected and will be fixed once the unmigrated UI is migrated).

Resized the game window with or without sub-UIs open. Resized the game window when entering the filter string. Resizing was working as intended apart from a small glitch (see additional context below).

inventory_pick_selector: tested with inventory menu ('i')
inventory_compare_selector: tested with item compare menu ('I')
inventory_iuse_selector: tested by using washboard ('a'->select wash borad)
inventory_drop_selector: tested with drop menu ('d')
Also tested eating menu to make sure filter and selection is preserved after eating once.

Additional context

Due to the way the UI initialization code was written, there are some inconsistency when you resize the window multiple times. For example, if multiple columns are present and window width is reduced, the columns can be merged into a single column, but then remains a single column even if the window is widened again. I've fixed a different case where truncated cell does not reset its width when the window is widened by resetting the column width before re-initializing the UI, but the former case seems to be trickier to tackle because the logic in inventory_selector::rearrange_columns destroys the column information, so I could not come up with a fix.

There's also another bug with filtering that existed before this PR. In instances of inventory_multiselector (for example item compare menu 'I' or multidrop menu 'd'), if a filter is set after toggling some of the items, the toggled status of these items resets but the selection column is not cleared. This is because inventory_column::prepare_paging filters out entries by removing them from entries, and removes the toggled status in the process. I do not entirely understand how filtering works in inventory_ui so I could not come up with a fix.

@codemime it seem that you are the original author of the inventory UI system, so perhaps you could provide some insights on how to handle these issues?

Comment on lines 993 to 1016
for( const inventory_column *const col : all_columns ) {
if( !dynamic_cast<const selection_column *>( col ) ) {
for( const inventory_entry *const ent : col->get_entries( always_yes ) ) {
expand_to_fit( *ent );
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clang-tidy doesn't like this:

/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/inventory_ui.cpp:995:53: error: Called C++ object pointer is null [clang-analyzer-core.CallAndMessage,-warnings-as-errors]
            for( const inventory_entry *const ent : col->get_entries( always_yes ) ) {
                                                    ^
/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/inventory_ui.cpp:993:10: note: 'col' initialized here
    for( const inventory_column *const col : all_columns ) {
         ^
/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/inventory_ui.cpp:994:13: note: Assuming 'col' is null
        if( !dynamic_cast<const selection_column *>( col ) ) {
            ^
/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/inventory_ui.cpp:994:9: note: Taking true branch
        if( !dynamic_cast<const selection_column *>( col ) ) {
        ^
/home/runner/work/Cataclysm-DDA/Cataclysm-DDA/src/inventory_ui.cpp:995:53: note: Called C++ object pointer is null
            for( const inventory_entry *const ent : col->get_entries( always_yes ) ) {
                                                    ^

@ifreund ifreund added [C++] Changes (can be) made in C++. Previously named `Code` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Info / User Interface Game - player communication, menus, etc. labels May 3, 2020
@Qrox Qrox force-pushed the migrate-ui-12 branch from 5f28a0b to 92c76c8 Compare May 4, 2020 00:27
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` Code: Infrastructure / Style / Static Analysis Code internal infrastructure and style Info / User Interface Game - player communication, menus, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants