Demonstrate gui.dflayout library and use it in gui/mass-remove.toolbar #1426
Conversation
|
i feel like you should move the demo code into a new file under the |
|
Beyond the usual review stuff, here are some things that I think merit some review consideration:
|
|
@ChrisJohnsen Here are my thoughts:
Yes i think so
i'll leave this to myk
strongly agreed
i think that might be a good idea on an as we find them basis after this pr is merged (unless you already have a list). regardless tho i think thats outside the scope of this PR.
yes it is and as i stated before, i think it should move to
maybe |
|
I added a merge with DFHack/scripts:master that keeps this branch's gui/mass-remove.toolbar changes. Hopefully the unrelated merged commits don't interfere with GitHub's review functionality. |
yes. yes it was ; )
What error does it have? I wasn't aware of any positioning errors for the warmdamp button.
Yes, I'd rather have this positioning code in one place, even if just for the current use cases, rather than have magic constants sprinkled around
Pure library code like this should go under https://github.com/DFHack/dfhack/tree/develop/library/lua -- this in particular should probably go under https://github.com/DFHack/dfhack/tree/develop/library/lua/gui . A new module would be fine.
I believe there are, but we can focus on fort mode for now
Once we migrate the various widgets to using this library, we can play with the window and visually verify that they behave properly. Two DF windows can be brought up simultaneously to verify before/after behavior
Docs would go in Lua API.rst -- more later, have to go |
|
Thank you for your time! BTW, due to trying to rush out a preview, I have churned this PR more than I would have liked. I intend to rebase and squash a bunch of this stuff to get rid of some of the intermediate missteps, so except for curiosity, it probably makes sense to mostly review the code from the "files changed" view. Since the library code would need to swap repos anyway, I can squash out my intermediate mess then.
My interpretation of what warmdamp does is that it effectively "reinterprets" the player-configured overlay position (relative to its default position) as "toolbar-relative". Once I started implementing variations on that idea (e.g., intermediate commits in this PR: 70262e0, 3c325cc), I found that the width-only modification that warmdamp does to "reinterpret" its overlay's position as "toolbar-relative" only works (as I inferred its intention) when the overlay is anchored to the left and bottom edges of the interface area (the bottom edge works without extra manipulation since the toolbar never moves relative to the bottom edge of the interface). When anchored to the top or right edges, the result is the normal overlay system interface-edge-anchoring (the overlay doesn't "stick with" the toolbar when the interface is resized). Maybe this isn't an error. It felt inconsistent, but maybe the top and right edges were kept as "escape hatches" for extra placement flexibility. In 3c325cc I found that making the position toolbar-relative in all four directions meant that the overlay could only be positioned (relative to the toolbar) as far away as it could in a minimum-size interface area; I didn't expect this before trying it, but it does match the current left-anchored behavior of warmdamp. I also tried only applying the "padding" toward the anchoring edges, but that causes very strange jumps in effective button position when the overlay approaches and then touches a non-anchor edge (switching the anchoring edge, thus also the "padded side", when it does touch). Probably almost no one even bothers to move the warmdamp overlay, and in that case there is no problem. And if someone did move the button, it was probably not moved very far (just to another nearby gap in the dig toolbar); that wouldn't be a problem either. But if someone tried moving it to the far right end of the (advanced) options, the overlay could change its anchor to the right edge (it looks like it takes an interface with more than 236 columns to move the overlay over past the DF dig blueprint buttons without the overlay swapping over to right-edge anchoring). Even then, it would still look right except at different interface/window sizes. Probably many players always use full-screen. |
This brings up an issue that I've become more and more aware of: many of our overlays are not actually repositionable. They have hard-coded behavior that depends on not moving from their default position. I think this might be a distinction that we need to formalize. The |
|
continuing:
yes, please ; )
as you said, it would probably be useful for verifying future updates. It can live as a
this would amount to cataloging the positions and dimensions of all DF panels and characterizing how they react to varying window sizes, which would be useful, but also a lot of work. we could try handling some of the current use cases, and then we can put a policy in place for extending this library if/when we need to add more. |
|
Okay, here is my tentative plan:
This PR could be recycled for one or both of those last two, if you think that makes sense. Any thoughts, or preferences for the name of the module? As named above, the module would be |
|
sgtm calling it you can reuse this PR for the demo script |
ChrisJohnsen
force-pushed
the
cj/mr-button/pr
branch
from
55d3acd to
6c26925
Compare
ChrisJohnsen
changed the title
|
This has been respun to use DFHack/dfhack#5385. |
gui/mass-remove.lua
Outdated
| local function mass_remove_button_offsets(interface_size) | ||
| local erase_frame = erase_toolbar.frame(interface_size) | ||
| return { | ||
| l = erase_frame.l + erase_frame.w + 1, | ||
| w = erase_frame.w, | ||
| r = erase_frame.r - 1 - MR_BUTTON_WIDTH, | ||
|
|
||
| t = erase_frame.t, | ||
| h = erase_frame.h, | ||
| b = erase_frame.b, | ||
| } | ||
| end |
There was a problem hiding this comment.
I'm a bit torn by this. I like the idea of consolidating the logic for vanilla UI layout, but this function is neither shorter nor clearer than the status quo.
What would be nice is an API that would take a spec and return values that we can use directly.
For example, would it be possible to write:
local spec = { ... }
...
default_pos=dflayout.getOverlayDefaultPos(spec),
frame=dflayout.getOverlayFrame(spec),
...
MassRemoveToolbarOverlay.preUpdateLayout = dflayout.getPreUpdateLayoutFn(spec)spec would possibly contain a reference to some UI element and a frame specifier to size/position the widget relative to that UI element.
There was a problem hiding this comment.
Yes, something like that would be nicer. I'll see what I can come up with for this. I suspect there might be some extra complexity here to fully support different kinds of alignment/offsetting…
There was a problem hiding this comment.
Updates to DFHack/dfhack#5385 and this are up now.
It currently looks like this:
local PLACEMENT = dflayout.getOverlayPlacementInfo{
size = { w = 20, h = 1 },
ui_element = dflayout.elements.fort.secondary_toolbar_buttons.DIG.DIG_DIG,
h_placement = 'align left edges',
v_placement = 'above',
}
...
default_pos=PLACEMENT.default_pos
...
TheOverlay.preUpdateLayout = PLACEMENT.preUpdateLayout_fnHaving one function seemed simpler (there is a bit of shared calculation between default_fn and preUpdateLayout_fn), but it could be multiple functions if you think that would be better.
frame is also available, but doesn't seem to really be needed for overlay widgets: OverlayWidget establishes an initial frame, and preUpdateLayout_fn will update it.
It also supports off-nominal default_pos values (like gui/mass-remove.toolbar here).
The toolbar UI elements (the only ones currently supported) all have static sizes and non-decreasing "insets" (from the interface area edges). The "placement" implementation works inside these limitations, but I would like to see if it can be generalized a bit to support variable-size UI elements (e.g., the rows of DF info window, as an aggregate UI element) and those without non-decreasing insets (e.g., the bottom inset of the last row of in DF info, which varies up and down as the interface area height changes (mod 3)).
I'll start experimenting with that next. It seems like something that might be worth having in the "final" initial implementation even if it doesn't include "definitions" for any such UI elements. The documentation encourages treating the "UI element" values as opaque to try to leave room for possible changes needed to support cases like these.
Partially addresses DFHack#1426 (comment)
Linking can be made to work, but would require - a ref declaration (label) in the Lua API docs, and - the gui.dflayout section to be present in the scripts CI's view (it is still in a PR, so the scripts CI does not currently see it). Just use normal code formatting.
Makes explicit the fields required from each "demo".
Remove demo_available(), just use demo.available() directly. ("Always
available if .available is nil" seems a bit awkward)
The interface percentage was not being saved, so it was constantly initiating updates. Once the spurious updates were fixed, it uncovered broken update processing when activating a demo and when the fort toolbar demo was told to update itself.
Have the toolbar demos set their own frames via computeFrame. These overridden computeFrames also allow the button extent indicator strings to draw over the edges of the Panel's border. The left, right, and secondary toolbars don't have their own "borders" like the center toolbar, so the first and last buttons in those toolbars previously had their button indicators cut off by the Panel border. Certainly not a good look for general use, but for a devel inspection tool, it should be okay (and leaves the Panel borders showing the true extent of each toolbar). Only compute the button strings once for the static left, center, and right toolbars.
Use hasFocus instead of "not defocused". hasFocus also checks that the ZScreen is the topmost. Check demo availability before updating from main window.
Using a local was causing it to revert and get "stuck on" when re-running the script.
Previous code missed interface percentage updates when the ZScreen was focused but not visible_when_not_focused.
Now they are right above the final "raise or create" sequence.
Keeps the secondary toolbar demo from needing to hook into the center toolbar (or some other "always on" widget) to notice changes in the secondary toolbar.
devel/dflayout now highlights the "demo" element when the mouse cursor is over the (computed) real UI element
ChrisJohnsen
force-pushed
the
cj/mr-button/pr
branch
from
ae11d39 to
8e8b0ea
Compare
I mentioned on Discord that I had worked up a data-driven way of calculating the positions of DF toolbars.
Here it is. Of course, it would be better to get position information directly from DF, but having a single place that calculates this stuff should make it easier to provide reliable positioning for all the new toolbar buttons that are being added.
Running
internal/df-bottom-toolbarswill launch a "demonstration" that can be used to check that the computed values match what DF does at various screen sizes and interface percentages.Highlights of the positioning calculations:
the centered toolbar is always at least 7 columns away from the left toolbar, so not always exactly "centered"
the centering involves a little bit of odd rounding
This odd double-rounding seems to work correctly and kind of matches part of
gui.get_interface_rect()(was that re-implemented from reverse engineering?).It was nice to be able to fix the top-/right-anchored overlay positioning error that the warmdamp button also has, but I'm not sure if it is worth it in the form I came up with (last commit). It was kind of a last-minute thing that I tried after getting everything else ready for upload. It could have been a separate set of "tooltip" and "icon" that the layout updating code swapped between, but that maybe seems worse?