Conversation
|
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
chdoc
left a comment
chdoc
left a comment
There was a problem hiding this comment.
Since the requested changes are quite substantial, please re-request a review from me once you have implemented the desired changes. I anticipate this to require at least one more round of review.
| local dialogs = require('gui.dialogs') | ||
| local gui = require('gui') | ||
| local dialogs = require('gui.dialogs') | ||
| local widgets = require('gui.widgets') |
There was a problem hiding this comment.
Please do not just shuffle imports. They used to be in alphabetical order. Only change what needs to be changed. Small diffs are easier to review.
| widgets.Panel{ | ||
| frame={l=0, r=0, h=1}, | ||
| subviews={ | ||
| widgets.HotkeyLabel{ | ||
| frame={l=0}, | ||
| key='CUSTOM_C', | ||
| label='Include containers:', | ||
| on_activate=function() self:toggle_option('containers') end, | ||
| }, | ||
| widgets.Label{ | ||
| view_id='containers_lbl', | ||
| frame={r=24, w=10}, | ||
| text_halign=2, | ||
| text='', | ||
| }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
I see no reason for reinventing the wheel here. This should just be a ToggleHotkeyLabel. The same applies of course to the other instances below.
| function IncludeOptionScreen:onInput(keys) | ||
| if keys.SELECT then | ||
| self:confirm() | ||
| return true | ||
| elseif keys.CUSTOM_C then | ||
| self:toggle_option('containers') | ||
| return true | ||
| elseif keys.CUSTOM_G then | ||
| self:toggle_option('general') | ||
| return true | ||
| elseif keys.CUSTOM_A then | ||
| self:toggle_option('categories') | ||
| return true | ||
| elseif keys.CUSTOM_T then | ||
| self:toggle_option('types') | ||
| return true | ||
| end | ||
|
|
||
| return IncludeOptionScreen.super.onInput(self, keys) | ||
| end |
There was a problem hiding this comment.
This can actually go away, if you use the suggested ToggleHotkeyLabel. Do not use custom logic if it isn't required.
| dialogs.InputBox{ | ||
| frame_title='Export Stockpile Settings', | ||
| text='Please enter a filename', | ||
| on_input=function(text) | ||
| export_settings(text, { | ||
| id=sp and sp.id, | ||
| includes=includes, | ||
| }) | ||
| end, | ||
| }:show() |
There was a problem hiding this comment.
If you are already creating a window for the export, then this window should also a text field a for the file name. A well-developed name panel can be found in: https://github.com/DFHack/scripts/blob/56c934eb5d4c957ca731705783a0a43397a9ba2c/gui/blueprint.lua#L44-L71
Please have a look and decide whether this can be reused. In any case, the dialog box should be replaced with something along these lines.
| widgets.TooltipLabel{ | ||
| view_id='disclaimer', | ||
| frame={r=0, t=3, w=17}, | ||
| text_to_wrap='All set to [No] skips --include and includes everything!', |
There was a problem hiding this comment.
I think this is really bad UX design. The two approaches that seem reasonable to me are:
- Disable the "submit" button, when all toggles are set to "no"
- Change all toggles to "yes", if the last toggle is set to "no"
2 participants
New Feature:
Upon exporting a stockpile settings, a new popup window one step before "Please enter a filename" has been added.
It allows handling the quite neat
stockpiles export --includeoption that is in the code since I remember it, but absent from the current UI.Default view:
Full toggled view:
Comments/issues:
There is an unavailable gui/stockpiles piece of code that approximately tried to do what the new UI does.
I'm wondering if the UI elements of this script shouldn't be moved to and replace the old gui/stockpiles. Or implement a new structure?
Background note:
In my latest playthroughs I have observed issues where imported stockpile settings - with for example 64 max bins set on a 3x3 stockpile - would prevent the dwarves from ever starting to haul stuff to the stockpile. Only
stockpiles importis able to set more containers than there are available tiles in a given stockpile. The game prevents it.This is why I thought of this feature.
The real fix though could be added after the parsing of the .dfstock file, which could avoid setting more containers than there are available tiles in the selected stockpile.
It seems a pretty easy fix computing the value with the help of the vector
stockpile.room.extents, but this is C++ and I have zero clue.Disclaimer:
I'm not a professional programmer. Started learning basic coding 12 years ago in the hope of becoming a DFHack contributor someday (true story). Got sidetracked by life.
This is my first bigger lua piece of code, go ahead and be critical.
Until 2 weeks ago I was only Skill: Adequate Python (rusty).
Eager to learn along the way.
Thank you for your attention.