Add overlay to queue lever pull jobs from linked building view#5725
Add overlay to queue lever pull jobs from linked building view#5725pajawojciech wants to merge 7 commits into
Conversation
…ased on the state without considering the building type
|
I like the idea. However, I don't see any reason why this should be extending the |
Thanks for review. |
|
I still maintain that an overlay that exclusively deals with pulling levers does not belong in a file that is called The only slightly annoying part of moving this to |
chdoc
left a comment
chdoc
left a comment
There was a problem hiding this comment.
Finally got around to providing an in-depth review.
| function MechLeverPullOverlay:is_lever(building) | ||
| return building._type == df.building_trapst and building.trap_type == df.trap_type.Lever | ||
| end | ||
|
|
||
| function MechLeverPullOverlay:get_lever_pull_job(lever) | ||
| for _, job in ipairs(lever.jobs) do | ||
| if job.job_type == df.job_type.PullLever then | ||
| return job | ||
| end | ||
| end | ||
| return nil | ||
| end |
There was a problem hiding this comment.
These neither read nor write member variables, so there is no need for them to be member functions.
| local building_ref = df.general_ref_building_holderst:new() | ||
| building_ref.building_id = lever.id | ||
|
|
||
| local job = df.job:new() |
There was a problem hiding this comment.
Please use dfhack.job.createLinked() and remove the call to linkIntoWorld (or better reuse the existing functionality from the lever script),
| y = lever.centery, | ||
| z = lever.z | ||
| } | ||
| job.flags.do_now = false |
There was a problem hiding this comment.
| job.flags.do_now = false | |
| job.flags.do_now = true |
I think one can argue that the default for pulling levers should be with urgency.
| for i = #lever.jobs, 1, -1 do | ||
| local job = lever.jobs[i-1] | ||
| if job.job_type == df.job_type.PullLever then | ||
| lever.jobs:erase(i-1) |
There was a problem hiding this comment.
Is it really necessary to call erase? removeJob should already remove all references to the job,
| function MechLeverPullOverlay:onRenderFrame(dc, rect) | ||
| if self.bld_id ~= sheet.viewing_bldid then | ||
| self.bld_id = sheet.viewing_bldid | ||
| self.building = df.building.find(self.bld_id) |
There was a problem hiding this comment.
Since buildings can be destroyed at any time, storing buildings is generally not safe. This may be fine, because none of this is called unless there is a viewscreen open and therefore the game is paused, but please double check.
| end | ||
| end | ||
|
|
||
| function MechLeverPullOverlay:update_buttons() |
There was a problem hiding this comment.
I find it really hard to understand what is going on in these methods. I would really appreciate 1-3 lines of comments about the general idea here.
github-project-automation
Bot
moved this from Needs review
to Needs revision
in 53.10-r2
2 participants
How it works
Click button
After click text changes to "Pulling" with yellow color, and job is queued. Now you can click again if you change your mind - it removes queued job.
Job assigned
When job is assigned text changes color to green:
Pulled
After pull text, color and indicator changes
When Unlink is enabled button moves to the left
What I tried
Instead of pull text i tried to set Open / opening / close / closing labels. For standard bridges it works fine, but for example retracting bridges or cages should have reversed operation naming, so I use 'Pull' text and lever position indicator
Issue
#3938