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

toDict() for lazy folder without childrens should add children with empty array instead of no adding them at all. #989

Closed
Roinoss opened this issue Jan 12, 2020 · 8 comments

Comments

@Roinoss
Copy link

Roinoss commented Jan 12, 2020

Test in modified: https://wwwendt.de/tech/fancytree/demo/index.html#sample-multi-ext.html
ISSUE:
1.: Copied to CLIPBOARD lazy folder without children:
data: {…} data: Object { id: "68" } expanded: false folder: true key: "_5" lazy: true partsel: false selected: false title: "fff" type: "folder <prototype>: Object { … } mode: "copy" <prototype>: Object { … }

2.: Paste node from CLIPBOARD.
3.: Pasted folder arrives as expandable, as children != empty array means not loaded yet.

Issue fix ideas:
in lib: function toDict() adds to returned object "children: [],", if there is "lazy: true" option on.
in code that uses lib: echo '[]' from lazyLoad ajax or check in lazyLoad: function(event, data) { ... } for children calling it folder.

@mar10
Copy link
Owner

mar10 commented Jan 25, 2020

I would think that this is the expected behavior:
since the lazy node has children (even if only on the server and not in the tree markup yet) I would expect copy/paste to transfer those children as well - so I can expand/load them at the new location.

Closing this for now. but let me now if you have reasons for a different use case.

@mar10 mar10 closed this as completed Jan 25, 2020
@Roinoss
Copy link
Author

Roinoss commented Jan 27, 2020

I still think it isn't expected behavior (and maybe it was not clear it according to your answer: it is a case when folder is lazy:true but it don't have children), because:

  1. I use all folders as lazy and after copying EMPTY folder new copy still needs to load new data(no data) - it causes additional, unnecessary click for apps end user and, as tree knows new folder empty state, call to server to get 0 data.
  2. There is some inconsistency in expandable icons, after copying EMPTY lazy folder it is in 'expanded' state and it need two clicks to load its data (empty data):
  • 1st to close expanded state.
  • 2nd to expand and trigger lazy load (with no data in return).
  • as an effect folder lose expand icon in any state.

Copying not empty folder creates copy with 'not expanded state'.

@mar10
Copy link
Owner

mar10 commented Jan 28, 2020

So your case is

  1. have a lazy folder
  2. expand it -> the server returns []
  3. copy (Ctrl-C) that node
  4. paste it to another parent
  5. the new copy needs to be loaded again

?

@mar10 mar10 reopened this Jan 28, 2020
@Roinoss
Copy link
Author

Roinoss commented Jan 29, 2020

After some more testing, looks like there are two bugs or 1st bug causes second:

  1. With independent (to empty/have content folder state) expanded state - you can expand folder without content and its save that state to its copies (on click on folder - expanded state should stay false if content is empty).
  2. With unnecessary XHR call to server when new copy of the folder is lazy and empty. XHR on isn't called when folder-copy is not empty and original was expanded before being copied.
    (Jump to fancytree bugged code lines at the end of this post)

//Tested on my page:
0. Tree is loading on new page with not expanded Tier0 folders.
0.1. All folders loaded are Lazy.
0.2.
a - case for folder with children: lazy: true, children = none, false or not empty array.
b - case for folder without children: lazy: true, children = empty array ( = [] ).
0.3.
'>' - 'expandable' folder state icon
'V' - 'expanded' folder state icon

  1. Chose some folder.
    a. has > icon near them,
    b. no state icon near folder

  2. Copy it.

  3. Open browser console log and observe XHR requests.

  4. Paste folder.

  5. Pasted folder have state icon:
    a. same as original from time of the copy action
    b. have expanded state icon - is shouldn't! Icon depends on original, like in 4.a, but original state icon is hidden.

  6. Click state icon.

  7. Folder expands its content or closes, depending on original expanded state.
    a.1. If original was expanded before being copied there is no XHR is call to get copy children when copy is expands, even if original was closed when was copied.
    a.2. Else XHR is called to lazy load children.
    a.3 For me a case is correct and works as intended.
    b.'V' folder closes its empty content and 'V' changes to '>'. Go to 5.
    b.'>' folder calls XHR to get its content.


