add gui/workorder-details.lua: adjusts input item, material, traits #358

TymurGubayev · 2022-03-26T13:13:15Z

Adjust input items, material, or traits for work orders. Actual jobs created for it will inherit the details.

This is the equivalent of gui/workshop-job for work orders, with the additional possibility to set input items' traits.

F.e., we can make a work order to "Cut unknown material":

Which then appears like this in the workshop:

It has to be run from a work order's detail screen (:kbd:j-m, select work order, :kbd:d), because the game initializes work order's .items array as soon as the screen is opened (and then copies it over to the job).

For best experience add the following to your dfhack*.init::

keybinding add D@workquota_details gui/workorder-details

Adjust input items, material, or traits for work orders. Actual jobs created for it will inherit the details.

myk002

Overall, looks solid. Thanks!

A couple questions:

instead of copying code, can you reqscript('gui/workshop-job') and call any existing functions/inherit from existing classes directly?
instead of workorder-finetune, how about workorder-details or workorder-edit?
could you add some tests in the test/gui directory to validate that the code continues to work when DF or DFHack behavior changes? This will help a lot to identify code that breaks on the upcoming steam version of DF
could you add a changelog entry, under new scripts?

myk002 · 2022-03-26T18:49:31Z

Also, does it make sense to be able to adjust order quantities or frequency with this script?

TymurGubayev · 2022-03-26T20:45:11Z

Also, does it make sense to be able to adjust order quantities or frequency with this script?

IMO this would be too deep in the details: from the work order list view d for DF details and another d for this script; adjusting frequency or quantities would be better on the list view itself.

TymurGubayev · 2022-03-26T20:52:47Z

instead of workorder-finetune, how about workorder-details or workorder-edit?

I definitely don't like -edit because it suggests a complete editor, which this script isn't, it only manipulates a small part of a work order.

I am not quite sure about -details, because there is already one workorder details view - the game's workquota_details view where we land after pressing d in the j-m view. And here we have "additional details" (or "fine details"). Maybe something like workorder-details-plus? I'm not sure.

This allows to reuse previously duplicated code from `gui/workshop-job.lua`. Also, minor wording changes and comments.

TymurGubayev · 2022-03-27T11:00:48Z

instead of copying code, can you reqscript('gui/workshop-job') and call any existing functions/inherit from existing classes directly?

did that. Had to modify (very minorly) the gui/workshop-job.lua. Inheritance won't work (I think) because it's defclass(JobDetails, guidm.MenuOverlay) vs defclass(JobDetails, gui.FramedScreen), but just assigning functions works and did eliminate a huge chunk of duplicated code.

gui/workorder-finetune

myk002 · 2022-03-29T00:21:09Z

gui/workorder-finetune

+JobDetails.canChangeIType = wsj.JobDetails.canChangeIType
+JobDetails.setItemType = wsj.JobDetails.setItemType
+JobDetails.onChangeIType = wsj.JobDetails.onChangeIType
+JobDetails.canChangeMat = wsj.JobDetails.canChangeMat
+JobDetails.setMaterial = wsj.JobDetails.setMaterial
+JobDetails.findUnambiguousItem = wsj.JobDetails.findUnambiguousItem
+JobDetails.onChangeMat = wsj.JobDetails.onChangeMat


Stealing individual methods from a class, while technically functional, could lead to issues later if wsj is updated with undesired behavior. Could you refactor wsj.JobDetails so that the common functions are in a superclass? Then that class could be clearly marked as "shared" and both the JobDetails here and the JobDetails in wsj could inherit from that common class.

Is there a convention how such a shared class should look like? I think to call it JobDetailsBase and place in /scripts/gui/.

JobDetailsBase sounds good. It can either stay in wsj (commented to indicate that it is shared across scripts) or it can go in internal/gui/. It shouldn't go in a new file in gui/ because then it would show up in ls as an independent script

so, I started to actually do this and it would be a huge refactoring. The problem is one class inherits from MenuOverlay while another from FramedScreen. We need multi-inheritance or to change both scripts in such a way most of the rendering happens inside a widget, then JobDetailsBase can inherit from Widget. But this is IMO too much work for little benefit. Should we actually have the class inheritance in place, any change in the base class that's undesired in one of the scripts will still need to be addressed. The only thing we win is obviously marking those functions as shared - maybe we could just leave a comment? "Beware: this script shares functions with worder-details.lua" or something.

gui/workorder-finetune

TymurGubayev · 2022-04-03T14:33:06Z

could you add some tests in the test/gui directory to validate that the code continues to work when DF or DFHack behavior changes? This will help a lot to identify code that breaks on the upcoming steam version of DF

