Skip to content

Improve space drag #277

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

Closed
wants to merge 2 commits into from
Closed

Improve space drag #277

wants to merge 2 commits into from

Conversation

yinanazhou
Copy link
Collaborator

  • Refactored space drag logic to use D3 for event handling
  • Updated drag pattern for improved maintainability
  • Fixed: drag event listener now attaches to the.view element

closes: #270

- Used D3 for smooth dragging
- Renamed functions to use the same pattern as option drag
- Simplied dragging code

refs: #270
@yinanazhou yinanazhou requested a review from yrammos April 15, 2025 21:08
@yrammos
Copy link
Member

yrammos commented Apr 15, 2025

@yinanazhou thanks—this is a noticeable performance improvement. Latency is still prohibitively high in the Brandenburg concerto example (sent via Mattermost).

I am wondering, could we rethink div and SVG sizing so that we rely exclusively on CSS for scrolling (overflow: scroll etc.)?

Adding overflow: scroll to the .view class generates a scrollbar on the Y axis (which scrolls lightning-fast). Curiously, it does not generate a horizontal scrollbar. I wonder why… This could be the most efficient solution.

@yinanazhou
Copy link
Collaborator Author

Hi @yrammos, thank you for the feedback. The slow performance with this file may not be directly related to the changes in this PR. It appears that the file triggers a large number of warnings in Verovio, and other actions remain slow even on the master branch. This suggests that the performance issues are more broadly related to the overall state of the app.

As I mentioned earlier, the codebase is currently quite complex, which makes it difficult to have full control over the behavior in certain cases.

Regarding the scrollbar, I opted not to use it because navigating between multiple layers would require frequent scrolling, which could hinder usability.

@yrammos
Copy link
Member

yrammos commented Apr 15, 2025

@yinanazhou unlike you, I cannot reproduce the issue on the master branch. I opened the file at https://dcmlab.github.io/reductive_analysis_app/no and scrolling appears very smooth.

Would you please make a rough prototype with scrollbars (either as a branch of this PR, or as a new PR altogether)? No need to polish the edges—just a demo of the concept, so that we can compare the general experiences. Thanks!

@yinanazhou
Copy link
Collaborator Author

@yrammos, yes, scrolling alone appears smooth because it doesn't interact with the app. However, when you interact with the app—for example, selecting notes or creating relations. These actions are significantly slow.

@yrammos
Copy link
Member

yrammos commented Apr 16, 2025

@yinanazhou indeed, operations that require a search within the XML tree (e.g. matching a clicked-on notehead with a <note> object in the MEI file) become slower with larger XML trees. This is a real issue, but a different one. I will actually open a ticket for it—thanks.

@yinanazhou
Copy link
Collaborator Author

Not only that, dragging a fly-out relation window is also not possible. Please let me know how you would like to proceed with this PR.

@yrammos
Copy link
Member

yrammos commented Apr 16, 2025

Before deciding how to proceed, please make a rough prototype with standard browser scrollbars (either as a branch of this PR, or as a new PR altogether). As I wrote above, no need to polish the prototype—just a demo of the concept, so that we can compare the two experiences. Thanks.

@yrammos
Copy link
Member

yrammos commented Apr 17, 2025

UPDATE: I cannot space-drag beyond the first few measures of mozart_long.mei (see Mattermost discussion). The is not just a latency issue: space-drag just stops working past a certain point in the score. The same score scrolls without problems in the master branch.

@yinanazhou
Copy link
Collaborator Author

@yrammos are you on drag-perf? I cannot reproduce it. I have no problem dragging to the end.

image

@yrammos
Copy link
Member

yrammos commented Apr 17, 2025

Please disregard, @yinanazhou, my working tree was dirty, so I hadn’t successfully checked out the right branch. Let’s proceed as suggested in this comment. Thanks!

@yinanazhou
Copy link
Collaborator Author

Please check this branch about scrolling navigation.

@yrammos
Copy link
Member

yrammos commented Apr 18, 2025

@yinanazhou this feels immensely better and more comfortable to me in actual use of the app—and will be even better when combined with #268.

Some larger scores are rendered either not at all, or invisibly small (examples: 25 Beethoven—Piano Sonata Op 2 No 1 II_analysis.mei and 17 Mozart—Piano Sonata K 309 I_analysis, sent via Mattermost).

yinanazhou added a commit that referenced this pull request Apr 19, 2025
- Ensured that `adjustSvgDimension` is called after drawing complete
- Added 100px offset to y dimension

refs: #277 (comment)
@yinanazhou yinanazhou closed this Apr 19, 2025
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