Hide/show child nodes of a branch #325

sorenjohanson · 2024-07-05T09:29:49Z

closes #105

sorenjohanson · 2024-07-10T14:37:04Z

Do we want this to be a client-side only feature or server-side as well? Right now, there's still a bug left that the hidden state is not updated across all clients immediately (though I'd assume it calls map.draw.update() whenever something changes), but nodes are correctly hidden if the client updates the page.

Regarding an icon: Somehow very tricky to implement correctly and synchronize across all clients simultaneously.

JannikStreek · 2024-07-15T07:18:13Z

Do we want this to be a client-side only feature or server-side as well? Right now, there's still a bug left that the hidden state is not updated across all clients immediately (though I'd assume it calls map.draw.update() whenever something changes), but nodes are correctly hidden if the client updates the page.

Regarding an icon: Somehow very tricky to implement correctly and synchronize across all clients simultaneously.

good question, I would say as it only affects visibility it should only be on the client side? otherwise people might confuse this for that a user deleted a branch.

sorenjohanson · 2024-07-15T07:48:28Z

good question, I would say as it only affects visibility it should only be on the client side? otherwise people might confuse this for that a user deleted a branch.

Does it make sense to keep the changes to the data model then? Since those changes are synced server-side, inevitably leading new clients to consider selected nodes hidden as well.

JannikStreek · 2024-07-15T12:28:35Z

good question, I would say as it only affects visibility it should only be on the client side? otherwise people might confuse this for that a user deleted a branch.

Does it make sense to keep the changes to the data model then? Since those changes are synced server-side, inevitably leading new clients to consider selected nodes hidden as well.

Either we sync it across clients so everybody is watching the same and we have it in our data model - or we just store that property on the client side without adding it to the data model. I think I would prefer the latter one.

sorenjohanson · 2024-07-19T08:46:36Z

Works up to 99% percent, but there's still a weird and oddly specific bug where if two clients hide the same branches at the same time (so branch A and B), client A will unhide branch A when clicking on branch B.

Also missing: Changing the eye icon in the toolbar based on whether or not any child nodes are hidden.

JannikStreek · 2024-07-19T11:18:26Z

Works up to 99% percent, but there's still a weird and oddly specific bug where if two clients hide the same branches at the same time (so branch A and B), client A will unhide branch A when clicking on branch B.

Also missing: Changing the eye icon in the toolbar based on whether or not any child nodes are hidden.

i dont really understand this problem if hiding/showing is a local operation 🤔

sorenjohanson · 2024-07-19T11:23:00Z

Works up to 99% percent, but there's still a weird and oddly specific bug where if two clients hide the same branches at the same time (so branch A and B), client A will unhide branch A when clicking on branch B.
Also missing: Changing the eye icon in the toolbar based on whether or not any child nodes are hidden.

i dont really understand this problem if hiding/showing is a local operation 🤔

It's not really consistently reproducible, but it's there - I'm trying to figure out if it maybe has something to do with how nodes selected by other users are rendered (maybe it affects the selectedNode variable in nodes.ts?). It's purely a client side issue though, most probably in how D3 renders the nodes, especially when other clients are involved.

sorenjohanson · 2024-07-24T10:34:27Z

Think I found the issue: The NodeUpdated event gets called for Client B whenever Client A hides certain nodes. However, if Client B has already hidden the same set of nodes, the "hidden" values gets overwritten with null, since UtilsService.get(newNode, propertyPath) doesn't return a proper value. That means Client B needs to click twice to unhide already hidden nodes, since it's set from null to true to false.

This can also lead to very funny issues when Client A hides a child, Client B hides every node except the root node and Client A then decides to also hide the root node, because this then hides every node except the originally hidden child.

JannikStreek · 2024-07-24T10:41:41Z

Think I found the issue: The NodeUpdated event gets called for Client B whenever Client A hides certain nodes. However, if Client B has already hidden the same set of nodes, the "hidden" values gets overwritten with null, since UtilsService.get(newNode, propertyPath) doesn't return a proper value. That means Client B needs to click twice to unhide already hidden nodes, since it's set from null to true to false.

This can also lead to very funny issues when Client A hides a child, Client B hides every node except the root node and Client A then decides to also hide the root node, because this then hides every node except the originally hidden child.

well why is an update event triggered in the first place? as the "hidden/show" property should be a local value, we shouldn't send a web socket update. An mmp event is fine as it's the local map that was updated. That should, however, not trigger a web socket update to other parties?

On the other hand, an update to not visible nodes should influence the local data model but not the drawing area.

sorenjohanson · 2024-07-24T10:46:54Z

well why is an update event triggered in the first place? as the "hidden/show" property should be a local value, we shouldn't send a web socket update. An mmp event is fine as it's the local map that was updated. That should, however, not trigger a web socket update to other parties?

On the other hand, an update to not visible nodes should influence the local data model but not the drawing area.

notifyWithEvent is true by default, forgot to change that to false.

Good point about the local data model - I'll have to test that. Hiding nodes is much more stable now, though I do have odd one-off cases where the parent nodes get hidden whilst children are still visible. I'll set the PR to open for review soon and then we can both test and iron out any kinks.

sorenjohanson · 2024-07-24T10:54:38Z

Updating the data model works fine even when nodes are hidden. One thing that's still missing is some visible way to tell nodes are hidden, either by updating the icon in the toolbar or by attaching an SVG icon inside the drawing area to nodes.

JannikStreek · 2024-07-25T11:53:55Z

Updating the data model works fine even when nodes are hidden. One thing that's still missing is some visible way to tell nodes are hidden, either by updating the icon in the toolbar or by attaching an SVG icon inside the drawing area to nodes.

a small svg icon somewhere in the node sounds good 👍 the toolbar should also change (thats already done for add/remove image). But I think an additional item inside the node makes also sense as otherwise one might forget it.

teammapper-backend/src/map/types.ts

…ry attribute in IMmpClientNode

teammapper-frontend/mmp/src/map/handlers/draw.ts

teammapper-frontend/mmp/src/map/handlers/nodes.ts

JannikStreek · 2024-07-26T10:40:38Z

teammapper-frontend/mmp/src/map/handlers/draw.ts

        let tapedTwice = false

+        dom.nodes.each((node: Node) => {


wouldnt it be easier to set the hidden field directly when the button is pressed for all nodes, and not inside this render method?

Not sure what you mean by this? There's practically no difference whether you actually set a field on the node itself or do it like this, as you'd still have to call both methods (setHiddenChildrenIcon and removeHiddenChildrenIcon) separately whenever you add/remove the icon to make it work reliably, at least in my testing.

You also can't avoid calling dom.nodes.each as outer.each (which is based on the enter method of D3.js) only seems to include nodes that were changed, ie those that were hidden, whereas we explicitly want the node that wasn't hidden.

JannikStreek · 2024-07-29T14:23:45Z

Looks good. Two bugs that need more checking:

Undo/Redo is bugged - I got very strange results with it with nodes flying around in space without being attached. If possible I wouldn't consider visibility changes as not un/redoable. But thats just on first thought.
Hiding nodes and then exporting the image looks like this:

Not sure what the preferred behavior for this feature is on exports. Either completely hide it (like it is now) or just show it - but without this visibility note thingy. The same also happens for attached links, which just show "link" - but thats a separate issue.

sorenjohanson · 2024-07-29T14:29:38Z

Not sure what the preferred behavior for this feature is on exports. Either completely hide it (like it is now) or just show it - but without this visibility note thingy. The same also happens for attached links, which just show "link" - but thats a separate issue.

Re this: I think keeping notes invisible when exporting is the preferred option. However, it seems like it doesn't work with Material Icons, so I'm guessing removing the link text ("visibility_off") is the better option.

sorenjohanson · 2024-07-29T14:29:47Z

I'll take a look at undo/redo.

sorenjohanson · 2024-07-30T07:28:30Z

I don't think there's much we can do about undo/redo without a rework. The issue is that if we hide two nodes, that's two steps in the snapshot, with the farthest node being the first step in the snapshot to be undone. Similar weird behaviour can be observed when creating a node with text and undoing it: Creating the node is one step, with the text being a separate step, so it takes two presses of undo to delete a single node.

sorenjohanson · 2024-07-30T07:44:36Z

Not sure what the preferred behavior for this feature is on exports. Either completely hide it (like it is now) or just show it - but without this visibility note thingy. The same also happens for attached links, which just show "link" - but thats a separate issue.

I've removed all text on export now, so neither "link" nor "visibility_off" should appear on export.

…logic

…r test

JannikStreek · 2024-08-10T12:10:24Z

There are still bugs: If another user adds nodes to a hidden parent node, it gets weird:

…ties: hidden when parent is hidden

…n state when map is updated

JannikStreek · 2024-08-13T09:37:29Z

teammapper-frontend/mmp/src/map/handlers/nodes.ts

                    if (!x.parent.hidden && x.hidden) {
                        this.updateNode('hidden', false, false, false, false, x.id)
                    }
                })
            }