Started on that. There is currently some ugliness involved, f.e. I couldn't figure out how to emulate typing: gui.simulateInput(dfhack.gui.getCurViewscreen(true), 'CUSTOM_C') doesn't do anything on the manager new order screen.

Also, the test file-name has underscore instead of hyphen because test -dhack/scripts/devel/tests -tworkorder-finetune doesn't work. It looks like this:

[DFHack]# test -dhack/scripts/devel/tests -tworkorder-finetune
Loading tests from hack/scripts/devel/tests
Loading file: /a-b.lua
Loading file: /dialogs.lua
Loading file: /workorder-finetune.lua
Filtering tests by name pattern:
workorder-finetune
Selected tests: 0/3
Running 0 tests

Test summary:
0/0 tests passed
0/0 checks passed
0 test files failed to load
0 tests in unreachable modes

myk002 · 2022-04-03T15:43:53Z

You have to use the actual key code to navigate the df menus. Look in data/init/interface.txt and see which mapping of 'c' to use

myk002 · 2022-04-03T15:47:46Z

Not matching hyphens sounds like a bug in the test harness. I'll get that fixed.

myk002 · 2022-04-06T03:46:28Z

Not matching hyphens sounds like a bug in the test harness. I'll get that fixed.

Actually, the test harness is working as designed. The problem is that the -t param takes a pattern, not a literal. You need to escape the hyphens in the script name:
test -tworkorder%-finetune

Or just test -tfinetune would do it too

myk002

I'd like to test in-game a bit before merging, but code looks good. Thanks for the test!

myk002 · 2022-04-06T04:09:06Z

Could you add a changelog entry for this?

TymurGubayev · 2022-04-06T08:30:58Z

Could you add a changelog entry for this?

I've not forgotten about that, there is a dependency graph in my head: find out if the hyphen is a problem for tests (✔), decide on the script name (will probably go with workorder-details), then add the changelog entry.

myk002 · 2022-04-06T12:58:21Z

👍

TymurGubayev · 2022-04-13T16:18:51Z

You have to use the actual key code to navigate the df menus. Look in data/init/interface.txt and see which mapping of 'c' to use

under "actual key code" do you mean f.e. D_JOBLIST instead of j to open the job list? I figured that out, what I can't figure out is how to type something so that the game would apply the filter?

myk002 · 2022-04-14T01:37:07Z

what I can't figure out is how to type something so that the game would apply the filter?

Yeah, that's tricky. I did something like that here:

scripts/test/gui/blueprint.lua

Line 371 in a1df858

view:onInput({_STRING=string.byte('h')})

You essentially have to call the onInput method directly

TymurGubayev · 2022-04-15T12:57:57Z

what I can't figure out is how to type something so that the game would apply the filter?

Yeah, that's tricky. I did something like that here:

scripts/test/gui/blueprint.lua

Line 371 in a1df858

view:onInput({_STRING=string.byte('h')})

You essentially have to call the onInput method directly

I think this only works when view is dfhack UI, but I need to send input to the result of dfhack.gui.getCurViewscreen(true), which doesn't have the onInput method - unless I'm missing something.

myk002 · 2022-04-15T15:38:47Z

I think this only works when view is dfhack UI, but I need to send input to the result of dfhack.gui.getCurViewscreen(true), which doesn't have the onInput method - unless I'm missing something.

Sorry about that -- I thought this was one of our filters.

Finding the keycode for each key such that the viewscreen will accept it would work, but would be messy and arduous. Quickfort has an API that might be useful here. For example, to enqueue a 'cut slade' job, you could use this:

local quickfort = reqscript('quickfort')

function enqueue_job(name)
    local keys = 'jmq' .. name .. '&1&^^'
    local blueprint_data = {[0]={[0]={[0]=keys}}}
    quickfort.apply_blueprint{mode='config', data=blueprint_data}
end

enqueue_job('cut slade')

The key sequence enters the jobs -> manager -> q new order screen, types in the requested job name, presses Enter, puts in a quantity of 1, hits Enter again, and hits Escape twice to get back to the main map screen. If you're not familiar with quickfort blueprints, & maps to {Enter} and ^ maps to {ESC}. See here for more info. Quickfort could be used to drive the entire UI flow, if you wish.

