Skip to content

Refactored CoordinateSystemMapper and added tests #1699

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 14, 2025
Merged

Refactored CoordinateSystemMapper and added tests #1699

merged 1 commit into from
Jan 14, 2025

Conversation

@amartya4256
Copy link
Contributor

@amartya4256 amartya4256 commented Jan 2, 2025

No description provided.

@amartya4256 amartya4256 force-pushed the coordinate_system_tests branch from de2fb6d to ff77ecb Compare January 2, 2025 11:13
@github-actions
Copy link
Contributor

github-actions bot commented Jan 2, 2025 •
edited
Loading

Test Results

   494 files  +  8     494 suites  +8   9m 24s ⏱️ +16s
 4 324 tests + 15   4 306 ✅ +10   18 💤 + 5  0 ❌ ±0 
16 534 runs  +120  16 386 ✅ +80  148 💤 +40  0 ❌ ±0 

Results for commit 7403c74. ± Comparison against base commit d7be959.

♻️ This comment has been updated with latest results.

@amartya4256 amartya4256 force-pushed the coordinate_system_tests branch from ff77ecb to 25c81c3 Compare January 2, 2025 12:30
@amartya4256 amartya4256 mentioned this pull request Jan 2, 2025
Closed
@amartya4256 amartya4256 linked an issue Jan 2, 2025 that may be closed by this pull request
Tackle limitation of point coordinate system vi-eclipse/Eclipse-Platform#125
Closed
@HeikoKlare HeikoKlare mentioned this pull request Jan 3, 2025
Closed
HeikoKlare
HeikoKlare reviewed Jan 8, 2025
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Thank you for adding tests for the mapping calculations in Display and refactoring to the code accordingly.
In order to have the refactoring integrated as soon as possible (i.e., to have this PR finalized and merged), I have two questions @amartya4256:

  • Since the MultiZoomCoordinateSystemMapper is still under development (and as long as that's the case some tests will probably always be failing), can we deactivate those test for now?
  • Can we parameterize (or copy and adapt) the tests to also be applied to the SingleZoomCoordinateSystemMapper? Having regression tests for the existing HiDPI behavior and the mapping of coordinates in it would be great as well.

@amartya4256 amartya4256 linked an issue Jan 13, 2025 that may be closed by this pull request
Add automatic test for issue #1508 vi-eclipse/Eclipse-Platform#131
Closed
@amartya4256 amartya4256 force-pushed the coordinate_system_tests branch 3 times, most recently from 5a4099e to 8de8aa6 Compare January 13, 2025 13:28
@amartya4256 amartya4256 marked this pull request as ready for review January 13, 2025 13:29
HeikoKlare
HeikoKlare requested changes Jan 13, 2025
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Thank you for the adaptations. Would it be possible to reuse / parameterize the tests for SingleZoomCoordinateSystemMapper (see #1699 (review))?

I have some further comments and please add a copyright header to the three new classes.

@amartya4256 amartya4256 force-pushed the coordinate_system_tests branch 2 times, most recently from 13df60f to 83c3ee7 Compare January 13, 2025 17:25
@amartya4256
Copy link
Contributor Author

amartya4256 commented Jan 13, 2025

Thank you for adding tests for the mapping calculations in Display and refactoring to the code accordingly. In order to have the refactoring integrated as soon as possible (i.e., to have this PR finalized and merged), I have two questions @amartya4256:

  • Can we parameterize (or copy and adapt) the tests to also be applied to the SingleZoomCoordinateSystemMapper? Having regression tests for the existing HiDPI behavior and the mapping of coordinates in it would be great as well.

I can adjust the tests to support that. The monitors need to be created differently because right now the monitors are created with the offset values for multi monitor. But other than that, it should be fine except for some tests specific to Gap which will not be valid for it.

@HeikoKlare
Copy link
Contributor

HeikoKlare commented Jan 14, 2025

I can adjust the tests to support that. The monitors need to be created differently because right now the monitors are created with the offset values for multi monitor. But other than that, it should be fine except for some tests specific to Gap which will not be valid for it.

If that can be added really quickly, it would be a great win for regression testing. But if not, we should just leave it as is, since changes to the existing coordinate system mapper will be rather seldom (and we could also extend the test cases once changes need to be performed).

@amartya4256 amartya4256 force-pushed the coordinate_system_tests branch 3 times, most recently from 404e1af to ab28cd8 Compare January 14, 2025 14:12
@amartya4256 @HeikoKlare
Refactor out CoordinateSystemMapper from Display and add tests
7403c74 
This commit refactors the Display to separate single zoom and multi zoom
coordinate system mappers in separate classes. Additionally, it adds
some tests for validating the multi-zoom coordinate system mapper. A few
tests are disabled because of the current state of multi-zoom coordinate
system limitations.

Contributes to eclipse-platform#62 and eclipse-platform#127
@HeikoKlare HeikoKlare force-pushed the coordinate_system_tests branch from ab28cd8 to 7403c74 Compare January 14, 2025 14:38
HeikoKlare
HeikoKlare approved these changes Jan 14, 2025
View reviewed changes
Copy link
Contributor

@HeikoKlare HeikoKlare left a comment

Choose a reason for hiding this comment

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

Thank you for the latest update, including the extension of the tests to the single-zoom coordinate system mapper.

It looks fine to me now. I have just rebased on master and applied some formatting as indentations in the test class were broken.

@HeikoKlare HeikoKlare merged commit a6dd128 into eclipse-platform:master Jan 14, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HeikoKlare HeikoKlare HeikoKlare approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add automatic test for issue #1508 Tackle limitation of point coordinate system

2 participants

@amartya4256 @HeikoKlare