Skip to content
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

Refactor stream handling for Cocoa and GTK in ImageLoader class #1760

Merged

Conversation

Michael5601
Copy link
Contributor

This PR refactors the stream handling by using try-with-resource for opening streams in the ImageLoader class for Cocoa and GTK.

@Michael5601
Copy link
Contributor Author

@HannesWell can you please review and merge this PR?

Copy link
Contributor

github-actions bot commented Jan 24, 2025

Test Results

   494 files  ±0     494 suites  ±0   9m 0s ⏱️ - 1m 25s
 4 331 tests ±0   4 318 ✅ ±0   13 💤 ±0  0 ❌ ±0 
16 572 runs  ±0  16 464 ✅ ±0  108 💤 ±0  0 ❌ ±0 

Results for commit 6776734. ± Comparison against base commit 5c2611d.

♻️ This comment has been updated with latest results.

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.

In general, using try-with-resources for these streams makes sense and the change looks sound.

But this also changes the behavior of the methods, as now an SWTError (ERROR_IO) will be thrown in case an IOException happens when closing the stream while currently that exception is swallowed.
I am still in favor of doing it, as it has already been changed that in Windows more than a year ago and (1) no one complained so far and (2) this change will make the implementations consistent across the operating systems again.

We might update the Javadoc as well to clarify that an error will also be thrown when closing the stream fails.

@laeubi
Copy link
Contributor

laeubi commented Jan 24, 2025

If closing fails we are probably all lost anyways...

@Michael5601 Michael5601 force-pushed the ImageLoader-Refactoring branch from f1d84dd to 995203f Compare January 24, 2025 11:19
@Michael5601
Copy link
Contributor Author

We might update the Javadoc as well to clarify that an error will also be thrown when closing the stream fails.

I agree and updated the javadoc accordingly.

@laeubi
Copy link
Contributor

laeubi commented Jan 24, 2025

I think we should not complicate this.... reading includes closing, and that a stream is used is and implementation detail, so simply say it do so when I/O error occurs when reading the file is enough.

@HeikoKlare
Copy link
Contributor

I think we should not complicate this.... reading includes closing, and that a stream is used is and implementation detail, so simply say it do so when I/O error occurs when reading the file is enough.

Agreed 👍

@Michael5601 Michael5601 force-pushed the ImageLoader-Refactoring branch from e470f1b to c00d202 Compare January 24, 2025 13:50
@Michael5601
Copy link
Contributor Author

Now it should be ready.

Copy link
Member

@HannesWell HannesWell left a comment

Choose a reason for hiding this comment

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

I think we should not complicate this.... reading includes closing, and that a stream is used is and implementation detail, so simply say it do so when I/O error occurs when reading the file is enough.

Fully agree, even though Heiko is technically correct, this is a potential change in behavior, I think this is neglectable. In other clean-ups (from myself and others) in other parts of Eclipse the same simplification was already applied multiple times. So I also think this is fine.

Refactor the file-stream handling by using try-with-resource for opening
and closing them in `ImageLoader` for Cocoa and GTK (Windows already
uses it).
@HannesWell HannesWell force-pushed the ImageLoader-Refactoring branch from c00d202 to 6776734 Compare January 24, 2025 18:19
@HannesWell
Copy link
Member

The failure of browser-tests is unrelated and known, submitting.
Thanks for this code enhancement.

@HannesWell HannesWell merged commit 88d5b53 into eclipse-platform:master Jan 24, 2025
10 of 14 checks passed
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.

4 participants