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

Make gui/workorder-details and gui/workshop-job work again #799

Open
wants to merge 31 commits into
base: master
Choose a base branch
from

Conversation

TymurGubayev
Copy link
Contributor

@TymurGubayev TymurGubayev commented Aug 13, 2023

This PR merges the gui/workorder-details.lua into gui/workshop-job.lua and updates the result to work with DF50.

image

@TymurGubayev
Copy link
Contributor Author

TymurGubayev commented Aug 13, 2023

issues:

  • Ctrl+D opens another DFHack screen as well -- keybindings: add /Default to Ctrl-D@dwarfmode gui/design dfhack#3727
  • I haven't found a better way to hide the widget when in the wrong context, other than self.subviews.button.visible = false in onRenderBody -- we'll keep it as is for now
  • also, I haven't tested it beyond ensuring the UI is kind of working

@myk002
Copy link
Member

myk002 commented Aug 13, 2023

  • Ctrl+D opens another DFHack screen as well

This can likely be solved by changing the gui/design keybinding from @dwarfmode to @dwarfmode/Default in dfhack.keybindings.init

  • I haven't found a better way to hide the widget when in the wrong context, other than self.subviews.button.visible = false in onRenderBody

The preferred solution here is to modify modules/Gui.cpp to produce the more specific focus string that you need. If that is not sufficient, then yes you can set the button invisible in onRender[Whatever]

  • also, I haven't tested it beyond ensuring the UI is kind of working

The UI definitely needs some updating and testing. It uses code from gui/workshop-job, which itself hasn't been looked at since v50 dropped. It also should use a ZScreen and draggable windows to fit in with the modern UI themes.

@@ -9,8 +9,7 @@ This tool allows you to adjust item types, materials, and/or traits for items
used in manager workorders. The jobs created from those workorders will inherit
the details.
Copy link
Member

Choose a reason for hiding this comment

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

the introductory text could use a few more examples of things you can do with this tool that you wouldn't be able to do in vanilla. what do you think are the most useful situations where this tool helps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably use dyed or dyeable (=undyed I think) cloth, or encrust unimproved items...

I'm not quite sure.

docs/gui/workorder-details.rst Outdated Show resolved Hide resolved
gui/workorder-details.lua Outdated Show resolved Hide resolved
@TymurGubayev TymurGubayev changed the title Make gui/workorder-details.lua work again Make gui/workorder-details and gui/workshop-job work again Sep 3, 2023
@TymurGubayev
Copy link
Contributor Author

TymurGubayev commented Sep 3, 2023

  • I'm not quite sure what is the replacement for dlg.showMessage -- don't replace it

@myk002
Copy link
Member

myk002 commented Sep 3, 2023

  • I'm not quite sure what is the replacement for dlg.showMessage

That's still fine to use. At some point, I'll swap out the implementation to use a ZScreenModal, but that should be transparent to the callers.

@TymurGubayev
Copy link
Contributor Author

Sphinx is complaining because of docs/changelogs/news.rst:861:undefined label: gui/workorder-details

What's the approach with removed scripts here, should I keep the referenced docs as is, or maybe make its only content a reference to the new script?

@myk002
Copy link
Member

myk002 commented Sep 3, 2023

Tool removal is a tad janky since it crosses repos. You have to add an entry to https://github.com/DFHack/dfhack/blob/develop/docs/about/Removed.rst, but CI for both repos will fail until everything is consistent.