+            // Lengthy but definitive check to see if we have any hidden nodes after toggling
+            if (this.map.nodes.nodeChildren(this.selectedNode.id)?.filter(x => x.hidden).length > 0) {
+                this.selectedNode.hasHiddenChildNodes = true


can't this be set directly, when the button is clicked (hide children)? Do we have to calculate this dynamically?

As I see it, this property makes sense. So we have two new fields:

hidden (on the descendants)

hiddenBranches (on the parent)

So I would set them both upon button press?

It was a computed property before, but there's an issue with the history if we make it computed: Suppose that parent node A has exactly one child node B which is hidden. If you undo/redo the creation of the child node B, there's no way to tell it's supposed to be hidden, as the previous map (client side) doesn't include child node B, only the snapshot we receive from the server does (which doesn't have the client side "hidden" attribute). That's why we have hasHiddenChildNodes, which is set on parent node A.

So now we have two attributes:

hasHiddenChildNodes, which is always set on the parent node of any hidden children

hidden, which is set on the actual nodes that are hidden

The former only affects history and rendering of the hidden eye icon, the latter affects whether the node is actually visible or not. Both of them are set when calling toggleBranchVisibility().

good point, maybe you can add this reason as a comment? that there is not only the button that triggers it and thats the reason you have to check here.

Good idea, I've added some comments.

JannikStreek · 2024-08-14T05:39:05Z

teammapper-frontend/mmp/src/map/handlers/draw.ts

-                nodes: this.map.dom.g.selectAll('.' + this.map.id + '_node').data(nodes),
-                branches: this.map.dom.g.selectAll('.' + this.map.id + '_branch').data(nodes.slice(1))
-            }
+        let nodes = this.map.nodes.getNodes()


why let here? do you need to modify this ?

Good catch, must've been a mistake from when I separated out nodes and dom variable creation.

JannikStreek · 2024-08-14T05:42:14Z

teammapper-frontend/mmp/src/map/handlers/draw.ts

@@ -265,29 +282,59 @@ export default class Draw {
     */
    public setLink(node: Node) {
        let domLink = node.getLinkDOM()
+        const domText = document.createElementNS('http://www.w3.org/2000/svg', 'text')


why was this change needed? On first glance, if the link Dom is already present, it makes no sense to draw on top of it?

Was testing to see if it made a difference in rendering, I should be able to revert the changes to the method since the issue was with the querySelector and not the setLink() method.

Changes are reverted, works as before now.

JannikStreek · 2024-08-14T05:42:59Z

teammapper-frontend/mmp/src/map/handlers/draw.ts

+     * Check if given node has hidden children and render the eye icon if so
+     * @param {Node} node
+     */
+    private checkForHiddenChildren(node: Node) {


this method does not only check, but actually hides or even removes Dom icons. That should be reflected in the name, as its otherwise confusing

JannikStreek · 2024-08-14T05:44:24Z

teammapper-frontend/mmp/src/map/handlers/history.ts

        } else if (this.checkSnapshotStructure(snapshot)) {
            const previousData = this.map.export.asJSON()

+            // Find all nodes where we've set hasHiddenChildNodes in the previous map


maybe apply refactoring: extract method, as this is a lot of logic only dedicating to the hiding of nodes.

JannikStreek · 2024-08-14T05:46:36Z

teammapper-frontend/mmp/src/map/handlers/nodes.ts

+        }
+
+        if (node.hidden !== hidden) {
+            node.hidden = hidden;


please check to return a consistent return type. here, you either return false or you assign a variable and return undefined (which is also false).

All the other methods also either assign a variable or return false, so I was just orienting myself on those. Maybe a general refactoring idea?

imho only because the other methods follow a bad practice, this doesn't mean we have to continue this way. The return value is useless if it's always false? But yes, the other methods could be refactored in a separate refactoring.

Yeah, I've refactored this one to return undefined, I'll open an issue for the other ones.

sorenjohanson added 4 commits July 5, 2024 09:29

add: hidden field to mmpnode

8f8e65a

add: functionality to show/hide nodes

bdfcfc5

add: tooltip and translations

c867398

changed: use hasHiddenChildren instead

12ff66d

sorenjohanson added 2 commits July 18, 2024 08:43

Merge branch 'main' into 105-hide-show-child-nodes-of-a-branch

7f5f35b

changed: use client-side hidden variable, add key function to dom nodes

204583d

fix: do not send websocket update when hiding nodes

068e26a

sorenjohanson marked this pull request as ready for review July 24, 2024 10:49

fix: only hide direct children, not all descendants

32d1e1e

JannikStreek reviewed Jul 25, 2024

View reviewed changes

teammapper-backend/src/map/types.ts Outdated Show resolved Hide resolved

sorenjohanson added 5 commits July 25, 2024 12:19

changed: toolbar icon depending on node visibility, removed unnecessa…

b8dfccf

…ry attribute in IMmpClientNode

changed: remove unnecessary UserNodeProperties attribute

fc9fe67

fix: hide all descendants properly based on their parent status

68141bc

add: hidden eye icon when updating nodes

84a6c25

add: disable hide node button if no node/root node is selected

ea2dd7d

JannikStreek reviewed Jul 26, 2024

View reviewed changes

teammapper-frontend/mmp/src/map/handlers/draw.ts Outdated Show resolved Hide resolved

JannikStreek reviewed Jul 26, 2024

View reviewed changes

teammapper-frontend/mmp/src/map/handlers/nodes.ts Outdated Show resolved Hide resolved

JannikStreek reviewed Jul 26, 2024

View reviewed changes

removed: unnecessary class

ea706ff

fix: remove text like visibility_off and link from exported image

4f197de

sorenjohanson added 4 commits August 9, 2024 09:54

Merge branch 'main' into 105-hide-show-child-nodes-of-a-branch

9ccde3b

add: comments to explain slightly more complicated branch visibility …

36b5319

…logic

fix: lint

4f2288c

changed: history should not include visibility changes, fixed: toolba…

0d010d5

…r test

sorenjohanson added 4 commits August 10, 2024 15:43

changed: visibility hidden instead of filtering out nodes, set proper…

04e5176

…ties: hidden when parent is hidden

fix: eye icon not rendering during undo/redo

bef9113

added: method to recursively hide child nodes and retain proper hidde…

250a84c

…n state when map is updated

fix: lint

587016f

JannikStreek reviewed Aug 13, 2024

View reviewed changes

sorenjohanson added 2 commits August 13, 2024 11:53

changed: small changes to variable names

5beaf21

fix: link icon disappearing

d0be27c

JannikStreek reviewed Aug 14, 2024

View reviewed changes

sorenjohanson mentioned this pull request Aug 14, 2024

Link icon disappears when reloading the page #380

Closed

sorenjohanson added 4 commits August 14, 2024 08:37

fix: refactoring

85454a7

fix: refactor updateNodeHidden

ee483a7

changed: add comments

63f58ec

Merge branch 'main' into 105-hide-show-child-nodes-of-a-branch

de316ce

JannikStreek merged commit c1b3dc2 into b310-digital:main Aug 14, 2024
5 checks passed

Hide/show child nodes of a branch #325

Hide/show child nodes of a branch #325

Conversation

sorenjohanson commented Jul 5, 2024

sorenjohanson commented Jul 10, 2024

JannikStreek commented Jul 15, 2024

sorenjohanson commented Jul 15, 2024

JannikStreek commented Jul 15, 2024

sorenjohanson commented Jul 19, 2024

JannikStreek commented Jul 19, 2024

sorenjohanson commented Jul 19, 2024

sorenjohanson commented Jul 24, 2024

JannikStreek commented Jul 24, 2024

sorenjohanson commented Jul 24, 2024 • edited Loading

sorenjohanson commented Jul 24, 2024

JannikStreek commented Jul 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JannikStreek commented Jul 29, 2024 • edited Loading

sorenjohanson commented Jul 29, 2024

sorenjohanson commented Jul 29, 2024

sorenjohanson commented Jul 30, 2024

sorenjohanson commented Jul 30, 2024

JannikStreek commented Aug 10, 2024

Choose a reason for hiding this comment

JannikStreek Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sorenjohanson commented Jul 24, 2024 •

edited

Loading

JannikStreek commented Jul 29, 2024 •

edited

Loading

JannikStreek Aug 13, 2024 •

edited

Loading