Skip to content

Better API for handling MonitorAware Coordinates #2186

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

amartya4256
Copy link
Contributor

This PR contributes to providing better interfaces and APIs for handling MonitorAware Rectangles and Points with Abstraction using prototype pattern.

All the Monitor Aware classes implement MonitorAware. Also th coordinate classes liek Rectangle and Point implement an interface copy. This should be introduced as a convention to use copy or copyWith, when a consumer wants to create a new object from the existing one. If there's any offsetting needed, the method copyWith can be used, making sure we copy all the abstract fields of the underlying Child class (if applicable) to the new object.

Also this PR introduces a new method Rectangle.of(Point, int, int) to make it easy to create rectangles using points instead of calling the Rectangle Constructor.

contributes to
#62 and #128

Copy link
Contributor

github-actions bot commented May 27, 2025

Test Results

104 files   -    441  104 suites   - 441   5s ⏱️ - 27m 9s
 66 tests  -  4 333   66 ✅  -  4 315  0 💤  -  18  0 ❌ ±0 
213 runs   - 16 510  213 ✅  - 16 369  0 💤  - 141  0 ❌ ±0 

Results for commit 1c2b80a. ± Comparison against base commit e4890da.

This pull request removes 4333 tests.
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_dollarSign
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_emptyString
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letterA
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicASCII_letters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16LE_null
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_AsciiLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_Asciiletter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_LotsOfLetters
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letter
AllGTKTests org.eclipse.swt.tests.gtk.Test_GtkConverter ‑ test_HeuristicUTF16_letters
…

♻️ This comment has been updated with latest results.

Copy link
Contributor

@fedejeanne fedejeanne left a comment

Choose a reason for hiding this comment

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

I'm looking at this PR together with its first usage (eclipse-platform/eclipse.platform.ui#3005) and I have some questions (it might be that there are more consumers that haven't been adapted yet so I might be missing "the big picture", feel free to comment on that :-) ):

  • Do we really need the interface MonitorAware? How is it helpful?
  • I would remove the new method copyWith(...) if it's only necessary to copy and modify the receiver since that is something that the consumer can do on its own without "special handling" (no monitor-awareness, zooms, etc seem to play a role here)
  • I see that Copyable is merely a "better named version of Cloneable" and it was probably added so that the methods copy() and copyWith(...) are associated with each other (?) . If that's the case, I think getting rid of copyWith(...) and replacing Copyable with Clonable is really a valid option.

/**
* @since 3.130
*/
public Rectangle copyWith(int dx, int dy, int dWidth, int dHeight) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I now see that copyWith does not do any scaling nor does it do any special handling regarding monitor-awareness. This method is not really necessary, the consumer could simply use copy and then modify the coordinates and/or size on its own.

I mean doing this:

Rectangle r1 = ...
Rectangle r2 = r1.copy();
r2.x += dx;
r2.y += dy;
r2.width += dWidth;
r2.height += dHeight;

... instead of this:

Rectangle r1 = ...
Rectangle r2 = r1.copyWith(dx, dy, dWidth. dHeight);

Copy link
Contributor Author

@amartya4256 amartya4256 May 30, 2025

Choose a reason for hiding this comment

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

The upper approach is not clean and the user might wanna rather call the constructor instead and create a new instance with the values. That's why having such API makes sense. Also had a talk with @akoch-yatta about it.

Also the API implementation can further be improved to update the monitor with respect to the offset as well (In case, the offset changes the monitor). Although that will be a separate issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would be followed in the next PR

@amartya4256 amartya4256 force-pushed the amartya4256/montior_aware_coordinates branch from c4be5d7 to dcb6310 Compare May 30, 2025 11:08
@amartya4256
Copy link
Contributor Author

  • I see that Copyable is merely a "better named version of Cloneable" and it was probably added so that the methods copy() and copyWith(...) are associated with each other (?) . If that's the case, I think getting rid of copyWith(...) and replacing Copyable with Clonable is really a valid option.

Cloneable has clone() method as protected. If it is fine to override it to public without breaking the API conventions of SWT, I can use that for sure.

@amartya4256 amartya4256 force-pushed the amartya4256/montior_aware_coordinates branch 2 times, most recently from e113616 to c5f86dd Compare May 30, 2025 11:36
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.

Overall, the change looks reasonable to me. Having proper clone functionality for those data classes is fine. I am not sure whether we can have a more expressive name for Rectangle.of.

Cloneable has clone() method as protected. If it is fine to override it to public without breaking the API conventions of SWT, I can use that for sure.

Note that Cloneable has not clone() method at all. clone() is defined by Object in Java and needs to be overwritten and made public in classes that implement the Clonable interface. I don't see that this can conflict with any conventions or contracts in SWT. Rectangle cannot be extended, so there is not chance that there is a conflicting overwrite of the method.

@laeubi
Copy link
Contributor

laeubi commented Jun 2, 2025

Just an idea:

Do we really need an own class here? Can't we add a (package protected?) field to the regular Rectangle and mark it as not for public use like we do with the handles in Control?

@amartya4256 amartya4256 force-pushed the amartya4256/montior_aware_coordinates branch 2 times, most recently from 8517b11 to 60dbf84 Compare June 2, 2025 11:54
This commit contributes to providing better interfaces and APIs for
handling MontiorAware Rectangles with Abstraction.

contributes to
eclipse-platform#62 and
eclipse-platform#128
@amartya4256 amartya4256 force-pushed the amartya4256/montior_aware_coordinates branch from 60dbf84 to 1c2b80a Compare June 2, 2025 11:55
@amartya4256 amartya4256 marked this pull request as ready for review June 2, 2025 13:03
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.

Drag and Drop Region moves with a different scale as compared to the cursor between 2 monitors
4 participants