Jemt / Fit.UI

Fit.UI is a JavaScript based UI framework built on Object Oriented principles
http://fitui.org
GNU Lesser General Public License v3.0
19 stars 7 forks source link

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

Closed FlowIT-JIT closed 1 year ago

FlowIT-JIT commented 1 year ago

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("james@server.com;hans@server.com;ole@server.com;michael@server.com")
    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("martin@server.com;anders@server.com;jacob@server.com;jan@server.com;christian@server.com");
        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 commented 1 year ago

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(): https://github.com/Jemt/Fit.UI/blob/28ae44dfd5afcbbdec2fe29cd37a69df4512431d/Controls/DropDown/WSDropDown.js#L514

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 commented 1 year ago

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: https://github.com/Jemt/Fit.UI/blob/28ae44dfd5afcbbdec2fe29cd37a69df4512431d/Controls/DropDown/WSDropDown.js#L514

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 commented 1 year ago

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.