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

Fix selection highlight issue for unordered layer lists set by script #3512

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lhiginbotham
Copy link
Contributor

  • fixes Scripting: Assigned map.selectedLayers displays incorrectly #3451 where layer selection highlighting was bugging out if the newly set selection was not ordered by layer ID
  • this seems to be an event interplay issue with the following sequence:
    • script sets the layer selection to some array
    • first step in handling that is to set the currently selected layers to be the full array, triggering LayerView::selectedLayersChanged to select the whole range
    • second step is to make sure that the mCurrentLayer variable is pointing to an object within that array selection, which in this bad case it is not, so we end up calling setSelectedLayers with one item, triggering LayerView::selectedLayersChanged again with a single item and effectively clearing out the selection we had just highlighted
  • fix this by doing the second step before the first step

	- fixes mapeditor#3451 where layer selection highlighting was bugging out if the newly set selection was not ordered by layer ID
	- this seems to be an event interplay issue with the following sequence:
		- script sets the layer selection to some array
		- first step in handling that is to set the currently selected layers to be the full array, triggering LayerView::selectedLayersChanged to select the whole range
		- second step is to make sure that the mCurrentLayer variable is pointing to an object within that array selection, which in this bad case it is not, so we end up calling setSelectedLayers with one item, triggering LayerView::selectedLayersChanged again with a single item and effectively clearing out the selection we had just highlighted
	- fix this by doing the second step before the first step
@lhiginbotham
Copy link
Contributor Author

FYI @bjorn @eishiya let me know what you think about this change (you will be a better judge of this fix than I am, a newcomer 😄)

@bjorn
Copy link
Member

bjorn commented Oct 27, 2022

let me know what you think about this change

There was probably a reason for having those function in that order, such that reversing it has a high chance of introducing another issue elsewhere. We'll have to be careful with this one.

Unfortunately I won't have time anymore in October to investigate this, but for your Hacktoberfest contributions we can still get #3513 merged. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scripting: Assigned map.selectedLayers displays incorrectly
2 participants