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.

Sign up for GitHub

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

Jump to bottom

Lua Element:GetElementsByTagName result is 0-indexed #237

Closed
nimble0 opened this issue Sep 23, 2021 · 3 comments · Fixed by #247
Closed

Lua Element:GetElementsByTagName result is 0-indexed #237

nimble0 opened this issue Sep 23, 2021 · 3 comments · Fixed by #247
Labels
Lua Lua binding issues

Comments

@nimble0
Copy link

nimble0 commented Sep 23, 2021

Reproducing:

  • Replace Samples/luainvaders/data/main_menu.rml (using luainvaders just so Lua is enabled) with:
<rml>
	<head>
		<title>Main Menu</title>
		<link type="text/template" href="window.rml" />
		<script>
			function test(document)
				local e = document:GetElementById("test")
				print(e.child_nodes[1].inner_rml)
				print(e:GetElementsByTagName("div")[1].inner_rml)
			end
		</script>
	</head>
	<body onload="test(document)">
		<div id="test">
			<div>1</div>
			<div>2</div>
			<div>3</div>
			<div>4</div>
		</div>
	</body>
</rml>
  • Run luainvaders

Expected result (in console):

... initialisation stuff ...
1

1

Actual result (in console):

... initialisation stuff ...
1

2

Obviously this could be breaking for someone if fixed but the inconsistency with child_nodes (as well as being 0-indexed in Lua) isn't great either.
@mikke89 mikke89 added the Lua Lua binding issues label Sep 26, 2021
@mikke89
Copy link
Owner

mikke89 commented Sep 26, 2021

Thanks for reporting! Indeed, I'd prefer this to be 1-indexed in Lua indeed.

See also this related discussion: #137
It is possible that there were some changes introduces there. In any case, I think this would be a welcome change even though it is breaking. I'll of course document it as such. Can you find other indexing methods that should also be changed?

Are you able to make a PR for this one?

@nimble0
Copy link
Author

nimble0 commented Oct 1, 2021

I looked through the Lua API reference and these are the properties/functions I found that could be affected by indexing:

  • Context.documents

  • DataSource:GetRow

  • ElementDataGrid.rows

  • ElementDataGridRow.parent_relative_index

  • ElementDataGridRow.table_relative_index

  • Element.child_nodes

  • Element:GetElementsByTagName

  • ElementFormControlSelect.options

  • ElementFormControlSelect.selection

  • ElementFormControlSelect:Add

  • ElementFormControlSelect:Remove

  • ElementTabSet.active_tab

  • ElementTabSet:SetPanel

  • ElementTabSet:SetTab

  • rmlui.contexts

I didn't check the DataGrid stuff. Just didn't want to learn it and set it up and it's deprecated anyway.

I couldn't get the index parameter of these functions to work at all, they just added to the end, whatever the value:

  • ElementTabSet:SetPanel
  • ElementTabSet:SetTab

Of the rest of them, these are 0-indexed:

  • ElementFormControlSelect.selection
  • ElementFormControlSelect:Add
  • ElementFormControlSelect:Remove
  • ElementTabSet.active_tab

I see another problem if ElementTabSet.active_tab is changed to be 1-indexed: tabchange events include it as a parameter. This might affect ElementFormControlSelect as well.

@mikke89
Copy link
Owner

mikke89 commented Oct 3, 2021

That's a very detailed list, good work!

I think it's okay to leave out the datagrid things with them being deprecated, they can have the old behavior.

A PR to change the indexing would be very much welcome, and perhaps (a separate?) one for fixing ElementTabSet:SetPanel and ElementTabSet:SetTab. I'll leave to your decision whether or not to change the indexing of ElementTabSet and/or change the tab_index event parameter when accessed from Lua. I can see pros and cons either way, so I am fine with any solution. In any case, we should document the chosen behavior.

This was referenced Oct 16, 2021
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Lua Lua binding issues
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix Lua 0-indexing nimble0/RmlUi
2 participants
@mikke89 @nimble0