Specifically for creating orders, you could also use the orders plugin to import the order that you need directly (see test/plugins/orders.lua for an example -- the code there for saving and restoring existing manager orders might be useful too so you can guarantee that your test won't make lasting changes to an active game)

edit: now that #372 is merged, if you pull latest master, you can simplify the above function to:

function enqueue_job(name)
    local keys = 'jmq' .. name .. '&1&^^'
    quickfort.apply_blueprint{mode='config', data=keys}
end

myk002

Could you merge latest master so everything is up to date with the latest code?

myk002 · 2022-06-03T21:05:24Z

ping on this

TymurGubayev · 2022-06-06T13:02:33Z

about local quickfort = reqscript('quickfort') in test: I don't like the idea of requiring another script or plugin just for the test to run. Any chance you could move the functionality to the dfhack-proper?

myk002 · 2022-06-06T23:31:10Z

It would be possible to factor some functionality out, but I'm not sure if there's anything shady about using the public API of quickfort to implement a test. It is used by other scripts (e.g. assign-minecarts) to perform similar utility functions. Interdependencies among the scripts is starting to become more common. (especially with quickfort, which has its own ecosystem test harness)

test/gui/workorder-details.lua

gui/workorder-details.lua

changelog.txt

gui/workorder-details.lua

myk002 · 2022-06-20T22:40:38Z

test/gui/workorder-details.lua

+            if c.enabled then
+                confirmRemove = function()
+                    wait()
+                    -- without delays `confirm` can miss the key event


are you sure? I have not found this to be the case in the past.

it happens sporadically, when it does, removing order fails and test doesn't exit to map screen (and test fails at the Test order should've been removed-check). With delay(5) it happened once the first time I startet DF after PC restart -- I think it has something to do with anti-virus... Without the first delay(5) it happens every 3rd time or so.

Let me look into this a little before merging. Otherwise code looks great!

Thanks :)

The most important question: could you reproduce the issue?

yes, reproduced, and I tracked down why. When the SELECT key is sent to the confirm prompt, the intercepted key isn't passed on to the underlying screen until the next rendered frame. Since we escape out of that screen before the next rendered frame, it is never sent -- until the screen is reopened. If you run the test, and it fails, and then you manually open the manager orders screen, you can see the test order there before it quickly disappears. If you pause the confirm dialog and resend MANAGER_REMOVE, it works without any injected delays. I'll put that suggestion in the PR.

Is there a way to "wait until the next rendered frame"? It seems possible with dfhack.timeout, but probably cumbersome. On a related note, I can't find docs for the delay itself...

Yes, but it's somewhat hacky. You can get how many ticks occur between each frame and wait that many ticks plus one. I'm not sure if this is always consistent, though, since the ticks per graphics frame is variable.

There are no docs for the unit tests, which is a bad thing. You can see the implementation of delay here: https://github.com/DFHack/dfhack/blob/0aa7ec877e4201af76b429438037fb5fc8a501c2/ci/test.lua#L82

It gets injected into the global environment for unit tests along with some other utility functions. In fact, you might be able to use delay_until to detect when a frame has been rendered.

This requires DFHack/dfhack#2211

test/gui/workorder-details.lua

When the `SELECT` key is sent to the `confirm` prompt, the intercepted key isn't passed on to the underlying screen until the next rendered frame. Since we escape out of that screen before the next rendered frame, it is never sent -- until the screen is reopened. If you run the test, and it fails, and then you manually open the manager orders screen, you can see the test order there before it quickly disappears. If you pause the confirm dialog and resend `MANAGER_REMOVE`, it works. Co-authored-by: Myk <[email protected]>

add gui/workorder-finetune.lua

8101146

Adjust input items, material, or traits for work orders. Actual jobs created for it will inherit the details.

myk002 requested changes Mar 26, 2022

View reviewed changes

use reqscript 'gui/workshop-job'

9dbe4e6

This allows to reuse previously duplicated code from `gui/workshop-job.lua`. Also, minor wording changes and comments.

myk002 requested changes Mar 29, 2022

View reviewed changes

TymurGubayev added 6 commits April 3, 2022 15:58

minor changes

ac2f00c

make everything local

136e0dd

clarify comment regarding gui/workshop-job

80902da

add test

a493869

remove tab

4237c52

update a comment

2235a1e

myk002 approved these changes Apr 6, 2022

View reviewed changes

rename to workorder-details, add a changelog entry

f448668

TymurGubayev changed the title ~~add gui/workorder-finetune.lua: adjust input item, material, traits~~ add gui/workorder-details.lua: adjusts input item, material, traits Apr 13, 2022

remove trailing spaces

706328f

myk002 requested changes May 9, 2022

View reviewed changes

Merge branch 'master' into patch-1

ead4f96

use the workorder script to create the work order for the test

7c546a2

TymurGubayev mentioned this pull request Jun 10, 2022

add ItemTraitsDialog to materials.lua DFHack/dfhack#2199

Merged

TymurGubayev added 2 commits June 12, 2022 19:35

Merge branch 'DFHack:master' into patch-1

da0f5c8

use gui.materials.ItemTraitsDialog

099dd8a

TymurGubayev requested a review from myk002 June 12, 2022 17:41

myk002 reviewed Jun 12, 2022

View reviewed changes

test/gui/workorder-details.lua Show resolved Hide resolved

check temporary order is removed

db54b5f

myk002 reviewed Jun 14, 2022

View reviewed changes

gui/workorder-details.lua Outdated Show resolved Hide resolved

TymurGubayev added 2 commits June 14, 2022 13:10

handle confirm plugin in test

0c0e9bc

add a test for removing every possible trait

d3f8c98

Merge branch 'master' into patch-1

50c29ec

myk002 approved these changes Jun 20, 2022

View reviewed changes

TymurGubayev added 2 commits June 22, 2022 11:39

use default ItemTraitsDialog.on_select handler

e137e2f

This requires DFHack/dfhack#2211

fix merge-error in changelog.txt

696bc14

TymurGubayev mentioned this pull request Jun 22, 2022

add keybinding for gui/workorder-details to dfhack.init-example DFHack/dfhack#2222

Merged

Merge branch 'master' into patch-1

18f0af3

myk002 approved these changes Jun 30, 2022

View reviewed changes

test/gui/workorder-details.lua Outdated Show resolved Hide resolved

myk002 approved these changes Jul 1, 2022

View reviewed changes

myk002 merged commit 9480cad into DFHack:master Jul 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add gui/workorder-details.lua: adjusts input item, material, traits #358

add gui/workorder-details.lua: adjusts input item, material, traits #358

TymurGubayev commented Mar 26, 2022 •

edited

Loading

myk002 left a comment •

edited

Loading

myk002 commented Mar 26, 2022

TymurGubayev commented Mar 26, 2022

TymurGubayev commented Mar 26, 2022

TymurGubayev commented Mar 27, 2022 •

edited

Loading

myk002 Mar 29, 2022

TymurGubayev Apr 3, 2022

myk002 Apr 3, 2022

TymurGubayev Jun 10, 2022

TymurGubayev commented Apr 3, 2022

myk002 commented Apr 3, 2022

myk002 commented Apr 3, 2022

myk002 commented Apr 6, 2022

myk002 left a comment

myk002 commented Apr 6, 2022

TymurGubayev commented Apr 6, 2022

myk002 commented Apr 6, 2022

TymurGubayev commented Apr 13, 2022

myk002 commented Apr 14, 2022

TymurGubayev commented Apr 15, 2022

myk002 commented Apr 15, 2022 •

edited

Loading

myk002 left a comment

myk002 commented Jun 3, 2022

TymurGubayev commented Jun 6, 2022

myk002 commented Jun 6, 2022 •

edited

Loading

myk002 Jun 20, 2022

TymurGubayev Jun 22, 2022

myk002 Jun 29, 2022

TymurGubayev Jun 30, 2022

myk002 Jun 30, 2022 •

edited

Loading

TymurGubayev Jul 1, 2022

myk002 Jul 1, 2022

add gui/workorder-details.lua: adjusts input item, material, traits #358

add gui/workorder-details.lua: adjusts input item, material, traits #358

Conversation

TymurGubayev commented Mar 26, 2022 • edited Loading

myk002 left a comment • edited Loading

Choose a reason for hiding this comment

myk002 commented Mar 26, 2022

TymurGubayev commented Mar 26, 2022

TymurGubayev commented Mar 26, 2022

TymurGubayev commented Mar 27, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TymurGubayev commented Apr 3, 2022

myk002 commented Apr 3, 2022

myk002 commented Apr 3, 2022

myk002 commented Apr 6, 2022

myk002 left a comment

Choose a reason for hiding this comment

myk002 commented Apr 6, 2022

TymurGubayev commented Apr 6, 2022

myk002 commented Apr 6, 2022

TymurGubayev commented Apr 13, 2022

myk002 commented Apr 14, 2022

TymurGubayev commented Apr 15, 2022

myk002 commented Apr 15, 2022 • edited Loading

myk002 left a comment

Choose a reason for hiding this comment

myk002 commented Jun 3, 2022

TymurGubayev commented Jun 6, 2022

myk002 commented Jun 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myk002 Jun 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TymurGubayev commented Mar 26, 2022 •

edited

Loading

myk002 left a comment •

edited

Loading

TymurGubayev commented Mar 27, 2022 •

edited

Loading

myk002 commented Apr 15, 2022 •

edited

Loading

myk002 commented Jun 6, 2022 •

edited

Loading

myk002 Jun 30, 2022 •

edited

Loading