Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate widgets constants to use getter/setter style #4972

Merged
merged 6 commits into from
Oct 8, 2024

Conversation

wiktor-obrebski
Copy link
Contributor

@wiktor-obrebski wiktor-obrebski commented Oct 1, 2024

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.

@@ -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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
image

and the control-panel interface looks like this:
image

the functions should no longer be called by anything other than the control panel interfaces

Comment on lines 41 to 47
---@return boolean
function getStandardScroll()
return Scrollbar.STANDARDSCROLL
end
function setStandardScroll(value)
Scrollbar.STANDARDSCROLL = value
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(my vote is that it doesn't make the API clearer)

Copy link
Member

@lethosor lethosor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines 41 to 47
---@return boolean
function getStandardScroll()
return Scrollbar.STANDARDSCROLL
end
function setStandardScroll(value)
Scrollbar.STANDARDSCROLL = value
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@myk002
Copy link
Member

myk002 commented Oct 2, 2024

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.

The issue is not in seeing stale values in widgets.DOUBLE_CLICK_MS, but rather in the widgets that now reference Panel.DOUBLE_CLICK_MS. This PR fixes the case where:

  1. Load DF
  2. widgets.DOUBLE_CLICK_MS gets the value from Panel.DOUBLE_CLICK_MS
  3. player starts gui/control-panel. it reads and displays widgets.DOUBLE_CLICK_MS, and player uses it to change value of widgets.DOUBLE_CLICK_MS (does not change Panel.DOUBLE_CLICK_MS since widgets.DOUBLE_CLICK_MS is a copy, not a reference)
  4. a tool starts that includes a Panel and then the player double clicks on the header. The Panel reads Panel.DOUBLE_CLICK_MS, and it still has the old, original value.

@lethosor
Copy link
Member

lethosor commented Oct 2, 2024

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.

@myk002
Copy link
Member

myk002 commented Oct 2, 2024

That sounds good to me -- @wiktor-obrebski could you move the settings/constants to a separate file? maybe call it widgets/wcommon.lua?

We could still expose them in widgets.lua as the current PR has it for external usage

@lethosor
Copy link
Member

lethosor commented Oct 3, 2024

We could still expose them in widgets.lua as the current PR has it for external usage

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.

@myk002
Copy link
Member

myk002 commented Oct 3, 2024

right -- that's what I meant. We'd still need setters/getters at the public widgets.lua API level.

docs/dev/Lua API.rst Outdated Show resolved Hide resolved
docs/dev/Lua API.rst Outdated Show resolved Hide resolved
docs/dev/Lua API.rst Outdated Show resolved Hide resolved
@myk002 myk002 merged commit ea4b174 into DFHack:develop Oct 8, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants