Revise entry sorting: refactor for readability, remove duplicates, sort rest alphabetically#52
Revise entry sorting: refactor for readability, remove duplicates, sort rest alphabetically#52LinqLover wants to merge 1 commit intoLeonMatthes:masterfrom
Conversation
…rt rest alphabetically Duplicates can occur for variables that are also selectors in untyped models, or also due to custom hooks.
|
Please discuss #39 (comment) before approving this. :) |
LeonMatthes
left a comment
LeonMatthes
left a comment
There was a problem hiding this comment.
Overall, nice refactoring, no objections to removing duplicates in principle. It shouldn't hurt performance though if possible.
If it's not too much trouble, could you benchmark this by creating a fake ECModel instance with random entries and running the old and new sortEntries functions?
If performance does indeed degrade, it may be enough and faster to only remove duplicates that are displayed directly after one another.
e.g. filter out:
aCuefollowed byaCue
As that should be doable in O(n) with low constant overhead and covers the most obvious case as described in #39
May even be doable in the rendering code, which should be really fast 🤔
| | seen | | ||
| entries sort: | ||
| [:entry | entry matchNarrowString: narrowString] descending | ||
| , [:aEntry :bEntry | aEntry <= bEntry ifTrue: [-1] ifFalse: [0]] |
There was a problem hiding this comment.
Where does the , operator on BlockClosure come from???
Is that new? It doesn't work on my system.
Or am I reading the syntax incorrectly?
There was a problem hiding this comment.
Ah, okay, descending returns a PropertySortFunction that can be comma-chained...
That's wild 😅, but okay.
| ifFalse: [aPriority >= bPriority]] No newline at end of file | ||
| | seen | | ||
| entries sort: | ||
| [:entry | entry matchNarrowString: narrowString] descending |
There was a problem hiding this comment.
Huh, cool, no idea #descending worked on BlockClosure 👍
| , [:aEntry :bEntry | (aEntry contents compare: bEntry contents caseSensitive: ECPreferences caseSensitive) - 2]. | ||
|
|
||
| "remove any duplicates" | ||
| seen := Set new. |
There was a problem hiding this comment.
I don't mind removing duplicates, as discussed in #39 , duplicates are just a side-effect of adding all selectors in the untyped model.
There's no real point in displaying them.
How is performance though? First sorting, then converting into a Set sounds a bit expensive 🤔
Especially in the untyped model that contains all selectors.
2 participants
Duplicates can occur for variables that are also selectors in untyped models, or also due to custom hooks.