-
Notifications
You must be signed in to change notification settings - Fork 491
orders: add search overlay for manager orders #5677
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
base: develop
Are you sure you want to change the base?
Conversation
Adds search overlay to find and navigate manager orders with arrow indicators showing current search result. Search uses Alt+S to focus, Alt+P/N for prev/next navigation. Overlays are disabled by default.
ChrisJohnsen
left a comment
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 is a nice workaround for the inability to filter the view like in the other info panel search widgets.
I like the labeled magic numbers in OrderHighlightOverlay. DF's layout behavior is not fun thing to have to replicate, but I think this would be fairly clear to someone that hasn't looked at this stuff as much (at least as long as they have DF at hand to try out different window sizes).
Re: not being able to detect exiting the window: yeah, I have also wanted something
like a callback for "this overlay is no longer active (not going to be rendered)".
- Would utils.search_text be useful here? The main SortOverlay uses it for searching.
- Could the first match automatically be highlighted on Enter?
I can see how you might not want to highlight (thus likely move the scroll position) for each change in search input, but Enter seems like a nice place to jump to the first match. - The highlight should probably be cleared when the search text changes (especially when the highlighted order does not match the new search input).
- It might just be my color vision being flaky, but I completely did not notice the highlight arrow at first. Now that I know what to look for, it isn't hard to spot. But my first cycling through the matches was confusing because it wasn't obvious which order was being indicated.
Now there is 16 visible chars in search
I switched to using utils.search_text instead of the custom search logic
Added Enter/Shift+Enter to cycle through matches using the default submit/submit2 methods.
Fixed, now the highlight gets cleared whenever the search text changes.
I reshaped the arrow, moved it to the right side of icon and used more contrasting colors (black on white) to make it more visible. @ChrisJohnsen Thanks for all the detailed feedback! I made each change in a separate commit to make the review easier. Let me know if there's anything else that needs adjusting. |
ChrisJohnsen
left a comment
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.
Good spotting of the need to hide when job_details.open. The other functionality changes all seem good (with a new note about order deletion handling).
I've put some more thoughts on the specifics of the code in this review.
| -- Shared state for search cursor visibility | ||
| local search_cursor_visible = false | ||
| local search_last_scroll_position = -1 | ||
| local order_count_at_highlight = 0 |
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.
The first two of these are shadowed later by the locals declared at the top of the OrdersSearchOverlay section. Probably all these search/highlight locals should just be consolidated in that section.
| for i = 0, #orders - 1 do | ||
| local order = orders[i] | ||
| local search_key = get_order_search_key(order) | ||
| if search_key and utils.search_text(search_key, text) then |
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.
As it currently is, get_order_search_key has the following returns:
- some non-falsy value from the reaction map
- an empty string
(it can also throw an error if the reaction map couldn't be loaded)
All of the return values are currently "truthy", so they won't affect the if condition here.
Searching an empty search_key will only "succeed" when it is searched for an empty string, which won't happen here because of the guard before the loop. So it all works, but it indicates a misalignment with the definition of get_order_search_key.
Probably it makes the most sense to keep this check and change get_order_search_key to return nil if the reaction map isn't available (guarding against a potential "attempt to index a nil value" error there) or if the map didn't have an entry for the order.
| self.cached_list_start_y = nil | ||
| self.cached_viewport_size = nil | ||
| self.cached_screen_width = nil | ||
| self.cached_screen_height = nil |
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.
None of these appear to be used anywhere?
| self.ORDER_HEIGHT = 3 | ||
| self.TABS_WIDTH_THRESHOLD = 155 | ||
| self.LIST_START_Y_ONE_TABS_ROW = 8 | ||
| self.LIST_START_Y_TWO_TABS_ROWS = 10 | ||
| self.BOTTOM_MARGIN = 9 | ||
| self.ARROW_X = 10 |
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.
These constants don't need to be fields of the overlay. Probably just a collection of module-locals at the top of the OrderHighlightOverlay section would be fine.
| self.ARROW_FG = COLOR_BLACK | ||
| self.ARROW_BG = COLOR_WHITE |
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.
These values can probably just be inlined into the dfhack.pen.parse call. The table key names there will establish the fg/bg intentions.
| function OrderHighlightOverlay:render(dc) | ||
| if mi.job_details.open then return end | ||
|
|
||
| if search_cursor_visible then |
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.
The indentation isn't getting too bad here, but this might be better as a early-returning guard instead of having the rest of the body of the function tucked into the if block. It makes it clear that nothing else is going to happen for the guarded condition.
E.g.
if not search_cursor_visible then return end| return selected_y | ||
| end | ||
|
|
||
| function OrderHighlightOverlay:render(dc) |
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.
It is a good idea bring back an unconditional call to the super render method here. It isn't technically necessary since there is nothing else that needs to be drawn in this overlay (e.g., a frame, sub-widgets, or onRenderFrame/onRenderBody methods), but having it can prevent hard-to-guess-at "why isn't this stuff isn't being drawn?" problems if any of those are added in the future.
| encrust_str) | ||
| end | ||
|
|
||
| local function build_reaction_map() |
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.
build_reaction_map could be inlined into get_cached_reaction_map (the only caller). I don't think there is any need to have the non-caching version available. An early return (if the map is already cached) can be used to limit the nesting in get_cached_reaction_map.

Add search to manager orders
Adds search box to find orders by job name + arrow indicators to show which one you're looking at.
What it does
Implementation
OrdersSearchOverlayfor search UI,OrderHighlightOverlayfor arrowsimportexportoverlay position to make room for search (and make new version of config)What didn't work
Related issues
order-search.mp4