Fix unused options in readdir system VFS adapter #164

mahsashadi · 2021-08-29T04:50:54Z

This change sends options correctly in readdir request.

Relevant:

andersevenrud

Could you please amend the commit message so that it says:

Fix unused options in readdir system VFS adapter

andersevenrud

I was just about to merge all your changes, and then it suddenly dawned on me:

The internal client options are now sent over to the server... which I feel like should be filtered.

A good example of a scenario like this is the following:

osjs-client/src/vfs.js

Lines 70 to 75 in 385b38f

    
           const handleDirectoryList = (path, options) => result => 
        
             Promise.resolve(result.map(stat => createFileIter(stat))) 
        
               .then(result => transformReaddir(pathToObject(path), result, { 
        
                 showHiddenFiles: options.showHiddenFiles !== false, 
        
                 filter: options.filter 
        
               }));

So before we can finally publish all the changes this needs to be addressed 🤓

andersevenrud · 2021-09-06T22:08:05Z

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:

Take all of the files in a readdir request
Filter out the ones that does not pass showHiddenFiles
Pass additional filter option
Go over the list, then:
1. Put all directories into a group with an internal filter
2. Put all the "special" into a group with an internal filter
3. Put all the rest of the "regular" files into a group
Sort all groups alphabetically and merge them together into a flat array

Which would mean that in some cases you might see this:

👎 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

👌 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

👍 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 🤔.

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 😅

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?

mahsashadi · 2021-09-07T09:41:27Z

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 🤔

mahsashadi · 2021-09-07T10:57:43Z

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.

mahsashadi · 2021-09-07T11:08:15Z

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

andersevenrud · 2021-09-07T13:48:28Z

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.

mahsashadi · 2021-09-08T09:55:36Z

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?

mahsashadi · 2021-09-08T10:19:38Z

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:

Is this format ok for detecting server capabilities?

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
    }),

For servers without sorting capabilities (as system adapter I think), should we use this kind of sorting in client-side, and per page:?

[dir] 0
[file] 1
[dir] A
[dir] B
[file] B
[dir] C
[dir] a
[file] c
[dir] d

Any other agreement if needed ...

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)

andersevenrud · 2021-09-08T16:49:10Z

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:

I'll merge this PR now
You'll add another PR with the filtering of client options
Then rebase your other PR
I will merge the new option serialization stuff
Then open a new PR for the capabilities

Does this sound good ? I know it's been a long process so far, but I really wanna make sure we do this right 😊

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 🤦‍♂️ I'll review that.

src/vfs.js

mahsashadi · 2021-09-11T12:36:10Z

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 👍

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.

mahsashadi · 2021-09-19T11:00:18Z

Hi.
I again find some time to work on paging project :)
So is there anything else to be done on this branch?

andersevenrud · 2021-09-19T13:22:33Z

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

andersevenrud · 2021-09-19T13:23:57Z

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/

mahsashadi · 2021-09-25T12:57:23Z

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.

andersevenrud · 2021-09-25T13:35:12Z

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 :)

andersevenrud · 2022-07-26T22:26:15Z

Just a small reminder. I would appreciate if you'd have another look at this when #182 is merged :)

mahsashadi mentioned this pull request Aug 29, 2021

Add option interface for VFS readdir #163

Open

andersevenrud self-requested a review August 29, 2021 21:07

andersevenrud approved these changes Aug 31, 2021

View reviewed changes

andersevenrud requested changes Sep 2, 2021

View reviewed changes

mahsashadi changed the title ~~Update system.js~~ Fix unused options in readdir system VFS adapter Sep 3, 2021

Fix unused options in readdir system VFS adapter

0701f30

mahsashadi force-pushed the patch-1 branch from bdd7301 to 0701f30 Compare September 3, 2021 15:14

andersevenrud self-requested a review September 3, 2021 18:08

andersevenrud approved these changes Sep 3, 2021

View reviewed changes

This was referenced Sep 5, 2021

Improve unserialization of VFS URL query parameters os-js/osjs-server#56

Closed

