Open mahsashadi opened 2 years ago
The internal client options are now sent over to the server... which I feel like should be filtered.
Actually, this brings up another interesting point! Should the client or the server be responsible for handling these options when you're using a "pagination" system like this.
Because the client sorting algorithm works like this:
showHiddenFiles
filter
optionWhich would mean that in some cases you might see this:
:-1: Hm... with client sorting and paging, this looks strange
-- PAGE 1 --
[dir] a
[dir] b
[dir] c
[file] bar
-- PAGE 2 --
[dir] d
[file] foo
[file] foo 1
[file] foo 2
-- PAGE 3 --
[dir] g
[dir] h
[file] i
[file] j
:ok_hand: This is it usually looks when you only get "one page" with client sorting
[dir] a
[dir] b
[dir] c
[dir] d
[dir] g
[dir] h
[file] bar
[file] foo
[file] foo 1
[file] foo 2
[file] i
[file] j
:+1: No sorting on the client side
-- PAGE 1 --
[dir] a
[dir] b
[file] bar
[dir] c
-- PAGE 2 --
[dir] d
[file] foo
[file] foo 1
[file] foo 2
-- PAGE 3 --
[dir] g
[dir] h
[file] i
[file] j
The original design of readdir was to get "everything" at once, so we need to settle on something that does not "look wrong".
I feel like the option to disable client sorting is a good choice, especially since most filesystems will sort alphabetically no mater the type (dir / file), which is a lot more "natural".
As far as I see it, the best way to solve this is to add a new method to the adapter that can tell the client what it's supposed to do :thinking:.
const adapter = (core) => {
return {
capabilities: async (file /*optional*/) => ({ // As async and with file as arument to future-proof implementation
sort: true // Sorting is done on the adater side (aka "server")
}),
readdir: async ({path}, options) => {}
};
};
I hope that makes sense... it's very late for me here, and I've been busy with other projects all day :sweat_smile:
Also, I had to edit this like a million times, so what you see in the email is not what I originally had in mind....
Maybe we can just merge everything as-is then tackle this afterwards?
he internal client options are now sent over to the server... which I feel like should be filtered.
I see.
As an another example, by opening a Open Dialog , since its options.filter has function typeof (which is not considered), it is sent to the server as options.filter.undefined
. so it will not work!
So do you believe that filtering should be done in encodeQueryData
method? Or some deeper solution is needed to completely separate server and client options :thinking:
So as I understand, you mean since most filesystems retuen the whole list in an alphabetic order, we can also make sure that every single page is already sorted (even beside other pages as a whole) if vfs.capabilities().sort
returns true. If not, we should sort client-sidely, but just in alphabetic order (dir or files does not matter).
BTW, I did not understand what and why is the file
argument in capabilities
.
In my case, the list returned by my storage API is ordered alphabetically, but files with uppercase letters precede the ones with lowercase letters.
[dir] 0
[file] 1
[dir] A
[dir] B
[file] B
[dir] C
[dir] a
[file] c
[dir] d
As an another example, by opening a Open Dialog
This fits under the scenario I described. This will create a client-side option.
So do you believe that filtering should be done in encodeQueryData method?
Probably in the vfs abstraction per-function:
const filterOptions = (ignore, options) => {} // Returns a new "options" object without properties from "ignore"
export const readdir = (adapter, mount) => (path, options = {}) =>
adapter.readdir(pathToObject(path), filterOptions(['a', 'b', 'c'], options), mount)
.then(handleDirectoryList(path, options));
BTW, I did not understand what and why is the file argument in capabilities.
Because I wanted to make sure that this function can return capabilities based on files, not just global settings. Think for example if in the future there needs to be a new check for readfile.
In my case, the list returned by my storage API is ordered alphabetically, but files with uppercase letters precede the ones with lowercase letters.
This is what I suspected. This is what most filesystems do, which is fine.
I commited some changes, I think we only need to filter options in getters (readdir, readfile, exists, stat), right? Except readdir, I didn't find options needed to be filtered in other getters, Is there any?
This is what I suspected. This is what most filesystems do, which is fine.
So to start working on this point, I think we should have some agreement:
capabilities: async (file /*optional*/) => ({ // As async and with file as arument to future-proof implementation
sort: true, // Sorting is done on the adater side (aka "server")
pagination: true // pagination is supported by this adapter
}),
[dir] 0
[file] 1
[dir] A
[dir] B
[file] B
[dir] C
[dir] a
[file] c
[dir] d
Maybe we can just merge everything as-is then tackle this afterwards?
I also think it's better to merge all these changes (after completing option filtering), and then work on this new point in two new PR (server and client)
I also think it's better to merge all these changes (after completing option filtering), and then work on this new point in two new PR (server and client)
I think we'll take this in these steps:
Does this sound good ? I know it's been a long process so far, but I really wanna make sure we do this right :blush:
Note to self: Since this is a semi-breaking change (both client and server** needs update together) I have to write some notices about this in the migration article in the online manual.
Edit: I just realized that you did 2) here :man_facepalming: I'll review that.
Does this sound good ? I know it's been a long process so far, but I really wanna make sure we do this right
You are completely right, and I do wanna the same :+1:
I just realized that you did 2) here I'll review that.
Sorry, since you said it is needed to be addressed before publishing, I did it here. So as you said after finishing these PR, maybe we can do sorting stuff in new one.
Hi. I again find some time to work on paging project :) So is there anything else to be done on this branch?
Hm. I don't think there's anything to do in this branch. This branch actually contains more changes than I really feel comfortable with, hehe.
I need to set off a day where I look over all this work again and then finally do a merge. Thankfully you don't need me to do that in order for you to continue the work on the pagination in the file manager :D
in order for you to continue the work on the pagination in the file manager
You probably already know this -- and might even be using this right now: you can just use your forks with your changes in your installation: https://manual.os-js.org/guide/modules/
you can just use your forks with your changes in your installation
Yes, but as I need to make pagination changes in the same fork package (osjs-client), I think it causes problem.
Yes, but as I need to make pagination changes in the same fork package (osjs-client), I think it causes problem.
I don't understand what you mean by this. If you wanna test/use all these changes, simply make a new branch where you merge #164 and #164 :)
Just a small reminder. I would appreciate if you'd have another look at this when #182 is merged :)
This change sends options correctly in readdir request.
Relevant: