Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added standalone multisort implementation to file_browser example #31

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

telamonian
Copy link
Contributor

@telamonian telamonian commented Jun 2, 2020

Adds a standalone implementation of multisort to the file_browser.html example. Slightly simplified, but mostly equivalent to the one recently removed from events.js:

image

src/js/utils.js Outdated Show resolved Hide resolved
@telamonian
Copy link
Contributor Author

telamonian commented Jun 2, 2020

Multicolumn sort now tree-aware:

image

Got a bit messy, but is fully functional. I'll try and clean it up

Cleanup done. Tree-aware multisort implementation still significantly more complex than multisort implementation for flat lists. The sort-in-place pattern becomes impractical, and you need a touch of recursion to deal gracefully with the arbitrary depth tree.

Which made me consider: does V8 do tail call optimization?

@telamonian telamonian force-pushed the add-sort-event-example branch 3 times, most recently from 57ad18b to 4aab87e Compare June 3, 2020 01:40
Comment on lines +296 to +315
const offsets = [200, 106, 12];
for (const offset of offsets) {
const vals = [];
for (let i = 6 + offset; i < 16 + offset; i++) {
const cell_values = await cell_values_at(0, i, 4, 300);
vals.push(cell_values[0]);
}
expect(vals).toEqual([
"add dog/",
"add charlie/",
"add baker/",
"add able/",
"file_89.txt",
"file_88.txt",
"file_87.txt",
"file_86.txt",
"file_85.txt",
"file_84.txt",
]);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is valid, but runs into some kind of issue with scrollTo when offset === 12. Basically, the failure seems to occur due to the fact that scrollTo(0, 24, 4, 300) scrolls to the same row as scrollTo(0, 25, 4, 300):

  ● file_browser.html › sorts tree filebrowser › uses same sort at all levels, sort then expand tree

    expect(received).toEqual(expected) // deep equality

    - Expected  - 1
    + Received  + 1

    @@ -3,10 +3,10 @@
        "add charlie/",
        "add baker/",
        "add able/",
        "file_89.txt",
        "file_88.txt",
    -   "file_87.txt",
    +   "file_86.txt",
        "file_86.txt",
        "file_85.txt",
        "file_84.txt",
      ]

@telamonian
Copy link
Contributor Author

I added sorting tests to file_browser.test.js. The tests for sorting a flat filebrowser all pass. The tests for sorting a filebrowser tree with several expanded nodes work in principle, but are currently failing due to an issue with scrollTo (see note in code here).

@texodus Not sure exactly what the problem is, but the scrollTo issue/test failures are sensitive to the exact value of viewport height. Any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant