Add gui/petitions.lua: shows list of fort's petitions #321
Conversation
TymurGubayev
commented
Aug 15, 2021
TymurGubayev
commented
| -- from gui/unit-info-viewer.lua | ||
| do -- for code folding | ||
| -------------------------------------------------- | ||
| ---------------------- Time ---------------------- |
There was a problem hiding this comment.
Is this section copied from somewhere else? Asking partly due to the different indentation. It might be useful to include in a library so that other scripts can use it too.
There was a problem hiding this comment.
See comment on line 42: from gui/unit-info-viewer.lua
| local sb = {} -- StringBuilder | ||
|
|
||
| sb[#sb+1] = {{text = "Agreement #" ..a.id, pen = COLOR_RED}} | ||
| sb[#sb+1] = NEWLINE |
There was a problem hiding this comment.
Recommendation:
| sb[#sb+1] = NEWLINE | |
| table.insert(sb, NEWLINE) |
This would avoid a decent amount of repetition throughout this function.
There was a problem hiding this comment.
Recommendation:
This would avoid a decent amount of repetition throughout this function.
I don't understand how this would avoid repetition? It would only change what is repeated to table.insert. Am I missing something?
As for the choice, the t[#t+1] is (usually) just a tiny bit faster, and also because Roberto Ierusalimschy said he prefers this version.
There was a problem hiding this comment.
Speed is irrelevant in this situation (runtime will be totally dominated by IO time), and I personally find the table.insert formulation more readable.
Also, minor visual changes
gui/petitions.lua
Outdated
| painter:seek(frame.x1+2, frame.y1 + frame.height-1):key_string('CUSTOM_F', "toggle fulfilled") | ||
|
|
||
| local label = self.subviews.text | ||
| if label:getTextHeight() > label.frame_body.height then |
There was a problem hiding this comment.
This looks like it could be added to the Label class. Would it still look good if rendered on the label widget instead of the frame boundary?
There was a problem hiding this comment.
I personally prefer the arrows and the text rendered on the frame. Second-best good-looking is arrows on an extra column on the very right.
I think both are possible to implement, although the second option is probably much easier.
Here some images for comparison:
on the border:
on an extra right-most column:
arrows on the label widget, no inset (2nd image illustrates a problem with that, notice how arrow hides a character):
label-widget with frame_inset = 1
There was a problem hiding this comment.
Thank you for visualizing the options!
Visual-wise, I think the arrows are easiest to see when they're inside the frame. We have a long-standing standard of putting hotkey hints on the bottom frame (and they look fine there too), so I vote for arrows inside frame, hotkey text on bottom frame.
Architecture-wise, the arrow logic should be part of the Label widget. Labels are used all over the place, and they don't always have a frame around them, but they're always scrollable. We could potentially have the logic in the Label widget but render it somewhere else. That might cause confusion, though.
Behavior-wise, the arrows should only appear if the content height is larger than the Label body area height, otherwise existing, static labels would get unneeded arrows. If they appear, maybe they should force at least a right margin of 1 to avoid the character hiding problem you highlighted. If the widget already has a frame_inset>=1 (or frame_inset.r>=1) then this is a noop, otherwise, just the right inset can be increased by 1. The arrows should probably align vertically with the top and bottom frame inset, and be in the column just to the right of the end of the text area, but I guess check what looks best if frame inset is like 5 or something.
Thank you for doing this! This will be an awesome addition to the Label widget!
There was a problem hiding this comment.
I've created a WIP pull request: DFHack/dfhack#2101 (there is a problem I'm not quite sure what to do about)
There was a problem hiding this comment.
DFHack/dfhack#2101 is now merged. Thanks!
