-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Fix #2076: Two indistinguishable events for different cases of working set reordering #3080
Conversation
…ethod with a Command object. Refactored some code. Fixed some comments.
I added a few fixes here so that #3440 is still fixed after this request is merged. |
Reassigned to @redmunds |
@TomMalbran Sorry it's taken so long to get to this one. I started to review it, but there's a merge conflict. Can you merge the latest code? Thanks. |
Sure, I'll fix the merge issue. Thanks for looking into this. |
@redmunds Merged with master and fixed the conflict issue. |
* Triggers a WorkingSet Sort event | ||
*/ | ||
function triggerWorkingSetSort() { | ||
$(exports).triggerHandler("workingSetSort"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The sortWorkingSet()
function also calls $(exports).triggerHandler("workingSetSort")
but the event does not seem to be handled anywhere. It should either be removed, or call triggerWorkingSetSort()
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is used on the WorkingSetView.js#L249. It was initially used to save the working set state into the preferences, which isn't required so is now mainly used to disable the automatic sort and have both sort methods trigger 1 event.
Done with initial review. |
@redmunds Fixed the comment issues. Check on my replies about the event to see what we can do next. Thanks. |
Done with second review. |
I updated a bit the comment on the |
Thanks! Looks good. Merging. |
Fix #2076: Two indistinguishable events for different cases of working set reordering
Note: I've reopened #2076 for more discussion since this doesn't appear to address the main confusion/pitfall I was concerned about (and in some sense the event name change may have made it worse). |
I went back to the working set sort and managed to merge both events
workingSetReoder
andworkingSetSort
into oneworkingSetSort
and leaving almost the same functionality as before fixing #2076.