[CR] Filter in vehicle menu for part installation #13341

Maykeye · 2015-08-18T08:27:21Z

This PR adds ability to filter parts for iinstallation in vehicle interaction menu.

Displaying query for filtering caused nasty artifacts (windows were not redrawn nor in console, not in tiles, there were black spaces) so I've updated curses port to redraw them and patched the same artifacts that appeared after closing help '?'.

I've tested it on linux with tiles on and off, can't say if behaves properly on windows.

BevapDin · 2015-08-18T10:50:28Z

src/cursesport.cpp

+        return 0;
+    }
+    win->draw = true;
+    for (int i = 0; i < win->height; i++) {


Instead of repeating the loop, why not call clearok?

Because it doesn't work.

clearok updates stdscr without touching win and I don't know if it's intended or not, but calling it doesn't fix black screen issues, while looping does.

(Oh, there's also a problem with clearok that its proper declaration is int clearok(WINDOW *win, bool bf);, not clearok(WINDOW *win) - but since veh_interact.cpp doesn't call it, I left it as is)

clearok updates stdscr without touching win and I don't know if it's intended or not, but calling it doesn't fix black screen issues, while looping does.

I did not see that, it looks like a bug. It's only used inside of wclear, which in turn is only used by clear in cursesport.cpp, it's a mess and it's probably not worth touching it.

The implementation of redrawwin is fine.

drbig · 2015-08-19T07:57:09Z

Regarding redrawwin - I wonder if it could help with: 1) drawing the deathcam message in tiles, 2) drawing the "now saving" popup in tiles.

Maykeye · 2015-08-19T11:56:57Z

@drbig, popup is actually separated issue caused by double buffering. cursesport updates out of screen buffer without actually outputting anything to the screen. When game ends saving files, it redraws game screen(overwrites out of screen buffer once more), then after a while back buffer is outputted to the screen but popups are already gone.

Night-Pryanik · 2015-08-19T14:52:53Z

Will it be possible solution for this (#11706)?

Maykeye · 2015-08-19T15:19:34Z

@Night-Pryanik No. It simply adds possibility to filter vehicle parts by name.

It doesn't change the actual installation process where parts are removed from inventory and attached to the vehicle, only UI which happens before the actual installation begins.

BevapDin · 2015-08-28T15:29:30Z

src/game.cpp

+/*
+ * case insensitive string::find( string::findstr ). findstr must be lowercased
+ */
+bool lcmatch(const std::string &str, const std::string &findstr)


Is there some specific reason why you moved this function around? game.cpp isn't really a good place for it, it has nothing to do with the game itself.

While ui.cpp may not be the best, it kind of fits there as it is mostly used for UI related stuff. Alternatively, it could be in in output.cpp or in catacharset.cpp (the function is btw. not Unicode-compatible).

I moved declaration to game.h so every second .cpp file would stop redeclaring it. And since it was moved to game.h for that, it might be defined in game.cpp as well.

I originally moved it to ui.h, but most files that used the function didn't include ui.h so it was not that much better(which I forgot to uninclude - it doesn't make sense to include entire ui.h for simple lcmatch). I din't find general helper functions header or file so moved to game.h which is included everywhere

"Because it's already included everywhere" is pretty much the worst reason to put a function in a particular file, the fact that everything includes game.h already is a major problem that we're trying to fix, and this makes the code go the wrong way.

Well, it doesn't feel like UI. It's string function. Should I make new header or there more appropriate header?

Maykeye · 2015-08-30T12:31:11Z

Rebased to fix unmergeability.

Coolthulhu · 2015-09-19T20:49:08Z

This PR didn't have updates for a while.

Is there a major problem with it that prevents merging?

It looks quite useful, as out vehicle part menu got quite bloated.

BevapDin · 2015-09-20T09:52:17Z

I'm not convinced that letting the input context redraw the windows is a good solution. In my opinion, the caller (the loop that calls input_context::handle_input) should detect that the key bindings menu had been opened (which is already possible, just check for the action "HELP_KEYBINDINGS") and it should then redraw the screen.

This is an important difference when the normal drawing code displays the current key bindings. If the bindings were changed and the input context only calls the curses redraw-functions, it will draw the current buffer, which will still contain the information about the old key bindings. That information must be updated by the caller as only the caller knows where to print it.

Maykeye · 2015-09-24T15:50:54Z

Just checking for HELP_KEYBINDINGS bloats code too much:

veh_interact.cpp for example has five instances of main_context.handle_input().

Adding special case for HELP_KEYBINDINGS also spreads windows information around code: most windows are created near input_context so if e.g. new window was added, it's much easier to add window to list of windows to redraw, than to remember where windows are actually updated.