Fixing idea:
Bug 1.: toDict() line 2331 when expanded: undefined it is removed here from final dictionary. It shouldn't, as knowledge about this state (undefined) is needed to correctly rendering empty folders.
Bug 2.: Looks it is in function toDict() line 2347 there is two state check:

  1. if not empty array => dist.children = [] then dist.children += copy this.children
  2. else => do nothing
    in place where should be three state check:
  3. if not empty array => dist.children = [] then dist.children += copy this.children
  4. if empty array => dist.children = []
  5. else do nothing

Enchanting www 1.: On website with code source code www ::marker with number of lines needs some more space (or on mouse over tooltip with number), as four digit numbers are partially hidden.

@mar10
Copy link
Owner

mar10 commented Jan 29, 2020

I agree that toDict() should keep empty children: []. This is fixed now.

I think this also fixes your problem; can you confirm?

Some remarks though

  • toDict() does not know anything about copy/paste use cases. Its sole task is to serialize an existing node structure including all status flags.
  • The demos only implement one of many possible use-cases. Different behavior can be implemented by tweaking event handler, implementing the toDict()callback parameter, etc.
  • A folder may be displayed as collapsed or expanded, even if it is empty. Windows File Explorer may not allow it, but macOS Finder does.
  • From a user's perspective it is preferable if an empty node is not sent as 'lazy' in the first place, because a user has to click on the expander just to find out there is nothing. It would be better to send it as unexpandable end-node instead.

Anyway: thank you for taking the time to find and describe your findings!

@mar10 mar10 closed this as completed in 200062f Jan 29, 2020
@Roinoss
Copy link
Author

Roinoss commented Jan 30, 2020

Ad 1. And it shouldn't. My trouble with toDict is similar to this with children array, its removes all undefined keywords from node and some of them (like expanded state) should stay anyway, even with undefined status. It would be nice if it allow to pass array of strings with keywords.
---> Ok, it allows to add it in callback, but callback wasn't want works for me, because i based on example that is bugged.
For example page (sample-multi-ext.html):

console.dir(node);
            CLIPBOARD = {
              mode: data.cmd,
              data: node.toDict(function(n) {
                alert(n.key);
                delete n.key;
              }),
            };
console.dir(CLIPBOARD);

function(n) is never called, because it is taken as recursive in toDict: function(recursive, callback) { in example should be: data: node.toDict(false, function(n) {

Suggested change to example:

CLIPBOARD = {
              mode: data.cmd,
              data: node.toDict(false, function(dict, node) {
                //toDict keeps all not empty keywords from node (list of them: NODE_ATTRS in fancytree  line143) and all keywords in node.data object, if you don't need it:
                delete dict.key;
                //Undefined (empty) and not listed in (NODE_ATTRS keywords are omitted by toDict, if you need some, add them here:
                dict.expanded = node.expanded;
             }),
};

With working callback adding if(!!!dict.children) dict.children = []; fixed my issue, so yours change should also did it.

Ad. 3. I knew it, just marked it as possible place for error (and missed with that call).

Ad. 4. Yes, it can be an issue, but it is fixable in server side ajax script, just returned json for empty folder should have children: [] and not children: null or children: false. EDITED: Tip added to wiki.

@mar10
Copy link
Owner

mar10 commented Feb 1, 2020

The demo was missing the first parameter, but it should be true, since all keys must be re-created.

I fixed the demos, but still don't understand why you need to manually copy dict.expanded = node.expanded. If it was set to true, toDict already copies it. It it is undefined, it doesn't need to be copied?

@Roinoss
Copy link
Author

Roinoss commented Feb 2, 2020

Ad 1. Yes, it should be true, i forgot to change in post after finding that error it in my code.
Ad 2. node.expanded - It was mb with wrong understanding due to stacking bugs, although it may be needed in case when you need key name

For toDict nice and easy addition may be whitelist.
whitelist - {array of strings} replaces NODE_ATTRS
example of implementation:

toDict(recursive, whitelist, callback)
...
var list = ((is_array(whitelist)) ? whitelist : NODE_ATTRS);
if()
...
$.each(list, function(i, a) {
...
+some filter for nodes data

It will create easy way to extract custom data from node and its children, without need to cut lot data in callback function as it is now.

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

No branches or pull requests

2 participants