Add output and labels to quickcmd#1534
Conversation
| local CONFIG_FILE_OLD = 'dfhack-config/quickcmd.json' | ||
| local CONFIG_FILE = 'dfhack-config/quickcmd-v2.json' |
There was a problem hiding this comment.
@ab9rf Is there a policy for updating the configuration format for tools that are not world or site specific? I would tend to keep the configuration filename aligned with the name of the tool without adding any version suffixes, putting any versioning information about the configuration file format into the configuration file itself. That is, back up the old configuration file under a new name and then convert the configuration in place.
There was a problem hiding this comment.
I'm inclined to agree with you here: keep the file name the same and store the version information into the file itself.
chdoc
left a comment
chdoc
left a comment
There was a problem hiding this comment.
Overall, I think this is a pretty nice change and the code looks pretty clean. Please update the configuration filed behavior as discussed in my previous comment and use a canonical value for commands that do not have assigned names.
| end) | ||
| screen:dismiss() | ||
|
|
||
| local OutputDialog = defclass(OutputDialog, gui.ZScreenModal) |
There was a problem hiding this comment.
I do not see any compelling reason why the output dialog should be modal. On the contrary, I can definitely see myself looking at the output and then trying to match it to various places inside the fort.
| local title | ||
| if self.name and self.name ~= '' then | ||
| title = self.name .. ': ' .. self.command | ||
| else | ||
| title = self.command | ||
| end |
There was a problem hiding this comment.
| local title | |
| if self.name and self.name ~= '' then | |
| title = self.name .. ': ' .. self.command | |
| else | |
| title = self.command | |
| end | |
| local title = ('%s%s'):format(self.name and self.name .. ': ' or '', self.command) |
This assumes that self.name is never actually the empty string, which doesn't make sense in the first place and should probably be enforced by the input dialog
| COLOR_GREEN, | ||
| self.commands[index].name or '', | ||
| function(name) | ||
| self.commands[index].name = name |
There was a problem hiding this comment.
If the user enters the empty string for the command name, that should probably unset (i.e. set to nil) the name for that command. Otherwise, you have two separate states that you need to treat essentially the same. The other option is to ensure that name is always set, even if it is only set to be the empty string. I think either one is fine, but I don't like to have two different states that basically mean the same and both need to be checked at various places. Either solution is find with me.
3 participants
Changes
quickcmd window:
Output window:
Old and new settings file:
Issues