Improve serialization of VFS URL query parameters #165

Closed

andersevenrud requested changes Sep 6, 2021

View reviewed changes

Ignore vfs client options from being send to the server

14615e5

andersevenrud requested changes Sep 8, 2021

View reviewed changes

src/vfs.js Outdated Show resolved Hide resolved

Preserving the options passed into filterOptions method

a5e92a1

mahsashadi mentioned this pull request Nov 22, 2021

Adding pagination capability os-js/osjs-filemanager-application#37

Open

andersevenrud self-assigned this Apr 10, 2022

mahsashadi mentioned this pull request Jul 27, 2022

Support serialization of url query parameters #182

Merged

andersevenrud mentioned this pull request Jul 28, 2022

VFS Capability detection, other extensions os-js/OS.js#804

Open

andersevenrud assigned mahsashadi and unassigned andersevenrud Aug 1, 2022

mahsashadi mentioned this pull request Aug 9, 2022

Add VFS context aware sorting #189

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix unused options in readdir system VFS adapter #164

Fix unused options in readdir system VFS adapter #164

mahsashadi commented Aug 29, 2021 •

edited by andersevenrud

Loading

andersevenrud left a comment

andersevenrud left a comment •

edited

Loading

andersevenrud commented Sep 6, 2021 •

edited

Loading

mahsashadi commented Sep 7, 2021 •

edited

Loading

mahsashadi commented Sep 7, 2021 •

edited

Loading

mahsashadi commented Sep 7, 2021

andersevenrud commented Sep 7, 2021

mahsashadi commented Sep 8, 2021

mahsashadi commented Sep 8, 2021 •

edited

Loading

andersevenrud commented Sep 8, 2021 •

edited

Loading

mahsashadi commented Sep 11, 2021

mahsashadi commented Sep 19, 2021

andersevenrud commented Sep 19, 2021

andersevenrud commented Sep 19, 2021

mahsashadi commented Sep 25, 2021 •

edited

Loading

andersevenrud commented Sep 25, 2021

andersevenrud commented Jul 26, 2022

	const handleDirectoryList = (path, options) => result =>
	Promise.resolve(result.map(stat => createFileIter(stat)))
	.then(result => transformReaddir(pathToObject(path), result, {
	showHiddenFiles: options.showHiddenFiles !== false,
	filter: options.filter
	}));

Fix unused options in readdir system VFS adapter #164

Are you sure you want to change the base?

Fix unused options in readdir system VFS adapter #164

Conversation

mahsashadi commented Aug 29, 2021 • edited by andersevenrud Loading

andersevenrud left a comment

Choose a reason for hiding this comment

andersevenrud left a comment • edited Loading

Choose a reason for hiding this comment

andersevenrud commented Sep 6, 2021 • edited Loading

mahsashadi commented Sep 7, 2021 • edited Loading

mahsashadi commented Sep 7, 2021 • edited Loading

mahsashadi commented Sep 7, 2021

andersevenrud commented Sep 7, 2021

mahsashadi commented Sep 8, 2021

mahsashadi commented Sep 8, 2021 • edited Loading

andersevenrud commented Sep 8, 2021 • edited Loading

mahsashadi commented Sep 11, 2021

mahsashadi commented Sep 19, 2021

andersevenrud commented Sep 19, 2021

andersevenrud commented Sep 19, 2021

mahsashadi commented Sep 25, 2021 • edited Loading

andersevenrud commented Sep 25, 2021

andersevenrud commented Jul 26, 2022

mahsashadi commented Aug 29, 2021 •

edited by andersevenrud

Loading

andersevenrud left a comment •

edited

Loading

andersevenrud commented Sep 6, 2021 •

edited

Loading

mahsashadi commented Sep 7, 2021 •

edited

Loading

mahsashadi commented Sep 7, 2021 •

edited

Loading

mahsashadi commented Sep 8, 2021 •

edited

Loading

andersevenrud commented Sep 8, 2021 •

edited

Loading

mahsashadi commented Sep 25, 2021 •

edited

Loading