Migrate widgets constants to use getter/setter style #4972
Conversation
wiktor-obrebski
force-pushed
the
fix/widgets-constants
branch
from
f2a5234
to
4144b0e
Compare
| @@ -5348,11 +5348,15 @@ If the panel has already been maximized in this fashion, then it will jump to | |||
| its minimum size. Both jumps respect the resizable edges defined by the | |||
| ``resize_anchors`` attribute. | |||
|
|
|||
| The time duration that a double click can span is defined by the global variable | |||
| ``DOUBLE_CLICK_MS``. The default value is ``500`` and can be changed by the end | |||
| The time duration that a double click can span can be controlled by widgets API: | |||
There was a problem hiding this comment.
Could you update this to indicate that the control-panel or gui/control-panel interfaces should be used to set these properties? That goes for the others as well. You can remove the widget.* function references. These docs were written before the control panel was a thing.
The gui/control-panel interface looks like this:
and the control-panel interface looks like this:
the functions should no longer be called by anything other than the control panel interfaces
| ---@return boolean | ||
| function getStandardScroll() | ||
| return Scrollbar.STANDARDSCROLL | ||
| end | ||
| function setStandardScroll(value) | ||
| Scrollbar.STANDARDSCROLL = value | ||
| end |
There was a problem hiding this comment.
STANDARDSCROLL is a constant and doesn't need a function wrapper. we could keep getStandardScroll() if you think it makes the API clearer, but there should not be a setStandardScroll()
There was a problem hiding this comment.
(my vote is that it doesn't make the API clearer)
There was a problem hiding this comment.
Maybe I'm being a bit grumpy, but I feel like this is a stylistic preference and I'm not sure changing it around benefits us much vs. the potential for broken scripts.
I do see #4955 (comment), and I'm not sure how this change fixes the issue - or rather, why the issue was an issue. The things you're changing in https://github.com/DFHack/scripts/pull/1313/files are all resolved at runtime, so changing e.g. widgets.DOUBLE_CLICK_MS before your change should have had exactly the same effects as changing what widgets.getDoubleClickMs() returns after your change. You could potentially see stale values if those expressions were evaluated at module-load or script-load time, but that's true for both the old and new expressions. So I guess I'm not understanding what you're actually trying to fix.
| ---@return boolean | ||
| function getStandardScroll() | ||
| return Scrollbar.STANDARDSCROLL | ||
| end | ||
| function setStandardScroll(value) | ||
| Scrollbar.STANDARDSCROLL = value | ||
| end |
There was a problem hiding this comment.
(my vote is that it doesn't make the API clearer)
| @@ -277,7 +277,7 @@ Label.ATTRS{ | |||
| auto_width = false, | |||
| on_click = DEFAULT_NIL, | |||
| on_rclick = DEFAULT_NIL, | |||
| scroll_keys = STANDARDSCROLL, | |||
| scroll_keys = Scrollbar.STANDARDSCROLL, | |||
There was a problem hiding this comment.
If we have multiple other widgets referencing STANDARDSCROLL, maybe it should move back to being a top-level constant (like I think it was before, in widgets.lua)
There was a problem hiding this comment.
the issue here, I think, is that widgets can't include gui.widgets since then it would be a circular dependency
another option is to move all constants (and settable preferences) into a separate, common file
There was a problem hiding this comment.
Ok, maybe that would be a circular dependency. In that case, I think we do need a separate constants file. Making Label depend on Scrollbar for a shared constant feels very arbitrary to me, and could result in cyclical dependencies down the road if we add more constants that multiple widgets need.
The issue is not in seeing stale values in
|
|
That does clear it up. I missed that DOUBLE_CLICK_MS was getting pulled from Panel.DOUBLE_CLICK_MS. I would argue that we should not maintain the constant in two places. If multiple widgets need it, I don't think the primary definition should live under one widget (Panel), similar to what I said for STANDARDSCROLL. |
|
That sounds good to me -- @wiktor-obrebski could you move the settings/constants to a separate file? maybe call it We could still expose them in |
We could do that, although for those "constants" that we want to allow changing, I think we would run into the same issue where only one copy of the constant(s) is changed unless we provide a setter. |
|
right -- that's what I meant. We'd still need setters/getters at the public widgets.lua API level. |
Pair PR: DFHack/scripts#1313
Continuation of #4955, its answer for the comment:
#4955 (comment)
Shortly, its migrate widgets constants to use getter/setter style.