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

WSDropDown: Changing selected items and reloading data restores previously selected items #165

Closed
FlowIT-JIT opened this issue Aug 15, 2022 · 3 comments
Labels

Comments

@FlowIT-JIT
Copy link
Collaborator

FlowIT-JIT commented Aug 15, 2022

See https://jsfiddle.net/604f8uak/3/

Click the button to change selected values. Notice how the previous selection remains, even though it shouldn't. The problem is related to the use of the Action Menu. Comment out UseActionMenu(true) and it works as expected.

Code from JSFiddle:

Fit.Events.OnReady(function()
{
	// Create DropDown
	var dd = new Fit.Controls.WSDropDown("WSDropDown1");
	dd.Url("https://fitui.org/demo/GetUsers.php");
	dd.JsonpCallback("JsonpCallback"); // Loading data from foreign domain
	dd.MultiSelectionMode(true);
	dd.Width(400);
	dd.DropDownMaxHeight(150);
	dd.UseActionMenu(true);
	dd.OnRequest(function(sender, eventArgs)
	{
		eventArgs.Request.SetParameter("Parent", eventArgs.Node && eventArgs.Node.Value() || "");
	});
	dd.Render(document.querySelector("#DropDownContainer"));
	
	// Set control value and resolve display values
	dd.Value("[email protected];[email protected];[email protected];[email protected]")
	dd.AutoUpdateSelected(); // Load display values for selected values
	
	// Create button that changes control value and resolve display values again.
	// BUG: Previous selection is kept intact along with new selection which is obviously
	// not the intended behaviour. We can work around the bug by not calling UseActionMenu(true).
	var btn = new Fit.Controls.Button();
	btn.Title("Change selection and update display values");
	btn.OnClick(function()
	{
		dd.Value("[email protected];[email protected];[email protected];[email protected];[email protected]");
		dd.ClearData();
		dd.AutoUpdateSelected(); // Load display values for selected values
	});
	btn.Render(document.querySelector("#ButtonContainer"));
	
	// Make DropDown available in console
	window.dd = dd;
});
@FlowIT-JIT FlowIT-JIT added the bug label Aug 15, 2022
@FlowIT-JIT
Copy link
Collaborator Author

The changed value on line 28 does not propagate to the TreeView control if the Action Menu is currently the active picker control. Therefore the WSTreeView does not know that the value has changed, so when ClearData() and AutoUpdateSelected() is called afterwards, the value known to the WSTreeView is restored once data has reloaded.

image

The reason why the selection is restored after reload can be found in the implementation for AutoUpdateSelected():

tree.Reload(true, function(sender)

Notice the true flag passed to tree.Reload(..) - this instructs the TreeView to restore selected items once data is reloaded. The TreeView instance is accessible via dropDown.GetTreeView(), and it should reflect the state of selected items in the DropDown control. But if the TreeView does not contain the correct value, then it will select the wrong nodes, which then propagates to the DropDown control.

@FlowIT-JIT
Copy link
Collaborator Author

FlowIT-JIT commented Aug 16, 2022

The solution might be as simple as to ensure the WSTreeView has the most recent value (selection) using tree.SetSelections(me.GetSelections()); before the call to tree.Reload(true, function) here:

tree.Reload(true, function(sender)

image

However, with this approach we do not ensure that the built-in WSTreeView's selection state is kept up to date at all time - although that's not a new problem. But it does mean that external code can't rely on DropDown's selection state being in sync with the WSTreeView instance's selection state, which is accessible through dropdown.GetTreeView().Selected().
If we are okay with this, we could probably just change the true flag to false in the call to tree.Reload(..) to prevent an outdated selection to be restored once data is reloaded. The WSTreeView's selection state is either up-to-date if it is the active picker, or it will be updated when it becomes the active picker control later:

image

But with this approach we would probably need to update the WSTreeView's selection state once Reload(..) has completed in AutoUpdateSelected(..), if the WSTreeView instance is currently the active picker. But beware that tree.Reload(..) is called in ensureTreeViewData() in WSDropDown where we might need the same fix - but ensureTreeViewData() is also called from AutoUpdateSelected(..) so make sure the selection is not updated twice!
I personally do not like this solution. It does not solve the problem with inconsistency between selection state in the WSDropDown and WSTreeView, and the solution (code wise) is more complex than it have to be.

For an always up-to-date selection state in the WSTreeView instance we could instead make sure to always update the WSTreeView's selection state when it is not the active picker control, like the example below demonstrates:

image

image

Unfortunately this comes with a major performance penalty when working with huge amounts of data and a very large selection of these data, since SetSelections(..) eventually ends up resolving selected nodes using a recursive lookup through GetChild(nodeValue, true), as shown in the profiler below, where we are working on a data set with 16.000+ nodes which are all selected in the DropDown control.

image

For an always-up-to-date TreeView we will most likely need to increase the performance for GetChild(..) first.

Rather than updating the entire selection state on every change, it would make more sense to only update the node that was actually selected or de-selected. But that solution would be more complicated, involving overriding the DropDown's AddSelection(..), RemoveSelection(..), ClearSelections(), and Value(..) functions in WSDropDown, and we would need a mechanism that would prevent DropDown (from which WSDropDown inherits) from synchronizing these changes from the WSTreeView back to the WSDropDown from where the change originated.

FlowIT-JIT added a commit that referenced this issue Aug 17, 2022
#165: Changes to WSDropDown's value was not synchronized to WSTreeView picker if one of the WSListView pickers (for search and action menu items) was the active picker. This resulted in the previous value being restored when calling wsDropDown.ClearData() and wsDropDown.AutoUpdateSelected() to resolve updated display values for the newly assigned dropdown value. This has now been fixed by ensuring that WSTreeView is always kept up to date with the current selection state in WSDropDown as they happen, rather than "catching up" when WSTreeView is set as the active picker control.

#166: Changes made to selection state via a WSDropDown's picker control directly (e.g. dd.GetTreeView().EnsureData(function(sender) { sender.GetChild("ROOTNODE").Selected(true); });) would not propagate to host control (WSDropDown) unless pull down menu had been opened first, which would set up the necessary synchronization events. This has now been resolved.

Also fixed a bug in WSTreeView where clearing the control with Value("") would add a preselection with an empty value.
@FlowIT-JIT
Copy link
Collaborator Author

Synchronization problem fixed. TreeView is now kept up to date at all times. The implementation was based on the last fix suggested - it did not require additional logic to prevent an infinite loop - it was already in place.

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

No branches or pull requests

1 participant