If handler needs more than curses functions, it can check action just as input_context::display_help does already. In most cases it is not necessary.

Filter filters parts by name Updated curses port to behave more like curses * Added redrawwin * Added handling of null pointer to wrefresh Moved lcmatch to game.cpp and game.h Function lcmatch was declared too many times in too many files. Since all of them included game.h already and including ui.h made little sense just for lcmatch, moved it to game.* Workaround for popup in tiles In several places in tiles build popup didn't appear for the following reason: Curses port writes to back renderer but the actual screen is not updated anywhere in cursesport.cpp. It's updated from renderer in try_update which is called from message handling routine. If screen was rewritten before message processed started again, then no matter how many calls to wrefresh were done, they were not actually printed to the screen. Only backscreen renderer changed. Update wrefresh to update actual renderer. Note that it's not placed inside of `if (win->draw)`. In e.g. when popup() calls refresh mainwin->draw is false. Refactored input_context to keep track of windows Now after input_context corrupts screen after HELP_KEYBINDING, it redraws windows that were assigned to it. As result, it fixes CleverRaven#8514, fixes CleverRaven#9835 in * Character menu (in-game) * Construction menu * Character creation menu * World creation menu * Vehicle interaction menu

sparr · 2015-12-17T04:13:48Z

Am I correctly interpreting the discussion here that the only thing wrong with the original feature is that our current window-redrawing code can't handle it, and no one likes the various possible solutions to that problem?

Conflicts: src/crafting.cpp src/game.cpp src/input.cpp src/newcharacter.cpp

Maykeye · 2016-01-16T17:44:40Z

@sparr
Essentially yes. Currently there is no good way which works both in tiles and curses to redraw screen after it was corrupted by window after it was destroyed.

Maykeye · 2016-03-18T15:28:11Z

Out of sync

BevapDin reviewed Aug 18, 2015
View reviewed changes

BevapDin reviewed Aug 28, 2015
View reviewed changes

Maykeye force-pushed the veh_menu branch from 4234e4d to 0a47ffb Compare August 30, 2015 12:09

Maykeye force-pushed the veh_menu branch from 0a47ffb to 521b86a Compare October 25, 2015 06:32

Maykeye force-pushed the veh_menu branch from 521b86a to 9cc8371 Compare October 29, 2015 02:47

Merge branch 'master' into veh_menu

a4fcc90

Conflicts: src/crafting.cpp src/game.cpp src/input.cpp src/newcharacter.cpp

Maykeye closed this Mar 18, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CR] Filter in vehicle menu for part installation #13341

[CR] Filter in vehicle menu for part installation #13341

Maykeye commented Aug 18, 2015

BevapDin Aug 18, 2015

Maykeye Aug 18, 2015

BevapDin Aug 18, 2015

drbig commented Aug 19, 2015

Maykeye commented Aug 19, 2015

Night-Pryanik commented Aug 19, 2015

Maykeye commented Aug 19, 2015

BevapDin Aug 28, 2015

Maykeye Aug 28, 2015

kevingranade Aug 31, 2015

Maykeye Sep 5, 2015

Maykeye commented Aug 30, 2015

Coolthulhu commented Sep 19, 2015

BevapDin commented Sep 20, 2015

Maykeye commented Sep 24, 2015

sparr commented Dec 17, 2015

Maykeye commented Jan 16, 2016

Maykeye commented Mar 18, 2016

[CR] Filter in vehicle menu for part installation #13341

[CR] Filter in vehicle menu for part installation #13341

Conversation

Maykeye commented Aug 18, 2015

BevapDin Aug 18, 2015

Choose a reason for hiding this comment

Maykeye Aug 18, 2015

Choose a reason for hiding this comment

BevapDin Aug 18, 2015

Choose a reason for hiding this comment

drbig commented Aug 19, 2015

Maykeye commented Aug 19, 2015

Night-Pryanik commented Aug 19, 2015

Maykeye commented Aug 19, 2015

BevapDin Aug 28, 2015

Choose a reason for hiding this comment

Maykeye Aug 28, 2015

Choose a reason for hiding this comment

kevingranade Aug 31, 2015

Choose a reason for hiding this comment

Maykeye Sep 5, 2015

Choose a reason for hiding this comment

Maykeye commented Aug 30, 2015

Coolthulhu commented Sep 19, 2015

BevapDin commented Sep 20, 2015

Maykeye commented Sep 24, 2015

sparr commented Dec 17, 2015

Maykeye commented Jan 16, 2016

Maykeye commented Mar 18, 2016