-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Conversation
Reviewing |
@@ -193,6 +193,10 @@ define({ | |||
"CMD_INCREASE_FONT_SIZE" : "Increase Font Size", | |||
"CMD_DECREASE_FONT_SIZE" : "Decrease Font Size", | |||
"CMD_RESTORE_FONT_SIZE" : "Restore Font Size", | |||
"CMD_SORT_WORKINGSET_BY_NAME" : "Sort by Name", | |||
"CMD_SORT_WORKINGSET_BY_TYPE" : "Sort by Type", | |||
"CMD_SORT_WORKINGSET_BY_MRU" : "Sort by Recently View", |
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.
How about "Sort Recent"?
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.
"Sort Recent" doesn't sounds like it will sort the working set by the MRU order (recently view order). I think it sounds like it would use the most recent order instead. Maybe "Sort by View Time" or "Sort by Selection".
Initial review complete. Great work! |
Thanks. Made changes and added some questions. Note: Some comments ended up in the outdated diff. |
I think I see where I misunderstood...maybe. What I really was expecting was sorting by the order files were added to the working set. MRU was originally intended for the Next/Previous Document commands. It doesn't seem to work here for sorting the working set. It seems like with auto-sort on, new documents always get added to the top. When off, new documents get added to the bottom. This MRU+auto-sort-off seems roughly equivalent to the current behavior without sorting. Would it make more sense to offer a "Sort by Newest" or "Sort by Added"? I would actually prefer the to see documents get added to the working set to appear at the top instead of the bottom. Also, I noticed that no default sorting option is set and once you set a sorting option you can't unset it. I propose you change MRU to "Sort by Added" and make it the default option. What do you think? |
Review complete. See my comments about MRU. If you agree, maybe we can simply remove MRU and implement the proposed option as a follow-up pull request. |
After trying it I didn't found MRU sort that useful either. But as it was proposed here: #1788 so I added it. The list is permanently changing as it doesn't just add to the top, if you open documents in the working set and then add a new one, this documents would also jump t the top. "Sort by Added/Newest/Opened" could work. I could have a working set list saved and changing it on every document that is opened and use it to then sort the working set. You can unset it, because once you set it and then click it again, I would never know if the user wants to sort again with that method or disable it. It doesn't really need to get disable, since when you don't have the automatic sort option enable it does nothing. But it could get disable if the user manually sorts the working set or adds files without the automatic sort enable. The default sort seems that would only work to enable the automatic sort without needing to first enable any sort method. Since it shouldn't sort the working set at the beginning if the user doesn't click on that option. |
I made all the fixed required and changed MRU sort for a new Added sort and made it the default option. I had to add a new array in DocumentManager to save AddedOrder, and then used this in the sort function. |
Looks great! Merging. |
This adds to the working set context menu 3 sorting methods and an automatic sort option. When the automatic sort option and a valid sort method are selected, the working set will be sorted when a file is added to it.
This pull also adds an API to easily add new sort methods by using the register function in SortUtils. This function would receive a compare function, used to sort the workingSet, an optional list of events that will be called for the automatic sort (the automatic sort will be disable if the list is empty) and an optional automatic function that will be called when the automatic events are triggered.