Skip to content

Add tests for auto-panning - #2597

Closed
rahat2134 wants to merge 11 commits into
GraphiteEditor:masterfrom
rahat2134:AutoPanningSelectTests
Closed

Add tests for auto-panning#2597
rahat2134 wants to merge 11 commits into
GraphiteEditor:masterfrom
rahat2134:AutoPanningSelectTests

Conversation

@rahat2134

@rahat2134 rahat2134 commented Apr 19, 2025

Copy link
Copy Markdown
Contributor

This pull request introduces automated tests for the auto-panning functionality.

Currently implemented:

  • Select Tool
  • Artboard Tool

Closes #2505

@Keavon Keavon changed the title Add Auto Pann tool tests within Select Tool Add auto-panning tests within the Select tool Apr 19, 2025
@rahat2134

Copy link
Copy Markdown
Contributor Author

@0HyperCube PTAL!

@rahat2134
rahat2134 marked this pull request as draft April 19, 2025 09:47
@rahat2134 rahat2134 changed the title Add auto-panning tests within the Select tool Add auto-panning tests Apr 19, 2025
@rahat2134

Copy link
Copy Markdown
Contributor Author

@0HyperCube Can you review the select tool part for this PR. I will add other tests soon!
In this way it will be easy for you and me both!

Thanks :)

@Keavon Keavon changed the title Add auto-panning tests Add tests for auto-panning Apr 19, 2025
@Keavon Keavon added the Testing Unit and integration tests label Apr 19, 2025
@rahat2134

Copy link
Copy Markdown
Contributor Author

@0HyperCube Can you review the select tool part for this PR. I will add other tests soon! In this way it will be easy for you and me both!

Thanks :)

@0HyperCube PTAL!

@0HyperCube

Copy link
Copy Markdown
Contributor

Please mark as ready to review.

@rahat2134

Copy link
Copy Markdown
Contributor Author

Please mark as ready to review.

I am marking this as ready to review!

@rahat2134
rahat2134 marked this pull request as ready for review April 24, 2025 20:11
@rahat2134

Copy link
Copy Markdown
Contributor Author

@0HyperCube !!!

@0HyperCube 0HyperCube left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution so far.

However the code is overly verbose and the checks are rather dubious.

Comment on lines +147 to +171
// Moving cursor outside viewport to trigger auto-panning
editor.move_mouse(2000., 100., ModifierKeys::empty(), MouseKeys::LEFT).await;

let pointer_keys = SelectToolPointerKeys {
axis_align: Key::Shift,
snap_angle: Key::Control,
center: Key::Alt,
duplicate: Key::Alt,
};

// Sending multiple outside viewport events to simulate continuous auto-panning
for _ in 0..5 {
editor.handle_message(SelectToolMessage::PointerOutsideViewport(pointer_keys.clone())).await;
}

editor
.mouseup(
EditorMouseState {
editor_position: DVec2::new(2000., 100.),
mouse_keys: MouseKeys::empty(),
scroll_delta: ScrollDelta::default(),
},
ModifierKeys::empty(),
)
.await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make a function instead of duplicating so much code.

let scale_ratio = final_scale / initial_scale;

assert!(
scale_ratio > 1.1 || scale_ratio < 0.9,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can you not determine what happens to the scale? Surely the scale should increase.


// Comparing transform matrices to detect scale changes
let initial_scale = initial_transform.matrix2.determinant().sqrt();
let final_scale = final_transform.matrix2.determinant().sqrt();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have assumed that the scale is uniform. Please justify this in a comment.

Comment on lines +260 to +261
mouse_keys: MouseKeys::empty(),
scroll_delta: ScrollDelta::default(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can just to ..Default::default()

assert!(!artboards.is_empty(), "Artboard should have been created");
assert!(
artboards[0].dimensions.x > 200,
"Artboard should have significant width due to auto-panning: {}",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What led you to decide that this was «significant». Please justify in a comment.

assert!(!artboards.is_empty(), "Artboard should have been created");
let initial_location = artboards[0].location;

editor.select_tool(ToolType::Artboard).await;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Artboard is already selected.


// This test is marked as ignored due to inconsistent behavior when run as part of the test suite.
// It passes when run individually but frequently fails when run with other tests.
// The issue appears to be related to difficulty consistently capturing resize handles with mouse events

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please elaborate further.

@Keavon
Keavon marked this pull request as draft April 30, 2025 05:10
@Keavon

Keavon commented May 30, 2025

Copy link
Copy Markdown
Member

Hi, are you interested in continuing this or should I close it as incomplete? Thanks!

@rahat2134 rahat2134 closed this Jun 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Testing Unit and integration tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Testing auto panning

3 participants