docs/gui/job-details.rst Show resolved Hide resolved
docs/gui/job-details.rst Outdated Show resolved Hide resolved
gui/job-details.lua Outdated Show resolved Hide resolved
gui/job-details.lua Outdated Show resolved Hide resolved
gui/job-details.lua Outdated Show resolved Hide resolved
gui/job-details.lua Outdated Show resolved Hide resolved
gui/job-details.lua Outdated Show resolved Hide resolved
gui/job-details.lua Outdated Show resolved Hide resolved
@@ -92,14 +86,13 @@ function JobDetails:init(args)
frame = { l = 0, b = 0 },
text = {
{ key = 'LEAVESCREEN', text = ': Back',
on_activate = self:callback('dismiss')
-- on_activate = self:callback('dismiss')
Copy link
Member

Choose a reason for hiding this comment

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

yeah, not needed. the base ZScreen superclass takes care of window lifecycles

Copy link
Member

@myk002 myk002 left a comment

Choose a reason for hiding this comment

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

what does the 20 mean?
image


DetailsHotkeyOverlay_BuildingTask = defclass(DetailsHotkeyOverlay_BuildingTask, DetailsHotkeyOverlay)
DetailsHotkeyOverlay_BuildingTask.ATTRS{
default_pos={x=-120, y=6}, -- {x=-120, y=6} is right above the job title on all but smallest widths
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this to the right by 5 so it still looks good at the smallest width

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I "reserved" the whole line using frame={w=1000, h= 1}, so that now with dfhack.screen.getWindowSize() I can move the hotkey around and it always fits.

It would've been nice to be able to move the widget itself. IIRC that wasn't possible last time I checked (half a year ago? because of label placement in the workorder-recheck.lua).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I improved upon this. The logic is as follows now.

  1. Fix the distance from the right edge; make the widget slightly wider than necessary to allow moving the text around
  2. determine the calculated x1 coordinate of the frame, which is the screen width minus the distance from the right minus the widget width
  3. if the x1 < 0, set it to 0 --- making it the real position of the widget (I think this works better than built-in methods for some reason, although it's a few experiments back)
  4. is the real left edge position of the widget too far to the left (x1 < 6), move the text to the right (offset = 6 - x1).

This works well enough until you try to narrow the game window to the minimum and then widen it again. Then the self.frame.r is changed from 102 to 77 and stays that way. That's why the 4th step is needed

  1. restore original self.frame.r

Even with this after widening the window it doesn't immediately get the right position, but another small resizing in any direction sets it right.

gui/job-details.lua Outdated Show resolved Hide resolved
@TymurGubayev
Copy link
Contributor Author

TymurGubayev commented Nov 5, 2023

what does the 20 mean? image

something changed somewhere and now we have for some reason a set bit without a name:

image

(even though df.job_item_flags3[20] == nil)

and this is how the list is filled from a bitfield:

local function list_flags(list, bitfield)
    for name,val in pairs(bitfield) do
        if val then
            table.insert(list, name)
        end
    end
end

Might this be a bug? Or maybe it should've been unknown?

To avoid confusion, I'll change the method above to exclude those.

@myk002
Copy link
Member

myk002 commented Nov 6, 2023

what does the 20 mean?

That might require a structure update. I'll look into it

@myk002
Copy link
Member

myk002 commented Nov 6, 2023

I identified the flags and pushed the updated build to the Steam testing branch if you'd like to test with it. Flag 20 is (unsurprisingly, given the context) "gem".

gui/job-details.lua Outdated Show resolved Hide resolved
@myk002
Copy link
Member

myk002 commented Nov 7, 2023

testing notes:

  1. the alignment seems off (screen dimensions: 150x66):
    image
  2. maybe don't enable the Configure job inputs HotkeyLabel if there is nothing that can be configured:
    image
  3. The ability to reset to defaults would be very nice
  4. The ability to remove traits (or change the item type) could be considered an exploit. Could we only offer that capability if armok mode is enabled? (dfhack.getHideArmokTools() returns false)
  5. I've learned from showmood that people want to see whole item quantities, not the internal counters. could we display 1 here instead of 150 (similar for cloth and thread)
    image
  6. The Esc hotkey hint probably isn't necessary. Most other windows don't have that.
  7. "quantity: 1" would be clearer than "x1"

@TymurGubayev
Copy link
Contributor Author

  1. the alignment seems off (screen dimensions: 150x66):

changed it to work based on window size instead of reading the screen

  1. maybe don't enable the Configure job inputs HotkeyLabel if there is nothing that can be configured:

did that

  1. The ability to reset to defaults would be very nice

defaults is hard - I'd have to know them for every job type in advance. I added a Ctrl-Z to reset changes

  1. The ability to remove traits (or change the item type) could be considered an exploit. Could we only offer that capability if armok mode is enabled? (dfhack.getHideArmokTools() returns false)

change item type is now disabled if armok tools aren't present. Traits have same problem as defaults: I'd need to know in advance what the defaults are, to prevent the user from removing the non-default ones.

  1. I've learned from showmood that people want to see whole item quantities, not the internal counters. could we display 1 here instead of 150 (similar for cloth and thread)

changed that. Now I'm not sure anything but 1 is even possible... Maybe in a modded game?

  1. The Esc hotkey hint probably isn't necessary. Most other windows don't have that.

removed that

  1. "quantity: 1" would be clearer than "x1"

changed that. Btw, there are two modes: work order and active job, with slightly different texts

image

image

It's possible we can only ever see 0 of 1 for an active job.

gui/job-details.lua Outdated Show resolved Hide resolved
for _, obj in pairs(self.list.choices) do
local stored_obj = self.stored[obj.index]

local item_type = stored_obj.item_type
Copy link
Member

Choose a reason for hiding this comment

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

Attempting to reset changes for me results in this error:

gui/job-details.lua:419: attempt to index a nil value (local 'stored_obj')

Copy link
Member

Choose a reason for hiding this comment

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

looking through the code, it looks like storeInitialProperties is checking job.items instead of job.job_items

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the job.job_items vs job.items is the bane of my existence 😣

because there are two structures involved, manager order and job, and one of them has both but uses the one that's distinct.

I think I fixed it now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants