Skip to content

Conversation

@akoch-yatta
Copy link
Contributor

@akoch-yatta akoch-yatta commented Feb 26, 2025

This adapts Transform in the win32 implementation to create handles only on demand. If a non-handle specific operation like isIdentity() is called, a temporary handle will be created and disposed afterwards if no handle exists already.

It contains the branch used for #1858 and is a second refactoring on transform, there it must be merged after #1858

@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2025

Test Results

   506 files  ±0     506 suites  ±0   7m 55s ⏱️ -17s
 4 339 tests ±0   4 325 ✅ ±0   14 💤 ±0  0 ❌ ±0 
16 595 runs  ±0  16 486 ✅ ±0  109 💤 ±0  0 ❌ ±0 

Results for commit d1cdbe4. ± Comparison against base commit 0368a6b.

♻️ This comment has been updated with latest results.

@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform-dynamic branch from 9414c03 to 2ad8c82 Compare February 28, 2025 17:39
@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform-dynamic branch 3 times, most recently from fbd40ef to 84ccb02 Compare March 3, 2025 15:01
@akoch-yatta
Copy link
Contributor Author

akoch-yatta commented Mar 3, 2025

@HeikoKlare I needed to adapt the MultiplyOperation, because the passed matrix could already be disposed when executed.

see the new test -> I am surprised there was no Test class for Transform available yet.

@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform-dynamic branch 3 times, most recently from cf4300e to d2696f3 Compare March 6, 2025 12:42
temporaryHandle.destroy();
}
} else {
return function.apply(zoomToHandle.values().iterator().next());
Copy link
Contributor

@amartya4256 amartya4256 Mar 6, 2025

Choose a reason for hiding this comment

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

Isn't it risky to apply an operation on any one of the existing handles? Even tho its being used for get operations like isIdentity, it can be misused to modify one of the handles and that operation will not be registered with any other handle in that case.

Copy link
Contributor

@amartya4256 amartya4256 Mar 6, 2025

Choose a reason for hiding this comment

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

Anyway, I think the name is a bit misleading. It sounds like we are applying some operation (as in to modify) on the handle. A better suggestion could be "applyUsingAnyHandle"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't it risky to apply an operation on any one of the existing handles? Even tho its being used for get operations like isIdentity, it can be misused to modify one of the handles and that operation will not be registered with any other handle in that case.

Not in these cases. If that method would anything else then private, I would agree, but applying it on existing handles was done before in all cases. Just the "create a temporary handle if none is available"-case was added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anyway, I think the name is a bit misleading. It sounds like we are applying some operation (as in to modify) on the handle. A better suggestion could be "applyUsingAnyHandle"

Makes sense to me. I will rename it

Copy link
Contributor

Choose a reason for hiding this comment

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

I think its wise to adapt the same for all the resources then.

This commit adapts Transform in the win32 implementation to create handles
only on demand. If a non-handle specific operation like isIdentity() is
called, a temporary handle will be created and disposed afterwards if no
handle exists already.
@akoch-yatta akoch-yatta force-pushed the win32-refactor-transform-dynamic branch from d2696f3 to d1cdbe4 Compare March 6, 2025 15:31
@akoch-yatta
Copy link
Contributor Author

Rebased on master after #1858 was merged and addressed the comment from @amartya4256

@HeikoKlare HeikoKlare merged commit f03d8f0 into eclipse-platform:master Mar 6, 2025
14 checks passed
@HeikoKlare HeikoKlare deleted the win32-refactor-transform-dynamic branch March 6, 2025 15:53
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.

Refactor Transform to support creation handles on demand

3 participants