Two indistinguishable events for different cases of working set reordering

[peterflynn]

DocumentManager dispatches either "workingSetReorder" or "workingSetSort" when the order of entries in the working set changes. Neither event gives any further info, so views must respond to both identically: by blowing away and rebuilding their entire rendition of the working set.

We should either:
(a) Merge the two events so views only need one listener.
(b) Provide more context for "workingSetSort" so optimized handling is possible, justifying having a second event.

I'd prefer (a) since it simplifies our code and APIs and is less error prone (can't forget to listen to one case). And because there's no evidence that we'll need the optimizations that would be enabled by (b).

[TomMalbran]

Both events are different. Maybe the naming wasn't the best thought.

• workingSetReorder is only triggered when items in the working set list are sorted by drag and drop. It is used to save the new workingSetList and to disable the automatic sort.
• workingSetSort is triggered both when a manual sort is done by clicking on the context menu items and when an automatic sort is done. It is used to save the new workingSetList, to update the working set DOM and to scroll the selected item into view.

Combining them without knowing from which type of sort it was triggered would give problems with the automatic sort disable, the DOM update and the scroll into view. The first problem can be fixed and wouldn't add much more operations needed as mentioned in: #2058. The DOM re-update for the drag and drop sort might be ok. But, the scroll into view might bring problems. If you drag and drop unselected items while the selected one not being in the view, might make it feel weird if the view scrolls to show that item after dropping. In a long working set view list where scrollbars appear, it might feel weird if the selected item ends outside the view after sorting, knowing that it would always be in the view before sorting. I am not sure how this last part can be solved without knowing what action produced the event.

Combining them and passing a boolean with the information if it was drag and drop or not, would make the combination really easy.

[redmunds]

Tom tried to merge these 2 events into 1, but the code became more complicated, and still didn't quite work. The 2 events have clearly distinguished purposes (it was just the poorly chosen names of the events that were "indistinguishable"), so the events were renamed workingSetSort and workingSetDisableAutoSorting (along with some minor code cleanup).

[peterflynn]

@redmunds: I'd like to reopen this, because the change did not address my main concern: that in order to maintain an accurate display of the working set, you need to listen for two different events even though you'll respond to both in the same way (blow away & re-render the whole list). (Actually, it's gotten more bug-prone, since the event name "workingSetDisableAutoSorting" does not in any way suggest you should listen to it to hear about reordering).

It would be much better if there was a single "workingSetReorder" event that is dispatched in all reorder cases. If we need a second event for notifying when auto-sort is turned off, that's fine, but manual drag & drop should still trigger the general "workingSetReorder" as well.

[peterflynn]

Nominating for Sprint 28 since, if we're going to change the events again, we shouldn't leave the new-old ones in place for too long...

[TomMalbran]

I understand your point now. The idea is not to merge them, but to have both actions trigger 1 event. The initial request to fix it, used the same event, and everything worked, but there was one small issue.

When you use the menu to sort the elements, the command first sorts the files internally and then triggers an event to update the DOM list. When dragging and dropping elements, you are already updating the DOM and then updating the internal structure. So the solution was to trigger the event after the drop was done, but then as it was the same event as before, it re-updated the DOM which isn't required.

What about having 1 event workingSetSort triggered by both methods and used to disable the automatic sort and a second event triggered by sorting from the context menu that is only used to update the DOM? Optionally, a parameter could be sent to tell if the DOM needs to be updated or not.

[njx]

Removing from sprint 28. Low priority.

[peterflynn]

Fixed by landing Tom's PR -- closing.