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

Update view.View.scrollTo method #1354

Closed
wants to merge 4 commits into from

Conversation

beru
Copy link

@beru beru commented Sep 14, 2024

This PR is opened to address the issue reported in #1352

@lutzroeder
Copy link
Owner

lutzroeder commented Sep 14, 2024

  1. Would it help to have a border zone for elements which are close to corners of the visible area? For example, shrink the test rectangle by 10% so elements close to the corners also get scrolled into the visible area?
  2. Should elements scroll to center or only move into the visible area?
  3. Long edges larger than the visible area cause scrolling when clicked. For example og_285 in gpt2-lm-head-bs-12.onnx.

@beru
Copy link
Author

beru commented Sep 14, 2024

  1. Would it help to have a border zone for elements which are close to corners of the visible area? For example, shrink the test rectangle by 10% so elements close to the corners also get scrolled into the visible area?

I'm not sure about that. Shall I create a commit to check UX?

  1. Should elements scroll to center or only move into the visible area?

Centering a clicked node is a bit too much I think. I suppose moving it inside a safe-area will suffice. The size of the safe area can be around 80% or 90%.

If any of the attemps won't satisfy @fdwr, reverting c5868fe would be needed.

  1. Long edges larger than the visible area cause scrolling when clicked. For example og_285 in gpt2-lm-head-bs-12.onnx.

Firstly, I'm not really sure if scrolling is necessary when edges are selected. Does it really help? In some cases when lines are very long, clicking them triggers a long scroll and connected nodes that were previously shown tend to go out of the visible area due to the scrolling.

@beru beru force-pushed the neo_automatic_scrolling branch 2 times, most recently from 1275e1f to f2297c1 Compare September 15, 2024 09:23
@beru
Copy link
Author

beru commented Sep 15, 2024

@lutzroeder @fdwr done updating the code so can you please check new auto scroll behavior is OK?

@beru beru changed the title Scroll only when a selected node isn't inside of container's client area Update view.View.scrollTo method Sep 15, 2024
@lutzroeder
Copy link
Owner

  1. Open candy.onnx
  2. Open Find sidebar
  3. Scroll to bottom and select scale_B
  4. Scroll to bottom and select scalePreprocessor

Actual: does not scroll to new location.

@beru
Copy link
Author

beru commented Sep 15, 2024

@lutzroeder

  1. Scroll to bottom and select scalePreprocessor
    Actual: does not scroll to new location.

Fixed the bug with 19d547f by using Number.NEGATIVE_INFINITY instead of 0. Currently, the change of this PR disregards your recent commit 7b993c6 so a further adjustment might be needed...

This is an off-topic but I've noticed that outputImage connection of candy.onnx isn't shown in the FIND sidebar list albeit the fact that inputImage connection is shown.

@lutzroeder
Copy link
Owner

Regressed #1356

Open alexnet.zip.pth.

Actual: initial positioning shows center of node
Expected: initial positioning should show top of node

@beru
Copy link
Author

beru commented Sep 16, 2024

Regressed #1356

Honestly, it's difficult to change auto scroll behavior while keeping #1356.

@beru beru marked this pull request as draft September 16, 2024 00:58
@beru beru marked this pull request as ready for review September 16, 2024 13:48
lutzroeder added a commit that referenced this pull request Sep 17, 2024
@lutzroeder lutzroeder closed this Sep 17, 2024
@beru beru deleted the neo_automatic_scrolling branch September 17, 2024 03:17
lutzroeder added a commit that referenced this pull request Sep 17, 2024
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.

2 participants